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/jwt authentication #101

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

iankressin
Copy link

@iankressin iankressin commented Mar 6, 2023

Description

This pull request adds the option of sign in using JWT instead of session cookies.

Users can now pass the an authentication method to SSXServer constructor, which will tell the login route which type of authentication should be forward to the client, once the nonce signature is validated.

The signingKey prop from SSXServer was reused to server as private key to the sign method from jsonwebtoken lib.

Nothing was changed on the /ssx-nonce route, since session is still being used to record users nonce during the login process. The difference is that we now call req.session.destroy on /ssx-login route if authentication method is JWT and the login was successful, therefore no further session will be needed, all auth data will live on the client.

Also, /ssx-logout stays the same, because as mentioned above, the auth data lives on the client-side, therefore the there's nothing we have to do about that on the server.

This PR only provides support for express so far. Once we agree on the details of the implementation I'll implement for http package as well

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

System Configuration

(Please delete options that are not relevant)
Runtime version(s):
Browser version(s):
Firmware version(s):
SDK version(s):

Diligence Checklist

(Please delete options that are not relevant)

  • This change requires a documentation update
  • I have included unit tests
  • I have updated and/or included new integration tests
  • I have updated and/or included new end-to-end tests
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@iankressin iankressin marked this pull request as draft March 6, 2023 13:18
@iankressin iankressin marked this pull request as draft March 6, 2023 13:18
@CLAassistant
Copy link

CLAassistant commented Mar 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lilyannehall lilyannehall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I see this PR was marked as a draft, but wanted to take a quick look and leave a couple notes and check in to see if you need any support getting tests and CI passing and ready for review!

if (ssx.authenticationMethod === SSXAuthenticationMethod.COOKIES) {
Object.assign(req.session, payload);
req.session.save(() => res.status(200).json({ ...req.session }));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make this condition explicit instead of falling through? Does this make JWT usage the default?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default authentication is SSXAuthenticationMethod.COOKIES, it's defined inside SSXServer constructor.

As we have a default value for authenticationMethod handling each method explicitly makes a lot of sense,

@@ -32,7 +32,9 @@ export type SSXClientSession = {
walletAddress: string;
chainId: number;
/** Key to identify the session */
sessionKey: string;
sessionKey?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session key now an optional field?

Copy link
Author

@iankressin iankressin Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JWT doesn't require a session on the server, therefore a sessionKey may not exist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, if we aren't doing the not-null check at this layer, probably need to add an assertion in the cookies flow to make sure one is provided in that case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well noted, I'll take a closer look at it!

Copy link
Author

@iankressin iankressin Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently a check for the session key is already being done here,

I believe that this assertion is enough. What do you think?

@franklovefrank
Copy link
Contributor

Hey @iankressin, I reviewed your work so far and it looks like you've made really great progress! I just wanted to check in with you to see how things are going on the ticket. Please feel free to reach out if you are blocked or otherwise want to chat about the implementation

@iankressin
Copy link
Author

Hey @iankressin, I reviewed your work so far and it looks like you've made really great progress! I just wanted to check in with you to see how things are going on the ticket. Please feel free to reach out if you are blocked or otherwise want to chat about the implementation

Hey @franklovefrank! I just start to work on the http implementation and also fixed the issues @44203 pointed out. It's likely that the PR will be ready for another round of review late next week

@iankressin
Copy link
Author

Hi, I see this PR was marked as a draft, but wanted to take a quick look and leave a couple notes and check in to see if you need any support getting tests and CI passing and ready for review!

I guess I can your help right now.

I'm trying to test write a unity test for the http implementation method that looks like this:

describe.only('Should log in using JWT and COOKIES', () => {
  const ssx = new SSXServer({
    signingKey: 'FAKESIGNINKEY',
    authenticationMethod: SSXAuthenticationMethod.JWT,
  });
  const ssxMiddleware = SSXHttpMiddleware(ssx);
  const app = http.createServer(ssxMiddleware());

  test('Should receive JWT in response of a successful log in', async () => {
    const res = await request(app).post('/ssx-login').send({
      siwe: SIWE_MESSAGE,
      signature: SIGNATURE,
      daoLogin: false,
      resolveEns: false,
    });

    expect(res.statusCode).toEqual(200);
  });
});

And I get and error back saying:

'Expected the field nonce to be set on this session.'

Which makes a lot of sense since I'm not calling /ssx-nonce before. So the question is: is there anyway of mocking the session for the http implementation?

@franklovefrank
Copy link
Contributor

hey ian! you can look at an example of mocking the nonce in the tests here f8fc04d#diff-cd43c404c5fe37cbc7ea653c3cc90034d684397c500d227f536702dde74c044 . let me know if this helps

@iankressin
Copy link
Author

hey ian! you can look at an example of mocking the nonce in the tests here f8fc04d#diff-cd43c404c5fe37cbc7ea653c3cc90034d684397c500d227f536702dde74c044 . let me know if this helps

Thanks @franklovefrank I'll take a look into that!

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