Fix the error message when the token is incorrect (go-gitea#25701) (g…

…o-gitea#25836)

Backport go-gitea#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 go-gitea#24439  
related go-gitea#22119

Co-authored-by: caicandong <[email protected]>
Co-authored-by: Jason Song <[email protected]>

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
 }

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)
 

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"