-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Refactor token-related endpoints #26323
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
Conversation
|
@lunny @techknowlogick Do we need to provide an API for administrators to manipulate user tokens, and is there a security/privacy issue with that? |
You do if you don't want to break usability for many users, myself included. It's been possible until now. Use case: I provision everything via automation (ansible). If you remove the ability to completely provision a server (using an admin account, of course), then it won't be possible to use gitea with automation. |
| apiTokens := make([]*api.AccessToken, len(tokens)) | ||
| for i := range tokens { | ||
| apiTokens[i] = &api.AccessToken{ | ||
| ID: tokens[i].ID, | ||
| Name: tokens[i].Name, | ||
| TokenLastEight: tokens[i].TokenLastEight, | ||
| Scopes: tokens[i].Scope.StringSlice(), | ||
| } | ||
| } |
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.
You can add ToTokens and ToToken in services/convert package
Same to the others.
|
These new functions have similar logics to existing codes, maybe we can move them into services? |
| func ListAccessTokensDeprecated(ctx *context.APIContext) { | ||
| // swagger:operation GET /users/{username}/tokens user userGetTokensDeprecated |
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.
As an admin I would like it more to use this instead of an admin/users endpoint. I know I'm an admin, I don't need every of my actions behind admin/.
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.
Right, but for compatibility reasons I don't want to change the existing API behavior at the moment, so it's labeled Deprecated. Then the admin action is placed after admin/.
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.
Just label it as bugfix "/users/{username}/tokens does not respect username parameter" and breaking.
Fix #26234 close #26323 close #27040 --------- Co-authored-by: silverwind <[email protected]>
The current
tokenendpoints is ambiguous, it doesn't make sense for one user to manipulate another user's token (unless that user is admin)/api/v1/user/tokens.(Users manipulate their own tokens)/api/v1/admin/users/{username}/tokens.(Admin manipulate user's tokens)/api/v1/users/{username}/tokensensure that the{username}in path is consistent with theusernameof the logged in user (Marked as deprecated)fixed #26234