Skip to content

Commit

Permalink
fix: add error codes to password login flow (supabase#1721)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* Add error codes to the password login flow, which fixes supabase#1631 

## What is the current behavior?
* Most errors in the password login flow are returned as `oauthError`,
which doesn't have support for an error code as the struct conforms to
the [oauth error
response](https://datatracker.ietf.org/doc/html/rfc6749#section-5.2)
specified in the RFC

## What is the new behavior?
* Errors which were previously returned as an `oauthError` struct now
return `badRequestError` instead with the following error code
`invalid_login_credentials`
* In certain cases, we can return existing error codes like
`ErrorCodeUserBanned`, `ErrorCodeEmailNotConfirmed`,
`ErrorCodePhoneNotConfirmed` or `ErrorCodeValidationFailed`
* Some error messages are updated to provide more clarity on the
underlying error

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
  • Loading branch information
kangmingtay authored Aug 15, 2024
1 parent 4b04327 commit 4351226
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 16 deletions.
4 changes: 3 additions & 1 deletion internal/api/errorcodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,7 @@ const (
ErrorCodeMFAPhoneVerifyDisabled ErrorCode = "mfa_phone_verify_not_enabled"
ErrorCodeMFATOTPEnrollDisabled ErrorCode = "mfa_totp_enroll_not_enabled"
ErrorCodeMFATOTPVerifyDisabled ErrorCode = "mfa_totp_verify_not_enabled"
ErrorCodeVerifiedFactorExists ErrorCode = "mfa_verified_factor_exists"
ErrorCodeMFAVerifiedFactorExists ErrorCode = "mfa_verified_factor_exists"
//#nosec G101 -- Not a secret value.
ErrorCodeInvalidCredentials ErrorCode = "invalid_credentials"
)
2 changes: 1 addition & 1 deletion internal/api/mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *
if factor.Phone.String() == phone {
if factor.IsVerified() {
return unprocessableEntityError(
ErrorCodeVerifiedFactorExists,
ErrorCodeMFAVerifiedFactorExists,
"A verified phone factor already exists, unenroll the existing factor to continue",
)
} else if factor.IsUnverified() {
Expand Down
24 changes: 12 additions & 12 deletions internal/api/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (a *API) Token(w http.ResponseWriter, r *http.Request) error {
case "pkce":
return a.PKCE(ctx, w, r)
default:
return oauthError("unsupported_grant_type", "")
return badRequestError(ErrorCodeInvalidCredentials, "unsupported_grant_type")
}
}

Expand Down Expand Up @@ -131,22 +131,22 @@ func (a *API) ResourceOwnerPasswordGrant(ctx context.Context, w http.ResponseWri
params.Phone = formatPhoneNumber(params.Phone)
user, err = models.FindUserByPhoneAndAudience(db, params.Phone, aud)
} else {
return oauthError("invalid_grant", InvalidLoginMessage)
return badRequestError(ErrorCodeValidationFailed, "missing email or phone")
}

if err != nil {
if models.IsNotFoundError(err) {
return oauthError("invalid_grant", InvalidLoginMessage)
return badRequestError(ErrorCodeInvalidCredentials, InvalidLoginMessage)
}
return internalServerError("Database error querying schema").WithInternalError(err)
}

if !user.HasPassword() {
return oauthError("invalid_grant", InvalidLoginMessage)
return badRequestError(ErrorCodeInvalidCredentials, InvalidLoginMessage)
}

if user.IsBanned() {
return oauthError("invalid_grant", InvalidLoginMessage)
return badRequestError(ErrorCodeUserBanned, "User is banned")
}

isValidPassword, shouldReEncrypt, err := user.Authenticate(ctx, db, params.Password, config.Security.DBEncryption.DecryptionKeys, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID)
Expand Down Expand Up @@ -185,8 +185,7 @@ func (a *API) ResourceOwnerPasswordGrant(ctx context.Context, w http.ResponseWri
Valid: isValidPassword,
}
output := hooks.PasswordVerificationAttemptOutput{}
err := a.invokeHook(nil, r, &input, &output)
if err != nil {
if err := a.invokeHook(nil, r, &input, &output); err != nil {
return err
}

Expand All @@ -199,17 +198,17 @@ func (a *API) ResourceOwnerPasswordGrant(ctx context.Context, w http.ResponseWri
return err
}
}
return oauthError("invalid_grant", InvalidLoginMessage)
return badRequestError(ErrorCodeInvalidCredentials, output.Message)
}
}
if !isValidPassword {
return oauthError("invalid_grant", InvalidLoginMessage)
return badRequestError(ErrorCodeInvalidCredentials, InvalidLoginMessage)
}

if params.Email != "" && !user.IsConfirmed() {
return oauthError("invalid_grant", "Email not confirmed")
return badRequestError(ErrorCodeEmailNotConfirmed, "Email not confirmed")
} else if params.Phone != "" && !user.IsPhoneConfirmed() {
return oauthError("invalid_grant", "Phone not confirmed")
return badRequestError(ErrorCodePhoneNotConfirmed, "Phone not confirmed")
}

var token *AccessTokenResponse
Expand Down Expand Up @@ -292,7 +291,8 @@ func (a *API) PKCE(ctx context.Context, w http.ResponseWriter, r *http.Request)
}
token, terr = a.issueRefreshToken(r, tx, user, authMethod, grantParams)
if terr != nil {
return oauthError("server_error", terr.Error())
// error type is already handled in issueRefreshToken
return terr
}
token.ProviderAccessToken = flowState.ProviderAccessToken
// Because not all providers give out a refresh token
Expand Down
4 changes: 2 additions & 2 deletions internal/conf/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -732,15 +732,15 @@ func (config *GlobalConfiguration) ApplyDefaults() error {
config.JWT.AdminGroupName = "admin"
}

if config.JWT.AdminRoles == nil || len(config.JWT.AdminRoles) == 0 {
if len(config.JWT.AdminRoles) == 0 {
config.JWT.AdminRoles = []string{"service_role", "supabase_admin"}
}

if config.JWT.Exp == 0 {
config.JWT.Exp = 3600
}

if config.JWT.Keys == nil || len(config.JWT.Keys) == 0 {
if len(config.JWT.Keys) == 0 {
// transform the secret into a JWK for consistency
privKey, err := jwk.FromRaw([]byte(config.JWT.Secret))
if err != nil {
Expand Down

0 comments on commit 4351226

Please sign in to comment.