-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor auth interface to return error when verify failure #22119
Changes from 11 commits
1b83c11
0376f23
cc0c65b
97411d4
4c98e7b
e99dd1e
69920f5
087cac4
9b2ba0b
bb26b14
44e2416
a66cdf8
25b0080
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,20 +40,20 @@ func (b *Basic) Name() string { | |
// "Authorization" header of the request and returns the corresponding user object for that | ||
// name/token on successful validation. | ||
// Returns nil if header is empty or validation fails. | ||
func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { | ||
func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { | ||
// Basic authentication should only fire on API, Download or on Git or LFSPaths | ||
if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) { | ||
return nil | ||
return nil, nil | ||
} | ||
|
||
baHead := req.Header.Get("Authorization") | ||
if len(baHead) == 0 { | ||
return nil | ||
return nil, nil | ||
} | ||
|
||
auths := strings.SplitN(baHead, " ", 2) | ||
if len(auths) != 2 || (strings.ToLower(auths[0]) != "basic") { | ||
return nil | ||
return nil, nil | ||
} | ||
|
||
uname, passwd, _ := base.BasicAuthDecode(auths[1]) | ||
|
@@ -77,11 +77,11 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore | |
u, err := user_model.GetUserByID(req.Context(), uid) | ||
if err != nil { | ||
log.Error("GetUserByID: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding all these logs: |
||
return nil | ||
return nil, err | ||
} | ||
|
||
store.GetData()["IsApiToken"] = true | ||
return u | ||
return u, nil | ||
} | ||
|
||
token, err := auth_model.GetAccessTokenBySHA(authToken) | ||
|
@@ -90,7 +90,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore | |
u, err := user_model.GetUserByID(req.Context(), token.UID) | ||
if err != nil { | ||
log.Error("GetUserByID: %v", err) | ||
return nil | ||
return nil, err | ||
} | ||
|
||
token.UpdatedUnix = timeutil.TimeStampNow() | ||
|
@@ -99,13 +99,13 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore | |
} | ||
|
||
store.GetData()["IsApiToken"] = true | ||
return u | ||
return u, nil | ||
} else if !auth_model.IsErrAccessTokenNotExist(err) && !auth_model.IsErrAccessTokenEmpty(err) { | ||
log.Error("GetAccessTokenBySha: %v", err) | ||
} | ||
|
||
if !setting.Service.EnableBasicAuth { | ||
return nil | ||
return nil, nil | ||
} | ||
|
||
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 | |
if !user_model.IsErrUserNotExist(err) { | ||
log.Error("UserSignIn: %v", err) | ||
} | ||
return nil | ||
return nil, err | ||
} | ||
|
||
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 | |
|
||
log.Trace("Basic Authorization: Logged in user %-v", u) | ||
|
||
return u | ||
return u, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ import ( | |
"reflect" | ||
"strings" | ||
|
||
"code.gitea.io/gitea/models/db" | ||
user_model "code.gitea.io/gitea/models/user" | ||
) | ||
|
||
|
@@ -80,23 +79,23 @@ 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 { | ||
if !db.HasEngine { | ||
return nil | ||
} | ||
Comment on lines
-84
to
-86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have the slight suspicion that this check was there for a reason. |
||
|
||
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 | ||
for _, ssoMethod := range b.methods { | ||
user := ssoMethod.Verify(req, w, store, sess) | ||
user, err := ssoMethod.Verify(req, w, store, sess) | ||
if err != nil { | ||
return user, err | ||
lunny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if user != nil { | ||
if store.GetData()["AuthedMethod"] == nil { | ||
if named, ok := ssoMethod.(Named); ok { | ||
store.GetData()["AuthedMethod"] = named.Name() | ||
} | ||
} | ||
return user | ||
return user, nil | ||
} | ||
} | ||
|
||
return nil | ||
return nil, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, we don't log API verification failures?
This line is missing in
APIAuth
…