Fix MFA authentication page error: handle user read permissions #1802
Conversation
server/auth/handlers/handler.go
Outdated
| } | ||
|
|
||
| d["user"] = user | ||
| d["user"] = req.AuthUser.User | ||
| d["theme"] = user.Meta.Theme |
There was a problem hiding this comment.
is there a reason you did not change to req.AuthUser here as well?
There was a problem hiding this comment.
Yes, the reason is req.AuthUser doesn't have the most updated data.
| return nil | ||
|
|
||
| // check if err is not nil and cater for MFA by checking if the error is not allowed to read | ||
| if err != nil && !errors.Is(err, service.UserErrNotAllowedToRead()) { |
There was a problem hiding this comment.
what happens when there is a UserErrNotAllowedToRead error?
There was a problem hiding this comment.
In this case, since we have the User details from req.AuthUser and we're searching for the most updated users data, the UserErrNotAllowedToRead is ignored.
There was a problem hiding this comment.
Unless maybe we don't want the current user's theme to be applied on MFA login screen, then we can handle it the same way we handle other errors.
server/auth/handlers/handler.go
Outdated
| if err != nil { | ||
| return nil | ||
|
|
||
| service.CurrentSettings.Privacy.Mask.Email = true |
There was a problem hiding this comment.
how do you know this was true before in all cases? maybe just use a temp variable
KinyaElGrande
force-pushed
the
2023.9.x-fix-mfa-authentication-login-page
branch
from
7bce2ab to
cd6b849
Compare
Ref: #1782