Fix the error message when the token is incorrect (#25701) (#25834)

Backport #25701 by @CaiCandong

we refactored `userIDFromToken` for the token parsing part into a new
function `parseToken`. `parseToken` returns the string `token` from
request, and a boolean `ok` representing whether the token exists or
not. So we can distinguish between token non-existence and token
inconsistency in the `verfity` function, thus solving the problem of no
proper error message when the token is inconsistent.
close #24439
related #22119

Co-authored-by: caicandong <50507092+CaiCandong@users.noreply.github.com>
Co-authored-by: Jason Song <i@wolfogre.com>

diff --git a/services/auth/group.go b/services/auth/group.go
index 0a0330b3aa95eccea29aabb8e176eae87b5d611d..e60af273535af170b80612afd47d8b30eed60df1 100644 (file)
--- a/services/auth/group.go
+++ b/services/auth/group.go
@@ -81,12 +81,22 @@ func (b *Group) Free() error {
 // 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 <token>" 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 {
@@ -97,5 +107,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
 }

diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go
index b70f84da9b639e07abe46c02567992301adb21ab..0dd7a12d2c4369b43940c5ff9df2e47bb9e3ec42 100644 (file)
--- 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 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 := 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)
 

diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go
index b3a184edddae7b0b1ca24aa1627f1cfa0d2e4938..bb676a85a6436d2104bb62b6da5cd0e4d6b8e356 100644 (file)
--- 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})
+       wrongToken := fmt.Sprintf("Bearer %s", "wrong_token")
+       req := NewRequestf(t, "GET", "/api/v1/users/%s/repos", user.Name)
+       req = addTokenAuthHeader(req, wrongToken)
+       resp := MakeRequest(t, req, http.StatusUnauthorized)
+
+       assert.Contains(t, resp.Body.String(), "user does not exist")
+}
+
 func TestAPISearchRepo(t *testing.T) {
        defer tests.PrepareTestEnv(t)()
        const keyword = "test"