140 cognito

[chnnick]

ℹ️ Issue

Closes #140

📝 Description

Created an easily-importable NestJS guard that can be used by future projects to implement Cognito Authentication into their application. Amplify on the frontend authenticates users, providing registered users with a JWT access token that can be used to access authorized routes.

Briefly list the changes made to the code:

1. NestJS guard that checks and verifies the Bearer token in the authorization header via AWS Cognito to determine access onto specific routes, puts the verified token payload under a user field of the request that can be read by other methods.
2. CognitoService getUser() method that exposes the verified JWT payload attached to the request by the guard
3. Cognito module that imported into App Module for authentication across entire application
4. @public decorator that allows one to bypass authentication by putting metadata read by the guard
5. Interface for an expected JWT payload used by the guard and service
6. Cognito config that allows for a centralized place to extract env variables for Cognito
7. Tests for the cognito service and cognito guard
8. Implemented Amplify on the frontend to show an auth screen and configure auth for the application if frontend cognito env variables are set, if anyfrontend cognito env values are not set, there is no auth screen
9. Cognito .env variables in example.env that is used by both the backend (Cognito) and frontend (Amplify)
10. README.md in backend/aws/cognito for new devs/TLs that provides documentation for how the authentication flow works: Authentication in frontend -> Authorization in the backend, as well as how to expand in the future.

✔️ Verification

Backend TESTS:

Frontend TESTS:
No Auth:

With Auth:

🏕️ (Optional) Future Work / Notes

• No styling on auth page, can import the @aws-amplify/ui-react/styles.css library on main.tsx to use their premade styling?

NEW: I think that a workshop on Authentication / Authorization would be helpful for future devs, would 100% be up to helping with that! I spent wayyy too much time figuring out the difference between the two and their uses in larger apps like which scaffolding will fork off of.

[dburkhart07]

Amazing! Very educational for me to read through as well!

[dburkhart07]

few small things

[dburkhart07]

should check that this is in the past

[chnnick]

hmm, not sure what you mean by the past? jwt.verify will check for expired tokens though and return err on the guard.

[chnnick]

bump: Not sure what this meant mb

[dburkhart07]

hopefully some last comments. c4c security is becoming so good!!!!!!

[dburkhart07]

can we just make this a function just like the backend, and call that the same way?

[chnnick]

For the backend I changed it to check if cognito config returns a real config or null which is contingent on having those variables present. I think they're already the same? I can just mirror both config setups to empty (frontend) or null (backend) if the variables are missing or empty.

[chnnick]

They now mirror eachother (Just see if they are null or empty values)

[dburkhart07]

we should make this function the exact same thing as the frontend as well, so we can reject null entries and dont have to worry at all about the frontend and backend disagreeing about whether auth is enabled or not.

[chnnick]

The functions (isAuthEnabled and isNonEmptyEnv) for checking if auth is enabled or not is the exact same, but I can maybe move that functionality to the /shared folder so we know 100% it is identical??

[dburkhart07]

i think, for modules that rely on env variables to function entirely (so all of our aws modules), we should be validating the env variables at compile time. can we change this to rather use something like this:

  export class CognitoModule implements OnModuleInit {
    private readonly logger = new Logger(CognitoModule.name);

    onModuleInit() {
      const config = isAuthEnabled();
      if (config === null) {
        this.logger.warn('Cognito auth disabled: no env vars set. All routes open.');
      } else {
        this.logger.log(`Cognito auth enabled for pool ${config.userPoolId}`);
      }
    }
  }

while you are at it, would you mind making this change for the s3 and ses, where we would check the specific env variables that we need to check for ses being enabled, and s3 being enabled?

[chnnick]

Ill put the s3 ses changes into a separate PR

[dburkhart07]

all nits are small, so im going to improve. make sure after all these changes the README is fully up to date. thanks for doing so much work on this, was by far the hardest module to implement!

[dburkhart07]

i think, if any of these 3 are not set, we should return null. when we go to getCognitoConfig, if its null we should instead say one of the env vars are not set, so cognito is not enabled

[chnnick]

Rn it is already checking to see that if not all 3 are set, then getcognitoconfig() returns null. IsAuthEnabled() is a function used by both the guard and the module (OnInit) and log warnings in the console saying that the env vars are not set. That being said i agree the logic is a bit confusing. Will go ahead and take out the unneccessary demorgans law.

[dburkhart07]

now that i think about this, we should log an error when in production. for dev, a warning is fine. can we put somewhere in the readme that, for production, these should be changed to throw warnings instead, should they be used?

[chnnick]

Yes, will add to readme! I actually agree with what we talked about previously on how we may not want to disable auth entirely by default if just one cognito env value is missing, and instead loudly fail the backend on startup? - unless I misread our convo

[dburkhart07]

i know i said the opposite before, but i think we can remove the region from the checking altogether. the region is only used here, and it can be derived from the userPoolId (const region = userPoolId.split('_')[0];)

we should do this instead and remove the env variable checks altogether. this will make it much easier to avoid any potential configuration issues

[chnnick]

Quick clarification: You mean remove the env variable checks for just region correct?

[dburkhart07]

this is a question for when @maxn990 reviews this. with this flow, do we still need the auth directory, or anything from within it? i know ssf used little to nothing from it, so i wonder if part of the pr should be to delete this. it is scaffolding though, so if we want the option im not against keeping it.

[maxn990]

this is a question for when @maxn990 reviews this. with this flow, do we still need the auth directory, or anything from within it? i know ssf used little to nothing from it, so i wonder if part of the pr should be to delete this. it is scaffolding though, so if we want the option im not against keeping it.

I'm good with deleting it if we're not using it

[chnnick]

this is a question for when @maxn990 reviews this. with this flow, do we still need the auth directory, or anything from within it? i know ssf used little to nothing from it, so i wonder if part of the pr should be to delete this. it is scaffolding though, so if we want the option im not against keeping it.

I'm good with deleting it if we're not using it

Since we use amplify Authenticator to handle login/logout/signup a lot of the functionality built into the auth directory is no longer needed. New cognito module just verifies an cognito-issued access token to authorize access. We will need to edit the users folder though @dburkhart07 @maxn990 I can put that also in another pr after this is merged with the deleted /auth/ directory?

edit: won't allow me to commit with those missing deleted dependencies so just going to edit them now.