Skip to content
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

Refactor token-related endpoints #26323

Closed
wants to merge 7 commits into from

Conversation

CaiCandong
Copy link
Member

@CaiCandong CaiCandong commented Aug 4, 2023

The current token endpoints is ambiguous, it doesn't make sense for one user to manipulate another user's token (unless that user is admin)

  • Add /api/v1/user/tokens.(Users manipulate their own tokens)
  • Add /api/v1/admin/users/{username}/tokens.(Admin manipulate user's tokens)
  • Refactor /api/v1/users/{username}/tokens ensure that the {username} in path is consistent with the username of the logged in user (Marked as deprecated)

fixed #26234

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 4, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 4, 2023
@GiteaBot GiteaBot added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 4, 2023
@CaiCandong CaiCandong marked this pull request as draft August 4, 2023 06:54
@CaiCandong CaiCandong changed the title Refactor token-related API Refactor token-related endpoints Aug 4, 2023
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2023
@CaiCandong CaiCandong marked this pull request as draft August 7, 2023 14:08
@CaiCandong
Copy link
Member Author

@lunny @techknowlogick Do we need to provide an API for administrators to manipulate user tokens, and is there a security/privacy issue with that?

@lonix1
Copy link
Contributor

lonix1 commented Aug 9, 2023

Do we need to provide an API for administrators to manipulate user tokens

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.

@CaiCandong CaiCandong marked this pull request as ready for review August 10, 2023 06:48
Comment on lines +573 to +581
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(),
}
}
Copy link
Contributor

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.

@yp05327
Copy link
Contributor

yp05327 commented Aug 15, 2023

These new functions have similar logics to existing codes, maybe we can move them into services?
e.g. services/user/token.go

@CaiCandong CaiCandong closed this Aug 18, 2023
@CaiCandong CaiCandong deleted the refactor-api-token branch September 4, 2023 12:40
@CaiCandong CaiCandong restored the refactor-api-token branch September 13, 2023 14:38
@CaiCandong CaiCandong reopened this Sep 13, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Sep 13, 2023
@CaiCandong CaiCandong removed the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Sep 14, 2023
Comment on lines +188 to +189
func ListAccessTokensDeprecated(ctx *context.APIContext) {
// swagger:operation GET /users/{username}/tokens user userGetTokensDeprecated
Copy link
Member

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/.

Copy link
Member Author

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/.

Copy link
Member

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.

@CaiCandong CaiCandong marked this pull request as draft September 14, 2023 11:08
@CaiCandong CaiCandong marked this pull request as ready for review September 14, 2023 13:10
silverwind added a commit that referenced this pull request Sep 18, 2023
Fix #26234
close #26323
close #27040

---------

Co-authored-by: silverwind <me@silverwind.io>
@CaiCandong CaiCandong deleted the refactor-api-token branch October 3, 2023 03:21
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some endpoints ignore specified account and use auth account instead
5 participants