Only allow token authentication with 2FA enabled - #2184
Conversation
| } else { | ||
| _, err = models.GetTwoFactorByUID(authUser.ID) | ||
|
|
||
| if err == nil { |
There was a problem hiding this comment.
You need to also check that if !models.IsErrTwoFactorNotEnrolled(err) and do ctx.Handle(http.StatusInternalServerError, "IsErrTwoFactorNotEnrolled", err) + return otherwise if there is some kind of unexpected error you will let user in and it will bypass 2fa
| token, err := models.GetAccessTokenBySHA(authUsername) | ||
| if authUser == nil { | ||
| // Assume password is a token. | ||
| token, err := models.GetAccessTokenBySHA(authPasswd) |
There was a problem hiding this comment.
You need to also check if user models.GetUserByName(authUsername) returns correct user and validate that returned user.ID is equal to token.UID
lafriks
added
type/bug
topic/security
|
@lafriks I refactored the routine according to your suggestions 👍 |
tboerger
added
the
lgtm/need 2
| return | ||
| } | ||
|
|
||
| tokenUser, err := models.GetUserByID(token.UID) |
There was a problem hiding this comment.
No need to get user by ID, you can just compare authUser.ID != token.UID on line 190
|
|
||
| if err != nil { | ||
| ctx.Handle(http.StatusInternalServerError, "GetUserByID", err) | ||
| if !models.IsErrTwoFactorNotEnrolled(err) { |
There was a problem hiding this comment.
I think better structure for this code would be:
if err == nil {
ctx.HandleText(http.StatusUnauthorized, "Users with two-factor authentication enabled cannot perform HTTP/HTTPS operations via plain username and password. Please create and use a personal access token on the user settings page")
return
} else if !models.IsErrTwoFactorNotEnrolled(err) {
ctx.Handle(http.StatusInternalServerError, "IsErrTwoFactorNotEnrolled", err)
return
}
|
@moritzheiber few more optimizations needed and than from me it would be LG-TM |
…tor authentication
|
@lafriks Should be all there now |
|
LGTM |
lafriks
added
lgtm/need 1
|
@moritzheiber I have tested and works as expected only thing that I found that does not match GitHub behaviour is that when 2FA is enabled it returns |
|
@lafriks The only grievance I'd have with that is that within Gitea that's completely undocumented. Nothing on the two-factor authentication page tells you that you need to create an application token in order to login while having 2FA enabled: This is probably going to confuse a lot of people. I know the response is probably the wrong time and place of conveying this, but as of right now it's the only one. |
|
Maybe than add todo comment that later for security reasons response should be changed to |
|
@lafriks Done. |
lafriks
added
lgtm/done
|
LGTM |
|
LGTM |
|
make CI work |
6 participants
Fixes #1394
Previously, when a user had 2FA enabled they were still able to authenticate via HTTP(S) using their regular username and password. This PR makes token authentication mandatory for accounts with 2FA enabled.