From 725835f7dd47e353c365d503b4e00557195d3943 Mon Sep 17 00:00:00 2001 From: Joel Lee Date: Wed, 13 Mar 2024 08:20:45 +0800 Subject: [PATCH] fix: impose expiry on auth code instead of magic link (#1440) ## What kind of change does this PR introduce? Currently, we check for flow state expiry rather than auth code expiry. The auth code is created at the point when `/magiclink` is called and expiry starts from then. However, the auth code should probably start expiring when the link is verified and the auth code is issued. We can eventually extend this to other magic link like flows if need. Note that the Flow State expiry is capped at 24 hours, as that is when the regular cleanup takes place. Considered adding a hard restriction on the maximum validity of `GOTRUE_MAILER_OTP_EXP` but there are a handful of projects which have it >86400. The handful of existing projects (number on internal channel) with a OTP expiry of longer than 24 hours will continue to have the expiry capped at 24 hours when using PKCE. This should be the same as the current behaviour since we aren't changing the cleanup duration. --------- Co-authored-by: joel --- internal/api/external.go | 3 +++ internal/api/pkce.go | 4 ++++ internal/api/verify.go | 2 +- internal/models/flow_state.go | 13 +++++++++++++ ...0240306115329_add_issued_at_to_flow_state.up.sql | 3 +++ 5 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 migrations/20240306115329_add_issued_at_to_flow_state.up.sql diff --git a/internal/api/external.go b/internal/api/external.go index facb8ce91..177d20059 100644 --- a/internal/api/external.go +++ b/internal/api/external.go @@ -223,6 +223,9 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re flowState.ProviderAccessToken = providerAccessToken flowState.ProviderRefreshToken = providerRefreshToken flowState.UserID = &(user.ID) + issueTime := time.Now() + flowState.AuthCodeIssuedAt = &issueTime + terr = tx.Update(flowState) } else { token, terr = a.issueRefreshToken(ctx, tx, user, models.OAuth, grantParams) diff --git a/internal/api/pkce.go b/internal/api/pkce.go index 7c24b6301..56ac1acb9 100644 --- a/internal/api/pkce.go +++ b/internal/api/pkce.go @@ -45,6 +45,10 @@ func issueAuthCode(tx *storage.Connection, user *models.User, authenticationMeth } else if err != nil { return "", err } + if err := flowState.RecordAuthCodeIssuedAtTime(tx); err != nil { + return "", err + } + return flowState.AuthCode, nil } diff --git a/internal/api/verify.go b/internal/api/verify.go index deb52113d..4af21e023 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -181,9 +181,9 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request, params *VerifyPa if terr := tx.Reload(user); err != nil { return terr } + if isImplicitFlow(flowType) { token, terr = a.issueRefreshToken(ctx, tx, user, models.OTP, grantParams) - if terr != nil { return terr } diff --git a/internal/models/flow_state.go b/internal/models/flow_state.go index 04e880ec2..9a770d81d 100644 --- a/internal/models/flow_state.go +++ b/internal/models/flow_state.go @@ -28,6 +28,7 @@ type FlowState struct { ProviderType string `json:"provider_type" db:"provider_type"` ProviderAccessToken string `json:"provider_access_token" db:"provider_access_token"` ProviderRefreshToken string `json:"provider_refresh_token" db:"provider_refresh_token"` + AuthCodeIssuedAt *time.Time `json:"auth_code_issued_at" db:"auth_code_issued_at"` CreatedAt time.Time `json:"created_at" db:"created_at"` UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } @@ -152,5 +153,17 @@ func (f *FlowState) VerifyPKCE(codeVerifier string) error { } func (f *FlowState) IsExpired(expiryDuration time.Duration) bool { + if f.AuthCodeIssuedAt != nil && f.AuthenticationMethod == MagicLink.String() { + return time.Now().After(f.AuthCodeIssuedAt.Add(expiryDuration)) + } return time.Now().After(f.CreatedAt.Add(expiryDuration)) } + +func (f *FlowState) RecordAuthCodeIssuedAtTime(tx *storage.Connection) error { + issueTime := time.Now() + f.AuthCodeIssuedAt = &issueTime + if err := tx.Update(f); err != nil { + return err + } + return nil +} diff --git a/migrations/20240306115329_add_issued_at_to_flow_state.up.sql b/migrations/20240306115329_add_issued_at_to_flow_state.up.sql new file mode 100644 index 000000000..d6eff157a --- /dev/null +++ b/migrations/20240306115329_add_issued_at_to_flow_state.up.sql @@ -0,0 +1,3 @@ +do $$ begin +alter table {{ index .Options "Namespace" }}.flow_state add column if not exists auth_code_issued_at timestamptz null; +end $$