Skip to content

Commit 900e158

Browse files
lunnyKN4CK3Rwolfogre
authored
refactor auth interface to return error when verify failure (#22119) (#22259)
backport #22119 This PR changed the Auth interface signature from `Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User` to `Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error)`. There is a new return argument `error` which means the verification condition matched but verify process failed, we should stop the auth process. Before this PR, when return a `nil` user, we don't know the reason why it returned `nil`. If the match condition is not satisfied or it verified failure? For these two different results, we should have different handler. If the match condition is not satisfied, we should try next auth method and if there is no more auth method, it's an anonymous user. If the condition matched but verify failed, the auth process should be stop and return immediately. This will fix #20563 Co-authored-by: KN4CK3R <[email protected]> Co-authored-by: Jason Song <[email protected]>
1 parent e9bc2c7 commit 900e158

File tree

15 files changed

+111
-79
lines changed

15 files changed

+111
-79
lines changed

‎modules/context/api.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,13 @@ func (ctx *APIContext) CheckForOTP() {
220220
func APIAuth(authMethod auth_service.Method) func(*APIContext) {
221221
return func(ctx *APIContext) {
222222
// Get user from session if logged in.
223-
ctx.Doer = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
223+
var err error
224+
ctx.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
225+
if err != nil {
226+
ctx.Error(http.StatusUnauthorized, "APIAuth", err)
227+
return
228+
}
229+
224230
if ctx.Doer != nil {
225231
if ctx.Locale.Language() != ctx.Doer.Language {
226232
ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req)

‎modules/context/context.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -663,7 +663,13 @@ func getCsrfOpts() CsrfOptions {
663663
// Auth converts auth.Auth as a middleware
664664
func Auth(authMethod auth.Method) func(*Context) {
665665
return func(ctx *Context) {
666-
ctx.Doer = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
666+
var err error
667+
ctx.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
668+
if err != nil {
669+
log.Error("Failed to verify user %v: %v", ctx.Req.RemoteAddr, err)
670+
ctx.Error(http.StatusUnauthorized, "Verify")
671+
return
672+
}
667673
if ctx.Doer != nil {
668674
if ctx.Locale.Language() != ctx.Doer.Language {
669675
ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req)

‎routers/api/packages/api.go

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212

1313
"code.gitea.io/gitea/models/perm"
1414
"code.gitea.io/gitea/modules/context"
15+
"code.gitea.io/gitea/modules/log"
1516
"code.gitea.io/gitea/modules/setting"
1617
"code.gitea.io/gitea/modules/web"
1718
"code.gitea.io/gitea/routers/api/packages/composer"
@@ -57,7 +58,13 @@ func Routes(ctx gocontext.Context) *web.Route {
5758

5859
authGroup := auth.NewGroup(authMethods...)
5960
r.Use(func(ctx *context.Context) {
60-
ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
61+
var err error
62+
ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
63+
if err != nil {
64+
log.Error("Verify: %v", err)
65+
ctx.Error(http.StatusUnauthorized, "authGroup.Verify")
66+
return
67+
}
6168
ctx.IsSigned = ctx.Doer != nil
6269
})
6370

@@ -317,7 +324,13 @@ func ContainerRoutes(ctx gocontext.Context) *web.Route {
317324

318325
authGroup := auth.NewGroup(authMethods...)
319326
r.Use(func(ctx *context.Context) {
320-
ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
327+
var err error
328+
ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
329+
if err != nil {
330+
log.Error("Failed to verify user: %v", err)
331+
ctx.Error(http.StatusUnauthorized, "Verify")
332+
return
333+
}
321334
ctx.IsSigned = ctx.Doer != nil
322335
})
323336

‎routers/api/packages/conan/auth.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,22 +20,22 @@ func (a *Auth) Name() string {
2020
}
2121

2222
// Verify extracts the user from the Bearer token
23-
func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) *user_model.User {
23+
func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) {
2424
uid, err := packages.ParseAuthorizationToken(req)
2525
if err != nil {
2626
log.Trace("ParseAuthorizationToken: %v", err)
27-
return nil
27+
return nil, err
2828
}
2929

3030
if uid == 0 {
31-
return nil
31+
return nil, nil
3232
}
3333

3434
u, err := user_model.GetUserByID(uid)
3535
if err != nil {
3636
log.Error("GetUserByID: %v", err)
37-
return nil
37+
return nil, err
3838
}
3939

40-
return u
40+
return u, nil
4141
}

‎routers/api/packages/container/auth.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,25 +21,25 @@ func (a *Auth) Name() string {
2121

2222
// Verify extracts the user from the Bearer token
2323
// If it's an anonymous session a ghost user is returned
24-
func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) *user_model.User {
24+
func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) {
2525
uid, err := packages.ParseAuthorizationToken(req)
2626
if err != nil {
2727
log.Trace("ParseAuthorizationToken: %v", err)
28-
return nil
28+
return nil, err
2929
}
3030

3131
if uid == 0 {
32-
return nil
32+
return nil, nil
3333
}
3434
if uid == -1 {
35-
return user_model.NewGhostUser()
35+
return user_model.NewGhostUser(), nil
3636
}
3737

3838
u, err := user_model.GetUserByID(uid)
3939
if err != nil {
4040
log.Error("GetUserByID: %v", err)
41-
return nil
41+
return nil, err
4242
}
4343

44-
return u
44+
return u, nil
4545
}

‎routers/api/packages/nuget/auth.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,25 +21,26 @@ func (a *Auth) Name() string {
2121
}
2222

2323
// https://docs.microsoft.com/en-us/nuget/api/package-publish-resource#request-parameters
24-
func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) *user_model.User {
24+
func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) {
2525
token, err := auth_model.GetAccessTokenBySHA(req.Header.Get("X-NuGet-ApiKey"))
2626
if err != nil {
2727
if !(auth_model.IsErrAccessTokenNotExist(err) || auth_model.IsErrAccessTokenEmpty(err)) {
2828
log.Error("GetAccessTokenBySHA: %v", err)
29+
return nil, err
2930
}
30-
return nil
31+
return nil, nil
3132
}
3233

3334
u, err := user_model.GetUserByID(token.UID)
3435
if err != nil {
3536
log.Error("GetUserByID: %v", err)
36-
return nil
37+
return nil, err
3738
}
3839

3940
token.UpdatedUnix = timeutil.TimeStampNow()
4041
if err := auth_model.UpdateAccessToken(token); err != nil {
4142
log.Error("UpdateAccessToken: %v", err)
4243
}
4344

44-
return u
45+
return u, nil
4546
}

‎services/auth/basic.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,20 +41,20 @@ func (b *Basic) Name() string {
4141
// "Authorization" header of the request and returns the corresponding user object for that
4242
// name/token on successful validation.
4343
// Returns nil if header is empty or validation fails.
44-
func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {
44+
func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
4545
// Basic authentication should only fire on API, Download or on Git or LFSPaths
4646
if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) {
47-
return nil
47+
return nil, nil
4848
}
4949

5050
baHead := req.Header.Get("Authorization")
5151
if len(baHead) == 0 {
52-
return nil
52+
return nil, nil
5353
}
5454

5555
auths := strings.SplitN(baHead, " ", 2)
5656
if len(auths) != 2 || (strings.ToLower(auths[0]) != "basic") {
57-
return nil
57+
return nil, nil
5858
}
5959

6060
uname, passwd, _ := base.BasicAuthDecode(auths[1])
@@ -78,11 +78,11 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
7878
u, err := user_model.GetUserByID(uid)
7979
if err != nil {
8080
log.Error("GetUserByID: %v", err)
81-
return nil
81+
return nil, err
8282
}
8383

8484
store.GetData()["IsApiToken"] = true
85-
return u
85+
return u, nil
8686
}
8787

8888
token, err := auth_model.GetAccessTokenBySHA(authToken)
@@ -91,7 +91,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
9191
u, err := user_model.GetUserByID(token.UID)
9292
if err != nil {
9393
log.Error("GetUserByID: %v", err)
94-
return nil
94+
return nil, err
9595
}
9696

9797
token.UpdatedUnix = timeutil.TimeStampNow()
@@ -100,13 +100,13 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
100100
}
101101

102102
store.GetData()["IsApiToken"] = true
103-
return u
103+
return u, nil
104104
} else if !auth_model.IsErrAccessTokenNotExist(err) && !auth_model.IsErrAccessTokenEmpty(err) {
105105
log.Error("GetAccessTokenBySha: %v", err)
106106
}
107107

108108
if !setting.Service.EnableBasicAuth {
109-
return nil
109+
return nil, nil
110110
}
111111

112112
log.Trace("Basic Authorization: Attempting SignIn for %s", uname)
@@ -115,7 +115,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
115115
if !user_model.IsErrUserNotExist(err) {
116116
log.Error("UserSignIn: %v", err)
117117
}
118-
return nil
118+
return nil, err
119119
}
120120

121121
if skipper, ok := source.Cfg.(LocalTwoFASkipper); ok && skipper.IsSkipLocalTwoFA() {
@@ -124,5 +124,5 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
124124

125125
log.Trace("Basic Authorization: Logged in user %-v", u)
126126

127-
return u
127+
return u, nil
128128
}

‎services/auth/group.go

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"reflect"
1111
"strings"
1212

13-
"code.gitea.io/gitea/models/db"
1413
user_model "code.gitea.io/gitea/models/user"
1514
)
1615

@@ -81,23 +80,23 @@ func (b *Group) Free() error {
8180
}
8281

8382
// Verify extracts and validates
84-
func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {
85-
if !db.HasEngine {
86-
return nil
87-
}
88-
83+
func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
8984
// Try to sign in with each of the enabled plugins
9085
for _, ssoMethod := range b.methods {
91-
user := ssoMethod.Verify(req, w, store, sess)
86+
user, err := ssoMethod.Verify(req, w, store, sess)
87+
if err != nil {
88+
return nil, err
89+
}
90+
9291
if user != nil {
9392
if store.GetData()["AuthedMethod"] == nil {
9493
if named, ok := ssoMethod.(Named); ok {
9594
store.GetData()["AuthedMethod"] = named.Name()
9695
}
9796
}
98-
return user
97+
return user, nil
9998
}
10099
}
101100

102-
return nil
101+
return nil, nil
103102
}

‎services/auth/httpsign.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,10 @@ func (h *HTTPSign) Name() string {
4040
// Verify extracts and validates HTTPsign from the Signature header of the request and returns
4141
// the corresponding user object on successful validation.
4242
// Returns nil if header is empty or validation fails.
43-
func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {
43+
func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
4444
sigHead := req.Header.Get("Signature")
4545
if len(sigHead) == 0 {
46-
return nil
46+
return nil, nil
4747
}
4848

4949
var (
@@ -54,36 +54,36 @@ func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataSt
5454
if len(req.Header.Get("X-Ssh-Certificate")) != 0 {
5555
// Handle Signature signed by SSH certificates
5656
if len(setting.SSH.TrustedUserCAKeys) == 0 {
57-
return nil
57+
return nil, nil
5858
}
5959

6060
publicKey, err = VerifyCert(req)
6161
if err != nil {
6262
log.Debug("VerifyCert on request from %s: failed: %v", req.RemoteAddr, err)
6363
log.Warn("Failed authentication attempt from %s", req.RemoteAddr)
64-
return nil
64+
return nil, nil
6565
}
6666
} else {
6767
// Handle Signature signed by Public Key
6868
publicKey, err = VerifyPubKey(req)
6969
if err != nil {
7070
log.Debug("VerifyPubKey on request from %s: failed: %v", req.RemoteAddr, err)
7171
log.Warn("Failed authentication attempt from %s", req.RemoteAddr)
72-
return nil
72+
return nil, nil
7373
}
7474
}
7575

7676
u, err := user_model.GetUserByID(publicKey.OwnerID)
7777
if err != nil {
7878
log.Error("GetUserByID: %v", err)
79-
return nil
79+
return nil, err
8080
}
8181

8282
store.GetData()["IsApiToken"] = true
8383

8484
log.Trace("HTTP Sign: Logged in user %-v", u)
8585

86-
return u
86+
return u, nil
8787
}
8888

8989
func VerifyPubKey(r *http.Request) (*asymkey_model.PublicKey, error) {

‎services/auth/interface.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,9 @@ type Method interface {
2525
// If verification is successful returns either an existing user object (with id > 0)
2626
// or a new user object (with id = 0) populated with the information that was found
2727
// in the authentication data (username or email).
28-
// Returns nil if verification fails.
29-
Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User
28+
// Second argument returns err if verification fails, otherwise
29+
// First return argument returns nil if no matched verification condition
30+
Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error)
3031
}
3132

3233
// Initializable represents a structure that requires initialization

‎services/auth/oauth2.go

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -109,18 +109,14 @@ func (o *OAuth2) userIDFromToken(req *http.Request, store DataStore) int64 {
109109
// or the "Authorization" header and returns the corresponding user object for that ID.
110110
// If verification is successful returns an existing user object.
111111
// Returns nil if verification fails.
112-
func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {
113-
if !db.HasEngine {
114-
return nil
115-
}
116-
112+
func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
117113
if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) {
118-
return nil
114+
return nil, nil
119115
}
120116

121117
id := o.userIDFromToken(req, store)
122118
if id <= 0 {
123-
return nil
119+
return nil, nil
124120
}
125121
log.Trace("OAuth2 Authorization: Found token for user[%d]", id)
126122

@@ -129,11 +125,11 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStor
129125
if !user_model.IsErrUserNotExist(err) {
130126
log.Error("GetUserByName: %v", err)
131127
}
132-
return nil
128+
return nil, err
133129
}
134130

135131
log.Trace("OAuth2 Authorization: Logged in user %-v", user)
136-
return user
132+
return user, nil
137133
}
138134

139135
func isAuthenticatedTokenRequest(req *http.Request) bool {

0 commit comments

Comments
 (0)