Skip to content

Commit ca67c5a

Browse files
lunnyKN4CK3Rwolfogre
authored
refactor auth interface to return error when verify failure (#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 <admin@oldschoolhack.me> Co-authored-by: Jason Song <i@wolfogre.com>
1 parent 7cc7db7 commit ca67c5a

File tree

15 files changed

+111
-79
lines changed

15 files changed

+111
-79
lines changed

Diff for: ‎modules/context/api.go

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

Diff for: ‎modules/context/context.go

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

Diff for: ‎routers/api/packages/api.go

+15-2
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"code.gitea.io/gitea/models/perm"
1313
"code.gitea.io/gitea/modules/context"
14+
"code.gitea.io/gitea/modules/log"
1415
"code.gitea.io/gitea/modules/setting"
1516
"code.gitea.io/gitea/modules/web"
1617
"code.gitea.io/gitea/routers/api/packages/composer"
@@ -58,7 +59,13 @@ func CommonRoutes(ctx gocontext.Context) *web.Route {
5859

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

@@ -321,7 +328,13 @@ func ContainerRoutes(ctx gocontext.Context) *web.Route {
321328

322329
authGroup := auth.NewGroup(authMethods...)
323330
r.Use(func(ctx *context.Context) {
324-
ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
331+
var err error
332+
ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
333+
if err != nil {
334+
log.Error("Failed to verify user: %v", err)
335+
ctx.Error(http.StatusUnauthorized, "Verify")
336+
return
337+
}
325338
ctx.IsSigned = ctx.Doer != nil
326339
})
327340

Diff for: ‎routers/api/packages/conan/auth.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -19,22 +19,22 @@ func (a *Auth) Name() string {
1919
}
2020

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

2929
if uid == 0 {
30-
return nil
30+
return nil, nil
3131
}
3232

3333
u, err := user_model.GetUserByID(req.Context(), uid)
3434
if err != nil {
3535
log.Error("GetUserByID: %v", err)
36-
return nil
36+
return nil, err
3737
}
3838

39-
return u
39+
return u, nil
4040
}

Diff for: ‎routers/api/packages/container/auth.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -20,25 +20,25 @@ func (a *Auth) Name() string {
2020

2121
// Verify extracts the user from the Bearer token
2222
// If it's an anonymous session a ghost user is returned
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
if uid == -1 {
34-
return user_model.NewGhostUser()
34+
return user_model.NewGhostUser(), nil
3535
}
3636

3737
u, err := user_model.GetUserByID(req.Context(), uid)
3838
if err != nil {
3939
log.Error("GetUserByID: %v", err)
40-
return nil
40+
return nil, err
4141
}
4242

43-
return u
43+
return u, nil
4444
}

Diff for: ‎routers/api/packages/nuget/auth.go

+5-4
Original file line numberDiff line numberDiff line change
@@ -20,25 +20,26 @@ func (a *Auth) Name() string {
2020
}
2121

2222
// https://docs.microsoft.com/en-us/nuget/api/package-publish-resource#request-parameters
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
token, err := auth_model.GetAccessTokenBySHA(req.Header.Get("X-NuGet-ApiKey"))
2525
if err != nil {
2626
if !(auth_model.IsErrAccessTokenNotExist(err) || auth_model.IsErrAccessTokenEmpty(err)) {
2727
log.Error("GetAccessTokenBySHA: %v", err)
28+
return nil, err
2829
}
29-
return nil
30+
return nil, nil
3031
}
3132

3233
u, err := user_model.GetUserByID(req.Context(), token.UID)
3334
if err != nil {
3435
log.Error("GetUserByID: %v", err)
35-
return nil
36+
return nil, err
3637
}
3738

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

43-
return u
44+
return u, nil
4445
}

Diff for: ‎services/auth/basic.go

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

4949
baHead := req.Header.Get("Authorization")
5050
if len(baHead) == 0 {
51-
return nil
51+
return nil, nil
5252
}
5353

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

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

8383
store.GetData()["IsApiToken"] = true
84-
return u
84+
return u, nil
8585
}
8686

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

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

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

107107
if !setting.Service.EnableBasicAuth {
108-
return nil
108+
return nil, nil
109109
}
110110

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

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

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

126-
return u
126+
return u, nil
127127
}

Diff for: ‎services/auth/group.go

+8-9
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"reflect"
1010
"strings"
1111

12-
"code.gitea.io/gitea/models/db"
1312
user_model "code.gitea.io/gitea/models/user"
1413
)
1514

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

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

101-
return nil
100+
return nil, nil
102101
}

Diff for: ‎services/auth/httpsign.go

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

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

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

7575
u, err := user_model.GetUserByID(req.Context(), publicKey.OwnerID)
7676
if err != nil {
7777
log.Error("GetUserByID: %v", err)
78-
return nil
78+
return nil, err
7979
}
8080

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

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

85-
return u
85+
return u, nil
8686
}
8787

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

Diff for: ‎services/auth/interface.go

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

3132
// Initializable represents a structure that requires initialization

Diff for: ‎services/auth/oauth2.go

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

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

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

134130
log.Trace("OAuth2 Authorization: Logged in user %-v", user)
135-
return user
131+
return user, nil
136132
}
137133

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

0 commit comments

Comments
 (0)