From d499c3b132bd1fa756b0a58de08276a02da2ffe8 Mon Sep 17 00:00:00 2001 From: Oliver Eikemeier Date: Mon, 9 Mar 2026 23:36:47 +0100 Subject: [PATCH] chore: rename `AccessTokenError` to `AccessTokenErrorResponse` (#11595) AccessTokenError is never used as a Go error. In fact, it is returned as a *AccessTokenError (which would result in a `nil` error when cast). Rename the struct to a more accurate name and remove the unused `Error() string` method to prevent future confusion. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11595 Reviewed-by: Gusted Co-authored-by: Oliver Eikemeier Co-committed-by: Oliver Eikemeier --- routers/web/auth/oauth.go | 63 +++++++++++++++------------------ tests/integration/oauth_test.go | 43 +++++++++------------- 2 files changed, 46 insertions(+), 60 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 8e8ede0008..22d8c7b846 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -112,18 +112,13 @@ const ( AccessTokenErrorCodeInvalidScope = "invalid_scope" ) -// AccessTokenError represents an error response specified in RFC 6749 +// AccessTokenErrorResponse represents an error response specified in RFC 6749 // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 -type AccessTokenError struct { +type AccessTokenErrorResponse struct { ErrorCode AccessTokenErrorCode `json:"error" form:"error"` ErrorDescription string `json:"error_description"` } -// Error returns the error message -func (err AccessTokenError) Error() string { - return fmt.Sprintf("%s: %s", err.ErrorCode, err.ErrorDescription) -} - // errCallback represents a oauth2 callback error type errCallback struct { Code string @@ -154,10 +149,10 @@ type AccessTokenResponse struct { IDToken string `json:"id_token,omitempty"` } -func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, serverKey, clientKey jwtx.SigningKey) (*AccessTokenResponse, *AccessTokenError) { +func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, serverKey, clientKey jwtx.SigningKey) (*AccessTokenResponse, *AccessTokenErrorResponse) { if setting.OAuth2.InvalidateRefreshTokens { if err := grant.IncreaseCounter(ctx); err != nil { - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidGrant, ErrorDescription: "cannot increase the grant counter", } @@ -174,7 +169,7 @@ func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, ser } signedAccessToken, err := accessToken.SignToken(serverKey) if err != nil { - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot sign token", } @@ -192,7 +187,7 @@ func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, ser } signedRefreshToken, err := refreshToken.SignToken(serverKey) if err != nil { - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot sign token", } @@ -203,7 +198,7 @@ func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, ser if grant.ScopeContains("openid") { app, err := auth.GetOAuth2ApplicationByID(ctx, grant.ApplicationID) if err != nil { - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot find application", } @@ -211,13 +206,13 @@ func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, ser user, err := user_model.GetUserByID(ctx, grant.UserID) if err != nil { if user_model.IsErrUserNotExist(err) { - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot find user", } } log.Error("Error loading user: %v", err) - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "server error", } @@ -251,7 +246,7 @@ func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, ser groups, err := getOAuthGroupsForUser(ctx, user, onlyPublicGroups) if err != nil { log.Error("Error getting groups: %v", err) - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "server error", } @@ -261,7 +256,7 @@ func newAccessTokenResponse(ctx go_context.Context, grant *auth.OAuth2Grant, ser signedIDToken, err = idToken.SignToken(clientKey) if err != nil { - return nil, &AccessTokenError{ + return nil, &AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot sign token", } @@ -711,7 +706,7 @@ func AccessTokenOAuth(ctx *context.Context) { if authType, authData, ok := strings.Cut(authHeader, " "); ok && strings.EqualFold(authType, "Basic") { clientID, clientSecret, err := base.BasicAuthDecode(authData) if err != nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot parse basic auth header", }) @@ -719,7 +714,7 @@ func AccessTokenOAuth(ctx *context.Context) { } // validate that any fields present in the form match the Basic auth header if form.ClientID != "" && form.ClientID != clientID { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "client_id in request body inconsistent with Authorization header", }) @@ -727,7 +722,7 @@ func AccessTokenOAuth(ctx *context.Context) { } form.ClientID = clientID if form.ClientSecret != "" && form.ClientSecret != clientSecret { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "client_secret in request body inconsistent with Authorization header", }) @@ -743,7 +738,7 @@ func AccessTokenOAuth(ctx *context.Context) { var err error clientKey, err = jwtx.CreateSigningKey(serverKey.SigningMethod().Alg(), []byte(form.ClientSecret)) if err != nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "Error creating signing key", }) @@ -757,7 +752,7 @@ func AccessTokenOAuth(ctx *context.Context) { case "authorization_code": handleAuthorizationCode(ctx, form, serverKey, clientKey) default: - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnsupportedGrantType, ErrorDescription: "Only refresh_token or authorization_code grant type is supported", }) @@ -767,7 +762,7 @@ func AccessTokenOAuth(ctx *context.Context) { func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, serverKey, clientKey jwtx.SigningKey) { app, err := auth.GetOAuth2ApplicationByClientID(ctx, form.ClientID) if err != nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidClient, ErrorDescription: fmt.Sprintf("cannot load client with client id: %q", form.ClientID), }) @@ -782,7 +777,7 @@ func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, server } // "invalid_client ... Client authentication failed" // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidClient, ErrorDescription: errorDescription, }) @@ -791,7 +786,7 @@ func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, server token, err := oauth2.ParseToken(form.RefreshToken, serverKey) if err != nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "unable to parse refresh token", }) @@ -800,7 +795,7 @@ func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, server // get grant before increasing counter grant, err := auth.GetOAuth2GrantByID(ctx, token.GrantID) if err != nil || grant == nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidGrant, ErrorDescription: "grant does not exist", }) @@ -809,7 +804,7 @@ func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, server // check if token got already used if setting.OAuth2.InvalidateRefreshTokens && (grant.Counter != token.Counter || token.Counter == 0) { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "token was already used", }) @@ -827,7 +822,7 @@ func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, server func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, serverKey, clientKey jwtx.SigningKey) { app, err := auth.GetOAuth2ApplicationByClientID(ctx, form.ClientID) if err != nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidClient, ErrorDescription: fmt.Sprintf("cannot load client with client id: '%s'", form.ClientID), }) @@ -838,14 +833,14 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s if form.ClientSecret == "" { errorDescription = "invalid empty client secret" } - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: errorDescription, }) return } if form.RedirectURI != "" && !app.ContainsRedirectURI(form.RedirectURI) { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "unexpected redirect URI", }) @@ -853,7 +848,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s } authorizationCode, err := auth.GetOAuth2AuthorizationByCode(ctx, form.Code) if err != nil || authorizationCode == nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "client is not authorized", }) @@ -861,7 +856,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s } // check if code verifier authorizes the client, PKCE support if !authorizationCode.ValidateCodeChallenge(form.CodeVerifier) { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "failed PKCE code challenge", }) @@ -869,7 +864,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s } // check if granted for this application if authorizationCode.Grant.ApplicationID != app.ID { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidGrant, ErrorDescription: "invalid grant", }) @@ -877,7 +872,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s } // remove token from database to deny duplicate usage if err := authorizationCode.Invalidate(ctx); err != nil { - handleAccessTokenError(ctx, AccessTokenError{ + handleAccessTokenError(ctx, AccessTokenErrorResponse{ ErrorCode: AccessTokenErrorCodeInvalidRequest, ErrorDescription: "cannot proceed your request", }) @@ -891,7 +886,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s ctx.JSON(http.StatusOK, resp) } -func handleAccessTokenError(ctx *context.Context, acErr AccessTokenError) { +func handleAccessTokenError(ctx *context.Context, acErr AccessTokenErrorResponse) { ctx.JSON(http.StatusBadRequest, acErr) } diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 21a1ff561f..2533a64a3d 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -249,8 +249,8 @@ func TestAccessTokenExchangeWithoutPKCE(t *testing.T) { "code": "authcode", }) resp := MakeRequest(t, req, http.StatusBadRequest) - parsedError := new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + var parsedError auth.AccessTokenErrorResponse + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "failed PKCE code challenge", parsedError.ErrorDescription) } @@ -267,8 +267,8 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) resp := MakeRequest(t, req, http.StatusBadRequest) - parsedError := new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + var parsedError auth.AccessTokenErrorResponse + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "invalid_client", string(parsedError.ErrorCode)) assert.Equal(t, "cannot load client with client id: '???'", parsedError.ErrorDescription) @@ -282,8 +282,7 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "invalid client secret", parsedError.ErrorDescription) @@ -297,8 +296,7 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "unexpected redirect URI", parsedError.ErrorDescription) @@ -312,8 +310,7 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "client is not authorized", parsedError.ErrorDescription) @@ -327,8 +324,7 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unsupported_grant_type", string(parsedError.ErrorCode)) assert.Equal(t, "Only refresh_token or authorization_code grant type is supported", parsedError.ErrorDescription) } @@ -364,8 +360,8 @@ func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { }) req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OmJsYWJsYQ==") resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError := new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + var parsedError auth.AccessTokenErrorResponse + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "invalid client secret", parsedError.ErrorDescription) @@ -377,8 +373,7 @@ func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "invalid_client", string(parsedError.ErrorCode)) assert.Equal(t, "cannot load client with client id: ''", parsedError.ErrorDescription) @@ -391,8 +386,7 @@ func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { }) req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OjRNSzhOYTZSNTVzbWRDWTBXdUNDdW1aNmhqUlBuR1k1c2FXVlJISGpKaUE9") resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "invalid_request", string(parsedError.ErrorCode)) assert.Equal(t, "client_id in request body inconsistent with Authorization header", parsedError.ErrorDescription) @@ -405,8 +399,7 @@ func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { }) req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OjRNSzhOYTZSNTVzbWRDWTBXdUNDdW1aNmhqUlBuR1k1c2FXVlJISGpKaUE9") resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "invalid_request", string(parsedError.ErrorCode)) assert.Equal(t, "client_secret in request body inconsistent with Authorization header", parsedError.ErrorDescription) } @@ -443,8 +436,8 @@ func TestRefreshTokenInvalidation(t *testing.T) { "refresh_token": parsed.RefreshToken, }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError := new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + var parsedError auth.AccessTokenErrorResponse + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "invalid_client", string(parsedError.ErrorCode)) assert.Equal(t, "invalid empty client secret", parsedError.ErrorDescription) @@ -456,8 +449,7 @@ func TestRefreshTokenInvalidation(t *testing.T) { "refresh_token": "UNEXPECTED", }) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "unable to parse refresh token", parsedError.ErrorDescription) @@ -486,8 +478,7 @@ func TestRefreshTokenInvalidation(t *testing.T) { // repeat request should fail req.Body = io.NopCloser(bytes.NewReader(bs)) resp = MakeRequest(t, req, http.StatusBadRequest) - parsedError = new(auth.AccessTokenError) - require.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError)) + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &parsedError)) assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode)) assert.Equal(t, "token was already used", parsedError.ErrorDescription) }