Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: glee authentication v1 #460

Closed
wants to merge 54 commits into from
Closed

feat: glee authentication v1 #460

wants to merge 54 commits into from

Conversation

oviecodes
Copy link
Contributor

Description
This PR continues the discussion on the glee authentication issue #377

We have moved on from the middleware approach proposed in PR #439 and have started exploring the possibilities of using lifecycle events for authentication. This PR details the progress that has been made using Lifecycle events.

Progress:

  1. Creation of an entirely new lifecycle event (onAuth)
  2. Successful emission of auth event from websocket server

Challenges:

  1. websocket proceeds to connect even when auth file has not finished running due to the nature of node.js events.

We're currently experimenting with potential fixes for this challenge.

Related issue(s)
Discuses issue #377

@Souvikns @KhudaDad414

@oviecodes oviecodes marked this pull request as ready for review June 16, 2023 06:03
@oviecodes oviecodes marked this pull request as draft June 16, 2023 06:05
@Souvikns
Copy link
Member

We should have a separate place for Authentication

We could have a separate place for authentication, instead of lifecycle events and config. We can have both server auth and client auth in the same place.

/** auth/serverName.ts **/

export function serverAuth({headers, done}) {
  // write logic for authentication 
  done()
}

export function clientAuth() {
  return {
    username: '',
    password: ''
  }
}
  • With this, we can remove the auth from the config.
  • Server and client auth stays together and the user does not have to change multiple things.
  • Since this follows the file system like functions we can find out which server the authentication file belongs to and thus figure out if it is http or websocket and call the functions accordingly.

@KhudaDad414, @oviecodes what do you think?

@oviecodes
Copy link
Contributor Author

oviecodes commented Jun 27, 2023

We should have a separate place for Authentication

We could have a separate place for authentication, instead of lifecycle events and config. We can have both servers auth and client auth in the same place.

@KhudaDad414, @oviecodes what do you think?

@Souvikns That's a good approach, we'll have to create a separate folder for authentication alone, since the logic in the auth file has grown past being housed in lifecycle files. Also, we'll have to change how and where the client gets its authentication parameters in the client.ts files of the respective adapters.

One thing on my mind is:
Should this change affect other adapters or should it be for just the WS and HTTP adapters?

@KhudaDad414
Copy link
Member

We could have a separate place for authentication, instead of lifecycle events and config. We can have both server auth and client auth in the same place.

@Souvikns that's a great idea. it would solve lots of issues and makes a lot of sense.

Should this change affect other adapters or should it be for just the WS and HTTP adapters?

@oviecodes For now lets focus on HTTP and WS. that's our goal for GSoC. we can extend it if the need arise.

@KhudaDad414
Copy link
Member

@oviecodes thanks to @Souvikns the formatting issue should be fixed with #469 the formatting issue seems to be solved. let's move your changes to a new PR so we can discuss it further. 😀

@sonarcloud
Copy link

sonarcloud bot commented Jul 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants