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>
This commit is contained in:
Giteabot 2023-07-12 05:29:30 -04:00 committed by GitHub
parent bd1946e372
commit ee87b4e18c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 54 additions and 26 deletions

View File

@ -81,12 +81,22 @@ func (b *Group) Free() error {
// Verify extracts and validates // Verify extracts and validates
func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { 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 // Try to sign in with each of the enabled plugins
var retErr error
for _, ssoMethod := range b.methods { for _, ssoMethod := range b.methods {
user, err := ssoMethod.Verify(req, w, store, sess) user, err := ssoMethod.Verify(req, w, store, sess)
if err != nil { 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 user != nil {
if store.GetData()["AuthedMethod"] == nil { if store.GetData()["AuthedMethod"] == nil {
if named, ok := ssoMethod.(Named); ok { 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
} }

View File

@ -59,31 +59,32 @@ func (o *OAuth2) Name() string {
return "oauth2" 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. // userIDFromToken returns the user id corresponding to the OAuth token.
// It will set 'IsApiToken' to true if the token is an API token and // It will set 'IsApiToken' to true if the token is an API token and
// set 'ApiTokenScope' to the scope of the access token // set 'ApiTokenScope' to the scope of the access token
func (o *OAuth2) userIDFromToken(req *http.Request, store DataStore) int64 { func (o *OAuth2) userIDFromToken(tokenSHA string, 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
}
// Let's see if token is valid. // Let's see if token is valid.
if strings.Contains(tokenSHA, ".") { if strings.Contains(tokenSHA, ".") {
uid := CheckOAuthAccessToken(tokenSHA) uid := CheckOAuthAccessToken(tokenSHA)
@ -129,10 +130,15 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStor
return nil, nil 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. 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) log.Trace("OAuth2 Authorization: Found token for user[%d]", id)

View File

@ -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) { func TestAPISearchRepo(t *testing.T) {
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
const keyword = "test" const keyword = "test"