From 491cc06ffe3491242ad9ff6227423d99e673d0c2 Mon Sep 17 00:00:00 2001 From: caicandong <50507092+CaiCandong@users.noreply.github.com> Date: Tue, 11 Jul 2023 10:04:28 +0800 Subject: [PATCH] Fix the error message when the token is incorrect (#25701) 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: Jason Song Co-authored-by: Giteabot --- services/auth/group.go | 15 +++++++-- services/auth/oauth2.go | 54 +++++++++++++++++------------- tests/integration/api_repo_test.go | 11 ++++++ 3 files changed, 54 insertions(+), 26 deletions(-) diff --git a/services/auth/group.go b/services/auth/group.go index a1ff65f20..7193dfcf4 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 } diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go index b70f84da9..0dd7a12d2 100644 --- a/services/auth/oauth2.go +++ b/services/auth/oauth2.go @@ -59,31 +59,32 @@ func (o *OAuth2) Name() string { return "oauth2" } +// 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. + if token := req.Form.Get("access_token"); token != "" { + return token, true + } + // 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(req *http.Request, store DataStore) int64 { - _ = req.ParseForm() - - // 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 len(tokenSHA) == 0 { - return 0 - } - +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 0f387192e..fae141556 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}) + 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"