diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go index ee9f64c91f..eb1dffa32a 100644 --- a/services/auth/oauth2.go +++ b/services/auth/oauth2.go @@ -147,7 +147,10 @@ func parseToken(req *http.Request) (string, bool) { // userIDFromToken returns the user id corresponding to the OAuth token. // It will set 'IsApiToken' to true if the token is an API token and // set 'ApiTokenScope' to the scope of the access token -func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string, store DataStore) int64 { +func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string, store DataStore) (int64, error) { + if tokenSHA == "" { + return 0, auth_model.ErrAccessTokenEmpty{} + } // Let's see if token is valid. if strings.Contains(tokenSHA, ".") { // First attempt to decode an actions JWT, returning the actions user @@ -155,7 +158,7 @@ func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string, store Dat if CheckTaskIsRunning(ctx, taskID) { store.GetData()["IsActionsToken"] = true store.GetData()["ActionsTaskID"] = taskID - return user_model.ActionsUserID + return user_model.ActionsUserID, nil } } @@ -169,7 +172,7 @@ func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string, store Dat store.GetData()["ApiTokenScope"] = auth_model.AccessTokenScopeAll // fallback to all } } - return uid + return uid, nil } t, err := auth_model.GetAccessTokenBySHA(ctx, tokenSHA) if err != nil { @@ -182,19 +185,22 @@ func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string, store Dat store.GetData()["IsActionsToken"] = true store.GetData()["ActionsTaskID"] = task.ID - return user_model.ActionsUserID + return user_model.ActionsUserID, nil } } else if !auth_model.IsErrAccessTokenNotExist(err) && !auth_model.IsErrAccessTokenEmpty(err) { log.Error("GetAccessTokenBySHA: %v", err) } - return 0 + return 0, err } if err := t.UpdateLastUsed(ctx); err != nil { log.Error("UpdateLastUsed: %v", err) } + if t.UID == 0 { + return 0, auth_model.ErrAccessTokenNotExist{} + } store.GetData()["IsApiToken"] = true store.GetData()["ApiTokenScope"] = t.Scope - return t.UID + return t.UID, nil } // Verify extracts the user ID from the OAuth token in the query parameters @@ -213,10 +219,9 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStor return nil, nil } - id := o.userIDFromToken(req.Context(), token, store) - - if id <= 0 && id != -2 { // -2 means actions, so we need to allow it. - return nil, user_model.ErrUserNotExist{} + id, err := o.userIDFromToken(req.Context(), token, store) + if err != nil { + return nil, err } log.Trace("OAuth2 Authorization: Found token for user[%d]", id) diff --git a/services/auth/oauth2_test.go b/services/auth/oauth2_test.go index 3fce7df50b..a8fe9961fa 100644 --- a/services/auth/oauth2_test.go +++ b/services/auth/oauth2_test.go @@ -7,6 +7,7 @@ import ( "net/http" "testing" + "forgejo.org/models/auth" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" "forgejo.org/modules/web/middleware" @@ -27,11 +28,32 @@ func TestUserIDFromToken(t *testing.T) { ds := make(middleware.ContextData) o := OAuth2{} - uid := o.userIDFromToken(t.Context(), token, ds) + uid, err := o.userIDFromToken(t.Context(), token, ds) + require.NoError(t, err) assert.Equal(t, int64(user_model.ActionsUserID), uid) assert.Equal(t, true, ds["IsActionsToken"]) assert.Equal(t, ds["ActionsTaskID"], int64(RunningTaskID)) }) + + t.Run("Actions error-JWT", func(t *testing.T) { + cases := map[string]struct { + Token string + Error error + }{ + "Empty": {"", auth.ErrAccessTokenEmpty{}}, + "To short": {"abc", auth.ErrAccessTokenNotExist{Token: "abc"}}, + } + + ds := make(middleware.ContextData) + o := OAuth2{} + for name, c := range cases { + t.Run(name, func(t *testing.T) { + uid, err := o.userIDFromToken(t.Context(), c.Token, ds) + require.ErrorIs(t, err, c.Error) + assert.Equal(t, int64(0), uid) + }) + } + }) } func TestCheckTaskIsRunning(t *testing.T) { @@ -60,13 +82,14 @@ func TestParseToken(t *testing.T) { ExpectedToken string Expected bool }{ - "Token Uppercase": {Header: "Token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Token Lowercase": {Header: "token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Token Unicode": {Header: "to\u212Aen 1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, - "Bearer Uppercase": {Header: "Bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Bearer Lowercase": {Header: "bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Missing type": {Header: "1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, - "Three Parts": {Header: "abc 1234567890 test", ExpectedToken: "", Expected: false}, + "Token Uppercase": {Header: "Token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Token Lowercase": {Header: "token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Token Unicode": {Header: "to\u212Aen 1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, + "Bearer Uppercase": {Header: "Bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Bearer Lowercase": {Header: "bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Missing type": {Header: "1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, + "Three Parts": {Header: "abc 1234567890 test", ExpectedToken: "", Expected: false}, + "Token Three Parts": {Header: "Token 1234567890 test", ExpectedToken: "", Expected: false}, } for name := range cases { diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index edd0c61a44..3dd79c8ebe 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -52,7 +52,7 @@ func TestAPIUserReposWithWrongToken(t *testing.T) { AddTokenAuth(wrongToken) resp := MakeRequest(t, req, http.StatusUnauthorized) - assert.Contains(t, resp.Body.String(), "user does not exist") + assert.Contains(t, resp.Body.String(), "access token does not exist") } func TestAPISearchRepo(t *testing.T) {