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

Improve use of OAuth2 tokens #298

Open
Willdotwhite opened this issue Apr 23, 2024 · 0 comments
Open

Improve use of OAuth2 tokens #298

Willdotwhite opened this issue Apr 23, 2024 · 0 comments
Labels
api Issue specific to the API component discussion Extra attention is needed to discuss issue

Comments

@Willdotwhite
Copy link
Collaborator

Willdotwhite commented Apr 23, 2024

I spoke to an OAuth guru to check where we're at and get some guidance, as my OAuth2 knowledge is a bit fuzzy. Our login mechanism has a few oddities that would be nice to sort out, but it's by no means a priority.

tldr What we have now currently works, isn't a security risk, and doesn't cause any major problems. However, we could tidy this up in one of two ways if we wanted to tidy things up a bit. The way proposed below is only one option, but it changes little and entirely removes the team-finder.auth table in the DB, at the expense of the discord bot being given more work to do.

Current process/data flow

  1. User triggers login (clicks Login button, tries to view authenticated-only page, etc)
  2. User is redirected to discord.com using the authorisation code flow
  3. User logs in, is redirected from discord back to API
  4. In 3, we are sent this standard token set
  5. We store these tokens in the DB alongside a unique securely random id token
  6. User is redirected back to UI with a JWT. This JWT only contains id as a meaningful piece of data
  7. On every authorised action afterwards, the JWT is sent in an Authorization: Bearer xxxxxxx header
  8. We use this header to make a /userinfo call to discord to retrieve the user ID and their name

This flow worked sterlingly at launch, but over time is now slightly disjointed for our needs. For context, at the original time of development:

  • we didn't have the bot integration we do now (which can do most of the work we use /userinfo for)
  • Ktor didn't natively support a Bearer token (or any other simple API auth mechanism that worked across domains)
  • we needed a cross-domain compliant approach
  • (IIRC) we also needed it pretty quickly

Proposal - user and guild IDs in the JWT, stop storing all tokens

We could fully put more user data into the JWT and use this as the user data store without hitting discord per request. This would save time hitting discord, although some changes to the user state will only update on logout/login (such as server nickname).

In order to get this data (e.g. server nickname, so we can display a "Hi $name" message on login) we could use the Javacord bot integration we now have. The only piece of data we get from /userinfo that we need is the unique discord user ID. That ID (plus the guild it is attached to) is enough for the Javacord bot to get the guild status, server nickname, DM permissions etc. status that we rely on.

We could instead just track the user ID and guild ID in the JWT (plus perhaps any info used by the UI, like nickname), bin off our external /userinfo endpoint (as we would pull this data once by hitting discord and saving in the JWT), and let the bot do the work of wrangling discord calls.

The end login flow would look something like this:

  1. User triggers login (clicks Login button, tries to view authenticated-only page, etc)
  2. User is redirected to discord.com using the authorisation code flow
  3. User logs in, is redirected from discord back to API
  4. In 3, we are sent this standard token set
  5. Using the accessToken only, we hit the discord API to retrieve the discord ID for this user
  6. User is redirected back to UI with a JWT. This new JWT contains the discord ID and guild ID of the current login session
  7. On every authorised action afterwards, the JWT is sent in an Authorization: Bearer xxxxxxx header
  8. We unpack this header and use the IDs to perform authenticated actions (no repeated hitting of discord required)

JWT schema

The proposed JWT would look something like this:

{
  "exp": <int: timestamp of token expiry>,
  "iat": <int: timestamp of token issue>,
  "iss": "findyourjam.team",
  "sub": <string: ID of the jam; e.g. "gmtk">,
  "discordId": <string: ID of user in discord>,
  "guildId": <string, ID of guild user has logged into in discord>
}

The guildId is not a required field here, but would have a database lookup on requests that need to translate from a local jam ID gmtk into the guild ID. This won't be a blocker yet but will be relevant if/when this project handles multiple jams.

Related tasks

Currently the signing secret for all JWTs is secret (in the application.conf file). This isn't a security problem because the wrapped value is a secure random string; if we move to using plaintext values in the JWT, the JWT config values need updating and jwt.secret needs to not live in code!

Although this proposal removes /userinfo as a mechanism to query discord, we do need a way to validate the sent JWT. The JWT needs to be signed correctly and includes an expiry field which we use as the TTL for a login session, both of which need using; in order to improve the UX for a user who's session has expired, we could (but I don't think it's a requirement) to have a /validate endpoint which we call asynchronously from the UI. I'm a little hazy on the OAuth2 spec, but my gut reaction is this would let us gracefully expire a login session without waiting for the user to try and perform an authenticated action.

@Willdotwhite Willdotwhite added discussion Extra attention is needed to discuss issue api Issue specific to the API component labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue specific to the API component discussion Extra attention is needed to discuss issue
Projects
None yet
Development

No branches or pull requests

1 participant