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: allow minimising the jwt size by omitting additional claims #1920

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

staaldraad
Copy link
Contributor

@staaldraad staaldraad commented Jan 20, 2025

What kind of change does this PR introduce?

Feature

What is the current behavior?

JWT claims are not controllable, other than using a custom access hook. This can lead to large JWTs containing claims that might not be needed.

What is the new behavior?

Adds a configuration to control which claims outside of the required claims can be added to the JWT automatically.

Additional context

To be backward compatible, the default is to include all supported claims in the generated JWT. To have fewer claims, the config option jwt.additional_claims can be modified with the claims to include. Because the currently deployed (hosted and self-hosted) version does not have this config option, the decision to apply this default is based on the config value being empty. And empty value could also mean "don't include any additional claims", which would immediately break backwards compatibility as JWTs would suddenly not contain the optional claims. To simulate an empty set, it is possible to simply include an unknown claim, which would get ignored. It could make sense to standardise on a reserved word for this configuration.

Slightly depends on #1913 to determine if some fields that are not yet omitempty should be set to a default value.

JWTs can be minimal with only required claims set. Add option for user
to configure which additional claims to include.
@coveralls
Copy link

coveralls commented Jan 20, 2025

Pull Request Test Coverage Report for Build 12867239919

Details

  • 22 of 22 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 65.47%

Totals Coverage Status
Change from base Build 12806215187: 0.03%
Covered Lines: 9829
Relevant Lines: 15013

💛 - Coveralls

AppMetaData map[string]interface{} `json:"app_metadata"`
Email string `json:"email,omitempty"`
Phone string `json:"phone,omitempty"`
AppMetaData map[string]interface{} `json:"app_metadata,omitempty"`
UserMetaData map[string]interface{} `json:"user_metadata"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
UserMetaData map[string]interface{} `json:"user_metadata"`
UserMetaData map[string]interface{} `json:"user_metadata,omitempty"`

noticed this while testing by excluding user_metadata in GOTRUE_JWT_ADDITIONAL_CLAIMS and got the following payload in the JWT returned:

{
  "iss": "https://projectref.supabase.co",
  "sub": "0c402da8-f1ac-4bf6-a39e-8f09078940af",
  "aud": "authenticated",
  "exp": 1837442173,
  "iat": 1737442173,
  "user_metadata": null,
  "role": "",
  "aal": "aal1",
  "session_id": "6cfb8463-249a-4a87-b3f8-de0986839220",
  "is_anonymous": false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Comment on lines +336 to +339
AuthenticatorAssuranceLevel: aal.String(),
SessionId: sid,
Role: user.Role,
IsAnonymous: user.IsAnonymous,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's alright for is_anonymous and aal to not be required claims since they are only relevant for folks who:

  1. Have anonymous sign-ins enabled
  2. Use MFA (need to write RLS policies to check against the AAL)

I didn't test this but i'm guessing it's required because if those claims are omitted, they may break existing functionality if either (1) or (2) are enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, I initially made them optional but decided to wait for the decision out of #1913 if we should have them. Some tests also started breaking with these omitted: https://github.com/supabase/auth/actions/runs/12866198657/job/35868267108#step:11:1147, which further motivated me to keep them as it seem a good indicator that they might be required

Copy link
Member

Choose a reason for hiding this comment

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

thought about this a bit more and i think it's safe to let the user decide whether is_anonymous and aal should be present in the JWT as long as we make it clear in the configuration UI that existing RLS policies that check against these claims would fail

@cstockton cstockton added the enhancement New feature or request label Jan 23, 2025
@staaldraad staaldraad changed the title Feat/min jwt feat: allow minimising the jwt size by omitting additional claims Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants