Skip to content

modify api authentication failed message #27842

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

modify api authentication failed message #27842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3519973
modify api authentication failed message
LeonDevLifeLog Oct 30, 2023
e009527
change error type
LeonDevLifeLog Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions models/auth/access_token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,23 @@ func (err ErrAccessTokenNotExist) Unwrap() error {
return util.ErrNotExist
}

// ErrBadAccessToken represents a "BadAccessToken" kind of error.
type ErrBadAccessToken struct{}

// IsErrBadAccessToken checks if an error is a ErrBadAccessToken.
func IsErrBadAccessToken(err error) bool {
_, ok := err.(ErrBadAccessToken)
return ok
}

func (err ErrBadAccessToken) Error() string {
return "Bad credentials or token"
}

func (err ErrBadAccessToken) Unwrap() error {
return util.ErrInvalidArgument
}

// ErrAccessTokenEmpty represents a "AccessTokenEmpty" kind of error.
type ErrAccessTokenEmpty struct{}

Expand Down
5 changes: 4 additions & 1 deletion routers/common/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
package common

import (
auth_model "code.gitea.io/gitea/models/auth"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/web/middleware"
auth_service "code.gitea.io/gitea/services/auth"
)
Expand All @@ -18,7 +20,8 @@ type AuthResult struct {
func AuthShared(ctx *context.Base, sessionStore auth_service.SessionStore, authMethod auth_service.Method) (ar AuthResult, err error) {
ar.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, sessionStore)
if err != nil {
return ar, err
log.Warn("authentication failed", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to detect the errors type to decide whether error we should return.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means to only return ErrBadAccessToken for authentication related errors?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Verify method returns various kinds of errors because it includes various auth methods. However, detecting the error type is also a tough task. I can recall there is a bugfix PR trying to fix 500 after deleting a user with the wrong password. But the PR is stuck because it can't cover error types for all cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny @lng2020
Maybe this func (userIDFromToken) could return type int64,error , by modifying the return type, the error type can be returned correctly.

gitea/services/auth/oauth2.go

Lines 139 to 143 in a18ecae

id := o.userIDFromToken(req.Context(), token, store)
if id <= 0 && id != -2 { // -2 means actions, so we need to allow it.
return nil, user_model.ErrUserNotExist{}
}

If you approve i will close this PR and open another one.

return ar, auth_model.ErrBadAccessToken{}
}
if ar.Doer != nil {
if ctx.Locale.Language() != ar.Doer.Language {
Expand Down