Skip to content

Commit

Permalink
refactor auth interface to return error when verify failure (go-gitea…
Browse files Browse the repository at this point in the history
…#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 go-gitea#20563

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: Jason Song <i@wolfogre.com>
  • Loading branch information
3 people committed Dec 28, 2022
1 parent 5583eaa commit f4edc75
Show file tree
Hide file tree
Showing 15 changed files with 111 additions and 79 deletions.
8 changes: 7 additions & 1 deletion modules/context/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,13 @@ func (ctx *APIContext) CheckForOTP() {
func APIAuth(authMethod auth_service.Method) func(*APIContext) {
return func(ctx *APIContext) {
// Get user from session if logged in.
ctx.Doer = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
var err error
ctx.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
if err != nil {
ctx.Error(http.StatusUnauthorized, "APIAuth", err)
return
}

if ctx.Doer != nil {
if ctx.Locale.Language() != ctx.Doer.Language {
ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req)
Expand Down
8 changes: 7 additions & 1 deletion modules/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,13 @@ func getCsrfOpts() CsrfOptions {
// Auth converts auth.Auth as a middleware
func Auth(authMethod auth.Method) func(*Context) {
return func(ctx *Context) {
ctx.Doer = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
var err error
ctx.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
if err != nil {
log.Error("Failed to verify user %v: %v", ctx.Req.RemoteAddr, err)
ctx.Error(http.StatusUnauthorized, "Verify")
return
}
if ctx.Doer != nil {
if ctx.Locale.Language() != ctx.Doer.Language {
ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req)
Expand Down
17 changes: 15 additions & 2 deletions routers/api/packages/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"code.gitea.io/gitea/models/perm"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/routers/api/packages/composer"
Expand Down Expand Up @@ -57,7 +58,13 @@ func Routes(ctx gocontext.Context) *web.Route {

authGroup := auth.NewGroup(authMethods...)
r.Use(func(ctx *context.Context) {
ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
var err error
ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
if err != nil {
log.Error("Verify: %v", err)
ctx.Error(http.StatusUnauthorized, "authGroup.Verify")
return
}
ctx.IsSigned = ctx.Doer != nil
})

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

authGroup := auth.NewGroup(authMethods...)
r.Use(func(ctx *context.Context) {
ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
var err error
ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session)
if err != nil {
log.Error("Failed to verify user: %v", err)
ctx.Error(http.StatusUnauthorized, "Verify")
return
}
ctx.IsSigned = ctx.Doer != nil
})

Expand Down
10 changes: 5 additions & 5 deletions routers/api/packages/conan/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,22 @@ func (a *Auth) Name() string {
}

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

if uid == 0 {
return nil
return nil, nil
}

u, err := user_model.GetUserByID(uid)
if err != nil {
log.Error("GetUserByID: %v", err)
return nil
return nil, err
}

return u
return u, nil
}
12 changes: 6 additions & 6 deletions routers/api/packages/container/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,25 @@ func (a *Auth) Name() string {

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

if uid == 0 {
return nil
return nil, nil
}
if uid == -1 {
return user_model.NewGhostUser()
return user_model.NewGhostUser(), nil
}

u, err := user_model.GetUserByID(uid)
if err != nil {
log.Error("GetUserByID: %v", err)
return nil
return nil, err
}

return u
return u, nil
}
9 changes: 5 additions & 4 deletions routers/api/packages/nuget/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,26 @@ func (a *Auth) Name() string {
}

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

u, err := user_model.GetUserByID(token.UID)
if err != nil {
log.Error("GetUserByID: %v", err)
return nil
return nil, err
}

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

return u
return u, nil
}
22 changes: 11 additions & 11 deletions services/auth/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,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])
Expand All @@ -78,11 +78,11 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
u, err := user_model.GetUserByID(uid)
if err != nil {
log.Error("GetUserByID: %v", err)
return nil
return nil, err
}

store.GetData()["IsApiToken"] = true
return u
return u, nil
}

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

token.UpdatedUnix = timeutil.TimeStampNow()
Expand All @@ -100,13 +100,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)
Expand All @@ -115,7 +115,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() {
Expand All @@ -124,5 +124,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
}
17 changes: 8 additions & 9 deletions services/auth/group.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"reflect"
"strings"

"code.gitea.io/gitea/models/db"
user_model "code.gitea.io/gitea/models/user"
)

Expand Down Expand Up @@ -81,23 +80,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
}

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 nil, err
}

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
}
14 changes: 7 additions & 7 deletions services/auth/httpsign.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ func (h *HTTPSign) Name() string {
// Verify extracts and validates HTTPsign from the Signature header of the request and returns
// the corresponding user object on successful validation.
// Returns nil if header is empty or validation fails.
func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {
func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
sigHead := req.Header.Get("Signature")
if len(sigHead) == 0 {
return nil
return nil, nil
}

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

publicKey, err = VerifyCert(req)
if err != nil {
log.Debug("VerifyCert on request from %s: failed: %v", req.RemoteAddr, err)
log.Warn("Failed authentication attempt from %s", req.RemoteAddr)
return nil
return nil, nil
}
} else {
// Handle Signature signed by Public Key
publicKey, err = VerifyPubKey(req)
if err != nil {
log.Debug("VerifyPubKey on request from %s: failed: %v", req.RemoteAddr, err)
log.Warn("Failed authentication attempt from %s", req.RemoteAddr)
return nil
return nil, nil
}
}

u, err := user_model.GetUserByID(publicKey.OwnerID)
if err != nil {
log.Error("GetUserByID: %v", err)
return nil
return nil, err
}

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

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

return u
return u, nil
}

func VerifyPubKey(r *http.Request) (*asymkey_model.PublicKey, error) {
Expand Down
5 changes: 3 additions & 2 deletions services/auth/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ type Method interface {
// If verification is successful returns either an existing user object (with id > 0)
// or a new user object (with id = 0) populated with the information that was found
// in the authentication data (username or email).
// Returns nil if verification fails.
Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User
// Second argument returns err if verification fails, otherwise
// First return argument returns nil if no matched verification condition
Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error)
}

// Initializable represents a structure that requires initialization
Expand Down
14 changes: 5 additions & 9 deletions services/auth/oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,14 @@ func (o *OAuth2) userIDFromToken(req *http.Request, store DataStore) int64 {
// or the "Authorization" header and returns the corresponding user object for that ID.
// If verification is successful returns an existing user object.
// Returns nil if verification fails.
func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {
if !db.HasEngine {
return nil
}

func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) {
return nil
return nil, nil
}

id := o.userIDFromToken(req, store)
if id <= 0 {
return nil
return nil, nil
}
log.Trace("OAuth2 Authorization: Found token for user[%d]", id)

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

log.Trace("OAuth2 Authorization: Logged in user %-v", user)
return user
return user, nil
}

func isAuthenticatedTokenRequest(req *http.Request) bool {
Expand Down
Loading

0 comments on commit f4edc75

Please sign in to comment.