-
-
Notifications
You must be signed in to change notification settings - Fork 387
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
Update to go-pkgz/auth/v2 and golang-jwt/jwt/v5 #1758
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 8638476796Details
💛 - Coveralls |
81ba327
to
9f38749
Compare
Pull Request Test Coverage Report for Build 10192720403Details
💛 - Coveralls |
a1b6029
to
4ac60d0
Compare
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.
looking good, just a couple of trully minor things
@@ -1215,10 +1215,15 @@ func (s *ServerCommand) getAuthenticator(ds *service.DataStore, avas avatar.Stor | |||
if c.User == nil { | |||
return c | |||
} | |||
c.User.SetAdmin(ds.IsAdmin(c.Audience, c.User.ID)) | |||
c.User.SetBoolAttr("blocked", ds.IsBlocked(c.Audience, c.User.ID)) | |||
if len(c.Audience) != 1 { |
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.
I think this check needs some comments explaining what is going on here, specifically how 0 or >1 is causing this lack of update and why.
@@ -107,13 +107,20 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
if err = a.dataService.DeleteUserDetail(claims.Audience, claims.User.ID, engine.AllUserDetails); err != nil { | |||
if len(claims.Audience) != 1 { | |||
rest.SendErrorJSON(w, r, http.StatusBadRequest, fmt.Errorf("bad request"), "can't process token, aud is not a single element", rest.ErrActionRejected) |
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.
Probably changing the error message to indicate which element is expected to be single will be better.
This will also serve as a reference for go-pkgz/auth/v2 migration, and I'll add links to this PR to the readme for the auth package.