From 53b4ad34e58a2db9bc68a53bb833177b47d11663 Mon Sep 17 00:00:00 2001 From: caicandong Date: Wed, 5 Jul 2023 19:23:07 +0800 Subject: [PATCH 01/10] Fix the error message when the token is incorrect (#24439) --- services/auth/oauth2.go | 50 +++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go index b70f84da9b639..3d39f1eeff9da 100644 --- a/services/auth/oauth2.go +++ b/services/auth/oauth2.go @@ -59,31 +59,32 @@ func (o *OAuth2) Name() string { return "oauth2" } -// 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(req *http.Request, store DataStore) int64 { +// parseToken returns the token from request, and a boolean value +// representing whether the token exists or not +func (o *OAuth2) parseToken(req *http.Request) (string, bool) { _ = req.ParseForm() - + // Check token. + if token := req.Form.Get("token"); token != "" { + return token, true + } // Check access token. - tokenSHA := req.Form.Get("token") - if len(tokenSHA) == 0 { - tokenSHA = req.Form.Get("access_token") - } - if len(tokenSHA) == 0 { - // Well, check with header again. - auHead := req.Header.Get("Authorization") - if len(auHead) > 0 { - auths := strings.Fields(auHead) - if len(auths) == 2 && (auths[0] == "token" || strings.ToLower(auths[0]) == "bearer") { - tokenSHA = auths[1] - } - } + if token := req.Form.Get("access_token"); token != "" { + return token, true } - if len(tokenSHA) == 0 { - return 0 + // check header token + if auHead := req.Header.Get("Authorization"); auHead != "" { + auths := strings.Fields(auHead) + if len(auths) == 2 && (auths[0] == "token" || strings.ToLower(auths[0]) == "bearer") { + return auths[1], true + } } + return "", false +} +// 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(tokenSHA string, store DataStore) int64 { // Let's see if token is valid. if strings.Contains(tokenSHA, ".") { uid := CheckOAuthAccessToken(tokenSHA) @@ -129,10 +130,15 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStor return nil, nil } - id := o.userIDFromToken(req, store) + token, ok := o.parseToken(req) + if !ok { + return nil, nil + } + + id := o.userIDFromToken(token, store) if id <= 0 && id != -2 { // -2 means actions, so we need to allow it. - return nil, nil + return nil, user_model.ErrUserNotExist{} } log.Trace("OAuth2 Authorization: Found token for user[%d]", id) From 93191f096fbbf20af9ee613e625cf3ce0f54c18a Mon Sep 17 00:00:00 2001 From: caicandong Date: Thu, 6 Jul 2023 11:54:37 +0800 Subject: [PATCH 02/10] refactor : remove the receiver of parseToken function --- services/auth/oauth2.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go index 3d39f1eeff9da..0dd7a12d2c436 100644 --- a/services/auth/oauth2.go +++ b/services/auth/oauth2.go @@ -61,7 +61,7 @@ func (o *OAuth2) Name() string { // parseToken returns the token from request, and a boolean value // representing whether the token exists or not -func (o *OAuth2) parseToken(req *http.Request) (string, bool) { +func parseToken(req *http.Request) (string, bool) { _ = req.ParseForm() // Check token. if token := req.Form.Get("token"); token != "" { @@ -130,7 +130,7 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStor return nil, nil } - token, ok := o.parseToken(req) + token, ok := parseToken(req) if !ok { return nil, nil } From 8f7c1eea3deeaceb61629ec03192aa6ac1e2af34 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 6 Jul 2023 16:06:53 +0800 Subject: [PATCH 03/10] feat: shortcut --- services/auth/shortcut.go | 52 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 services/auth/shortcut.go diff --git a/services/auth/shortcut.go b/services/auth/shortcut.go new file mode 100644 index 0000000000000..6192e6fec6c2e --- /dev/null +++ b/services/auth/shortcut.go @@ -0,0 +1,52 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package auth + +import ( + "net/http" + + user_model "code.gitea.io/gitea/models/user" +) + +// Shortcut is a group of verification methods that is like Group. +// It tries each method in order and returns the first non-nil user, +// but it never returns error even if some method returns error. +// It is useful for some methods that share the same protocol, shortcut can check them first. +// For example, OAuth2 and conan.Auth both read token from "Authorization: Bearer " header, +// If OAuth2 returns error, it is possible that the token is for conan.Auth but it has no chance to check. +// And Shortcut solves this problem by: +// +// NewGroup( +// Shortcut{&OAuth2, &conan.Auth}, +// &OAuth2, +// &auth.Basic{}, +// &nuget.Auth{}, +// &conan.Auth{}, +// &chef.Auth{}, +// ) +// +// Since Shortcut will set "AuthedMethod" in DataStore if any method returns non-nil user, +// so it is unnecessary to implement Named interface for it, the "name" of Shortcut should never be stored as "AuthedMethod". +type Shortcut []Method + +func (s Shortcut) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { + for _, method := range s { + user, err := method.Verify(req, w, store, sess) + if err != nil { + // Don't return error, just try next method + continue + } + + if user != nil { + if store.GetData()["AuthedMethod"] == nil { + if named, ok := method.(Named); ok { + store.GetData()["AuthedMethod"] = named.Name() + } + } + return user, nil + } + } + + return nil, nil +} From 9faf9ad58c6cd9a68f4f98883a144797e569dda2 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 6 Jul 2023 16:10:16 +0800 Subject: [PATCH 04/10] fix: /api/packages --- routers/api/packages/api.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index fa7f66f3ab38d..c77ad59cb12de 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -101,6 +101,12 @@ func CommonRoutes() *web.Route { r.Use(context.PackageContexter()) verifyAuth(r, []auth.Method{ + auth.Shortcut{ + // auth.OAuth2 and auth.Basic both read "Authorization: Bearer " header, + // so we need have a shortcut for them to avoid the first one returning error to skip the second one. + &auth.OAuth2{}, + &conan.Auth{}, + }, &auth.OAuth2{}, &auth.Basic{}, &nuget.Auth{}, From 5199ec0829ad3ee9dd83c0f431595bfb9fc0eaf8 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 6 Jul 2023 16:59:13 +0800 Subject: [PATCH 05/10] fix: typo --- services/auth/shortcut.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/services/auth/shortcut.go b/services/auth/shortcut.go index 6192e6fec6c2e..59277500a81b7 100644 --- a/services/auth/shortcut.go +++ b/services/auth/shortcut.go @@ -18,8 +18,11 @@ import ( // And Shortcut solves this problem by: // // NewGroup( -// Shortcut{&OAuth2, &conan.Auth}, -// &OAuth2, +// Shortcut{ +// &OAuth2{}, +// &conan.Auth{}, +// }, +// &OAuth2{}, // &auth.Basic{}, // &nuget.Auth{}, // &conan.Auth{}, From 0ae0de52864bed27eb1bb35403df3c620d8dfc2c Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 6 Jul 2023 20:00:53 +0800 Subject: [PATCH 06/10] Revert "feat: shortcut" Revert "fix: typo" This reverts commit 5199ec0829ad3ee9dd83c0f431595bfb9fc0eaf8. Revert "fix: /api/packages" This reverts commit 9faf9ad58c6cd9a68f4f98883a144797e569dda2. Revert "feat: shortcut" This reverts commit 8f7c1eea3deeaceb61629ec03192aa6ac1e2af34. --- routers/api/packages/api.go | 6 ---- services/auth/shortcut.go | 55 ------------------------------------- 2 files changed, 61 deletions(-) delete mode 100644 services/auth/shortcut.go diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index c77ad59cb12de..fa7f66f3ab38d 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -101,12 +101,6 @@ func CommonRoutes() *web.Route { r.Use(context.PackageContexter()) verifyAuth(r, []auth.Method{ - auth.Shortcut{ - // auth.OAuth2 and auth.Basic both read "Authorization: Bearer " header, - // so we need have a shortcut for them to avoid the first one returning error to skip the second one. - &auth.OAuth2{}, - &conan.Auth{}, - }, &auth.OAuth2{}, &auth.Basic{}, &nuget.Auth{}, diff --git a/services/auth/shortcut.go b/services/auth/shortcut.go deleted file mode 100644 index 59277500a81b7..0000000000000 --- a/services/auth/shortcut.go +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright 2023 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package auth - -import ( - "net/http" - - user_model "code.gitea.io/gitea/models/user" -) - -// Shortcut is a group of verification methods that is like Group. -// It tries each method in order and returns the first non-nil user, -// but it never returns error even if some method returns error. -// It is useful for some methods that share the same protocol, shortcut can check them first. -// For example, OAuth2 and conan.Auth both read token from "Authorization: Bearer " header, -// If OAuth2 returns error, it is possible that the token is for conan.Auth but it has no chance to check. -// And Shortcut solves this problem by: -// -// NewGroup( -// Shortcut{ -// &OAuth2{}, -// &conan.Auth{}, -// }, -// &OAuth2{}, -// &auth.Basic{}, -// &nuget.Auth{}, -// &conan.Auth{}, -// &chef.Auth{}, -// ) -// -// Since Shortcut will set "AuthedMethod" in DataStore if any method returns non-nil user, -// so it is unnecessary to implement Named interface for it, the "name" of Shortcut should never be stored as "AuthedMethod". -type Shortcut []Method - -func (s Shortcut) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { - for _, method := range s { - user, err := method.Verify(req, w, store, sess) - if err != nil { - // Don't return error, just try next method - continue - } - - if user != nil { - if store.GetData()["AuthedMethod"] == nil { - if named, ok := method.(Named); ok { - store.GetData()["AuthedMethod"] = named.Name() - } - } - return user, nil - } - } - - return nil, nil -} From a3cd2183d4b1ce6736bdde9bcdb755742374f6ef Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 6 Jul 2023 20:08:18 +0800 Subject: [PATCH 07/10] fix: try more metheds --- services/auth/group.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/services/auth/group.go b/services/auth/group.go index a1ff65f20385e..7193dfcf49e2f 100644 --- a/services/auth/group.go +++ b/services/auth/group.go @@ -49,12 +49,22 @@ func (b *Group) Name() string { // Verify extracts and validates func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { // Try to sign in with each of the enabled plugins + var retErr error for _, ssoMethod := range b.methods { user, err := ssoMethod.Verify(req, w, store, sess) if err != nil { - return nil, err + if retErr == nil { + retErr = err + } + // Try other methods if this one failed. + // Some methods may share the same protocol to detect if they are matched. + // For example, OAuth2 and conan.Auth both read token from "Authorization: Bearer " header, + // If OAuth2 returns error, we should give conan.Auth a chance to try. + continue } + // If any method returns a user, we can stop trying. + // Return the user and ignore any error returned by previous methods. if user != nil { if store.GetData()["AuthedMethod"] == nil { if named, ok := ssoMethod.(Named); ok { @@ -65,5 +75,6 @@ func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore } } - return nil, nil + // If no method returns a user, return the error returned by the first method. + return nil, retErr } From 331b29c4f827e38d4b80919aa7c5cd97039a0934 Mon Sep 17 00:00:00 2001 From: caicandong Date: Mon, 10 Jul 2023 17:14:10 +0800 Subject: [PATCH 08/10] Add a test case for issue # 24439 --- tests/integration/api_repo_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 0f387192eb09c..bf930375b3bc6 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -41,6 +41,17 @@ func TestAPIUserReposNotLogin(t *testing.T) { } } +func TestAPIUserReposWithWrongToken(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + wrong_token := fmt.Sprintf("Bearer %s", "wrong_token") + req := NewRequestf(t, "GET", "/api/v1/users/%s/repos", user.Name) + req = addTokenAuthHeader(req, wrong_token) + resp := MakeRequest(t, req, http.StatusOK) + + assert.Contains(t, resp.Body.String(), "user does not exist") +} + func TestAPISearchRepo(t *testing.T) { defer tests.PrepareTestEnv(t)() const keyword = "test" From dfe78411590c363e908b1ab654e63155a04f5fd3 Mon Sep 17 00:00:00 2001 From: caicandong Date: Mon, 10 Jul 2023 17:19:21 +0800 Subject: [PATCH 09/10] fix : http status should be StatusUnauthorized --- tests/integration/api_repo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index bf930375b3bc6..53b3e224bde78 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -47,7 +47,7 @@ func TestAPIUserReposWithWrongToken(t *testing.T) { wrong_token := fmt.Sprintf("Bearer %s", "wrong_token") req := NewRequestf(t, "GET", "/api/v1/users/%s/repos", user.Name) req = addTokenAuthHeader(req, wrong_token) - resp := MakeRequest(t, req, http.StatusOK) + resp := MakeRequest(t, req, http.StatusUnauthorized) assert.Contains(t, resp.Body.String(), "user does not exist") } From 65e84363db965761037aeae7e9e5d6a973ed1311 Mon Sep 17 00:00:00 2001 From: CaiCandong <1290147055@qq.com> Date: Mon, 10 Jul 2023 20:43:46 +0800 Subject: [PATCH 10/10] refactor wrong_token to wrongToken --- tests/integration/api_repo_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 53b3e224bde78..fae1415568692 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -44,9 +44,9 @@ func TestAPIUserReposNotLogin(t *testing.T) { func TestAPIUserReposWithWrongToken(t *testing.T) { defer tests.PrepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - wrong_token := fmt.Sprintf("Bearer %s", "wrong_token") + wrongToken := fmt.Sprintf("Bearer %s", "wrong_token") req := NewRequestf(t, "GET", "/api/v1/users/%s/repos", user.Name) - req = addTokenAuthHeader(req, wrong_token) + req = addTokenAuthHeader(req, wrongToken) resp := MakeRequest(t, req, http.StatusUnauthorized) assert.Contains(t, resp.Body.String(), "user does not exist")