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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions example.env
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ GOTRUE_JWT_EXP="3600"
GOTRUE_JWT_AUD="authenticated"
GOTRUE_JWT_DEFAULT_GROUP_NAME="authenticated"
GOTRUE_JWT_ADMIN_ROLES="supabase_admin,service_role"
GOTRUE_JWT_ADDITIONAL_CLAIMS="email,phone,app_metadata,user_metadata,amr,is_anonymous"

# Database & API connection details
GOTRUE_DB_DRIVER="postgres"
Expand Down
35 changes: 23 additions & 12 deletions internal/api/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ import (
// AccessTokenClaims is a struct thats used for JWT claims
type AccessTokenClaims struct {
jwt.RegisteredClaims
Email string `json:"email"`
Phone string `json:"phone"`
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!

Role string `json:"role"`
AuthenticatorAssuranceLevel string `json:"aal,omitempty"`
Expand Down Expand Up @@ -333,15 +333,26 @@ func (a *API) generateAccessToken(r *http.Request, tx *storage.Connection, user
ExpiresAt: jwt.NewNumericDate(expiresAt),
Issuer: config.JWT.Issuer,
},
Email: user.GetEmail(),
Phone: user.GetPhone(),
AppMetaData: user.AppMetaData,
UserMetaData: user.UserMetaData,
Role: user.Role,
SessionId: sid,
AuthenticatorAssuranceLevel: aal.String(),
AuthenticationMethodReference: amr,
IsAnonymous: user.IsAnonymous,
AuthenticatorAssuranceLevel: aal.String(),
SessionId: sid,
Role: user.Role,
IsAnonymous: user.IsAnonymous,
Comment on lines +336 to +339
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

}

// add additional claims that are optional
for _, ac := range config.JWT.AdditionalClaims {
switch ac {
case "email":
claims.Email = user.GetEmail()
case "phone":
claims.Phone = user.GetPhone()
case "app_metadata":
claims.AppMetaData = user.AppMetaData
case "user_metadata":
claims.UserMetaData = user.UserMetaData
case "amr":
claims.AuthenticationMethodReference = amr
}
}

var gotrueClaims jwt.Claims = claims
Expand Down
64 changes: 64 additions & 0 deletions internal/api/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -855,3 +855,67 @@ $$;`
})
}
}

func (ts *TokenTestSuite) TestConfigureAccessToken() {
type customAccessTokenTestcase struct {
desc string
additionalClaimsConfig []string
expectedClaims []string
}
requiredClaims := []string{"aud", "exp", "iat", "sub", "role", "aal", "session_id", "user_metadata", "is_anonymous"}
cases := []customAccessTokenTestcase{

{
desc: "Default claims all present",
additionalClaimsConfig: []string{"empty"},
expectedClaims: requiredClaims,
}, {
desc: "Minimal set of claims is returned",
additionalClaimsConfig: []string{},
expectedClaims: requiredClaims,
},
{
desc: "Selected additional claims are returned",
additionalClaimsConfig: []string{"email"},
expectedClaims: append(requiredClaims, []string{"email"}...),
},
}
for _, c := range cases {
ts.T().Run(c.desc, func(t *testing.T) {
ts.Config.JWT.AdditionalClaims = c.additionalClaimsConfig

var buffer bytes.Buffer
require.NoError(t, json.NewEncoder(&buffer).Encode(map[string]interface{}{
"refresh_token": ts.RefreshToken.Token,
}))

req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer)
req.Header.Set("Content-Type", "application/json")

w := httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)

var tokenResponse struct {
AccessToken string `json:"access_token"`
}
require.NoError(t, json.NewDecoder(w.Result().Body).Decode(&tokenResponse))
parts := strings.Split(tokenResponse.AccessToken, ".")
require.Equal(t, 3, len(parts), "Token should have 3 parts")

payload, err := base64.RawURLEncoding.DecodeString(parts[1])
require.NoError(t, err)

var responseClaims map[string]interface{}
require.NoError(t, json.Unmarshal(payload, &responseClaims))

assert.Len(t, responseClaims, len(c.expectedClaims), "More or less claims in the response than expected")
for _, expectedClaim := range c.expectedClaims {
_, exists := responseClaims[expectedClaim]
assert.True(t, exists, "Claim should exist {%s}", expectedClaim)
}

// reset
ts.Config.JWT.AdditionalClaims = []string{"email", "phone", "app_metadata", "user_metadata", "amr", "is_anonymous"}
})
}
}
10 changes: 10 additions & 0 deletions internal/conf/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ type JWTConfiguration struct {
KeyID string `json:"key_id" split_words:"true"`
Keys JwtKeysDecoder `json:"keys"`
ValidMethods []string `json:"-"`
AdditionalClaims []string `json:"additional_claims" split_words:"true"`
}

type MFAFactorTypeConfiguration struct {
Expand Down Expand Up @@ -886,6 +887,15 @@ func (config *GlobalConfiguration) ApplyDefaults() error {
config.JWT.AdminRoles = []string{"service_role", "supabase_admin"}
}

// default to all claims that were / are available at the time of this change
// to ensure backwards compatibility. To exclude all these claims, the value
// of jwt.additional_claims can be set to an invalid claim, such as "none", "empty", "null"
// also allow setting to default claims using the "default" keyword, making it possible to use
// this config as a binary flag "none" == use_mimimal_jwt == true, "default" == use_mimimal_jwt == false
if len(config.JWT.AdditionalClaims) == 0 || (len(config.JWT.AdditionalClaims) == 1 && config.JWT.AdditionalClaims[0] == "default") {
config.JWT.AdditionalClaims = []string{"email", "phone", "app_metadata", "user_metadata", "amr"}
}

if config.JWT.Exp == 0 {
config.JWT.Exp = 3600
}
Expand Down
8 changes: 4 additions & 4 deletions internal/hooks/auth_hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,15 @@ const MinimumViableTokenSchema = `{
"type": "string"
}
},
"required": ["aud", "exp", "iat", "sub", "email", "phone", "role", "aal", "session_id", "is_anonymous"]
"required": ["aud", "exp", "iat", "sub", "role", "aal", "session_id"]
}`

// AccessTokenClaims is a struct thats used for JWT claims
type AccessTokenClaims struct {
jwt.RegisteredClaims
Email string `json:"email"`
Phone string `json:"phone"`
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"`
Role string `json:"role"`
AuthenticatorAssuranceLevel string `json:"aal,omitempty"`
Expand Down
Loading