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

Add proper roles and User service to JWT authorization #2058

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Mar 31, 2021

This does a number of things toward #2053, specifically:

  • Updates the Auth service to know about the User service, so that we can look-up user info with the Users service during authentication. This depends on Add support for generating a service token DevelopingSpace/satellite#10 for the service token logic. NOTE: this will mean our e2e tests are going to fail, since we'll need to put a user into Firebase before we can do the entire authentication flow.
  • Adds name and roles claims to the JWT, so that we can include the user's display name (for convenience) and whether they are a user or admin, used by Satellite authorization middleware

I noticed while writing this that the schema we have for a user in the User service needs to be updated to include email and/or I need to change my code to use the id we get back from the Users service to set the email. @chrispinkney we should figure this out ASAP, since a lot of things depend on the layout of these objects.

@humphd humphd added Blocked Can't do this, until something else is done type: security Security concerns area: microservices area: satellite Issues related to the Satellite microservice project labels Mar 31, 2021
@humphd humphd self-assigned this Mar 31, 2021
};

const options = { expiresIn: JWT_EXPIRES_IN || '1h' };
const options = { expiresIn: JWT_EXPIRES_IN || '3h' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for us to extend this for PWA? I think most mobile apps let you stay logged in for a longer period of time than websites

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some risk in having tokens be long-lived, since they are effectively passwords that anyone can re-use without having to prove themselves. We can extend this, for sure, but for how long? With systems that do cookie based sessions on the server, you can have a login live forever, and not risk much. With tokens that a browser holds, it's harder. We could look into doing some kind of "refresh" token later, to let you renew your login.

Question: what would you do while logged in for longer? The only time you need to log in is when you create or modify your account.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understandable. I was just thinking if there is a safe way to let users stay logged in while using PWA. To answer your question, that's true, but only for now ;)

birtony
birtony previously approved these changes Mar 31, 2021
@humphd
Copy link
Contributor Author

humphd commented Mar 31, 2021

Depends on #2060

@humphd
Copy link
Contributor Author

humphd commented Apr 6, 2021

@chrispinkney @manekenpix @birtony @Metropass @HyperTHD this is finished (woo!) and ready for another review. It's exceedingly hard to test locally, so the best thing to do is to read the e2e tests that I've written, and make sure they pass on CI. Here's what I've done:

A user clicks Login in the front-end, it will go to the Auth service, and that will redirect to the SSO, where they enter credentials. They then get redirected back to the Auth service. Here, we use their Seneca email to check if a user exists in the Users service. If one does, we grab that Telescope profile info and hold on to it for later; if not, we use their Seneca profile info instead.

We then create a JWT token for the user, and populate it with info for the front-end. This includes their name, sub (subject in JWT--their email), picture (if we know it from GitHub), and roles. The last one is an array of all authorized roles the user has: seneca (they are an authenticated Seneca SSO user); telescope (they are an authenticated Seneca SSO user who also exists in Telescope's Users service); admin (they are a Seneca + Telescope user but also have isAdmin=true in Firebase).

I've integrated all the pieces from various other PRs in Satellite and Telescope to do this, and also added a bit of convenience code for running things locally in development.

After this lands, we can update the front-end to use the name + picture from the token, and also start protecting all of our routes that need it.

Metropass
Metropass previously approved these changes Apr 8, 2021
Copy link
Contributor

@Metropass Metropass left a comment

Choose a reason for hiding this comment

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

Took a while on my machine, but everything passed.

);

// Delete the Telescope users we created in the Users service.
const cleanupTelescopeUsers = () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is part of the test, but the function itself is quite useful for removing Users. Maybe we could salvage this into a function that cleans up any banned users?

manekenpix
manekenpix previously approved these changes Apr 8, 2021
chrispinkney
chrispinkney previously approved these changes Apr 8, 2021
DukeManh
DukeManh previously approved these changes Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: microservices area: satellite Issues related to the Satellite microservice project Blocked Can't do this, until something else is done type: security Security concerns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants