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.

Sign up for GitHub

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

Jump to bottom

Fix token endpoints ignore specified account #27080

Merged
merged 12 commits into from Sep 18, 2023
Merged

Fix token endpoints ignore specified account #27080

merged 12 commits into from Sep 18, 2023

Conversation

CaiCandong
Copy link
Member

@CaiCandong CaiCandong commented Sep 14, 2023 •
edited

Fix #26234
close #26323
close #27040

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 14, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 14, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Sep 14, 2023
@JakobDev
Copy link
Contributor

#27040 is also related

I'm not in favour of doing it this way. All other /username/{username} endpoints can be used by any other user.

@CaiCandong
Copy link
Member Author

#27040 is also related

I made the same change as #27040 in #26323, but the intent of the person who raised the issue was for admins to manage other users' tokens.
So I'm re-initiating this PR.

@JakobDev
Copy link
Contributor

JakobDev commented Sep 14, 2023 •
edited

If a endpoint is mean to uses by admins, it should be in the /admins route. You should also note that admins can use the sudo function of the API.

@CaiCandong
Copy link
Member Author

If a endpoint is mean to uses by admins, it should be in the /admins route. You should also note that admins can use the sudo function of the API.

Yes, that's how I implemented it in #26323 as well. but

#26323 (comment)

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

@JakobDev
Copy link
Contributor

I don't get that argument. It's nice that you know that, but that doesn't matter. here. A API should try to keep a consistent design to not confuse users. And the current design is, that all endpoints under /username/{username} can be used by any user (some even with login) and that endpoints, who are meant to be used by admins should live under /admin. Most of the users of the API are not Admins.

I think the best way to solve this is to simply move the endpoint to /user/token. It will only work for the current User. If you are a Admin, you can simply use the sudo parameter/header of the API. It works the same way as editing e.g. OAuth2Application though the API or changing the Avatar.

@CaiCandong
Copy link
Member Author

#26234 (comment)

I think the current api is ambiguous, it doesn't make sense for one user to manipulate another user's token > > (unless that user is an admin)

I think the current /api/v1/users/{username}/tokens should be changed to /api/v1/user/tokens.(Users
manipulate their own tokens)
And the admin manipulate token should be placed in: /api/v1/admin/users/{username}/tokens.(Admin
manipulate user's tokens)

My point of view is the same as yours, but no one reviewed #26323 for a long time.
This PR is the least altered, I think it's reasonable too.

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 14, 2023

I we introduce a new role moderator which could edit users and repos but not admin related stuff like auth sources, what do we do? Add all routes again under /moderator?

For me the /users/{username} routes are correct. If you have the permission to perform the action, everything is fine. If you don't have permission, you get the matching error code.

yp05327
yp05327 reviewed Sep 15, 2023
View reviewed changes
routers/api/v1/user/app.go Outdated Show resolved Hide resolved
@JakobDev
Copy link
Contributor

I'm not saying the current system is perfect, but at least we have a system. And in the current system we use the /user route for User stuff like e.g. editing OAuth2 Apps, GPG keys, avatars, emails, hooks, settings etc. This /user (not /users/<username>) route is for the current User. If a Admin wants to use those routes, he can use the sudo functionality. This may have quirks, but this is how it's currently works. I see no reason why a single endpoint should work differently.

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 15, 2023

I see no reason why a single endpoint should work differently.

This is the first and only endpoint, so there is no difference to other endpoints.
Gitlab follows the same scheme:
https://docs.gitlab.com/ee/api/users.html#add-ssh-key

Creates a new key owned by the authenticated user.
POST /user/keys
Create new key owned by specified user. Available only for administrator.
POST /users/:id/keys

@CaiCandong
Copy link
Member Author

CaiCandong commented Sep 15, 2023 •
edited

@waTeim @lonix1 mentioned that need to use these endpoints. What do you think?

@JakobDev
Copy link
Contributor

This is the first and only endpoint, so there is no difference to other endpoints.

As I said, there are other endpoints to edit the settings of a User

Gitlab follows the same scheme:

This is Gitea, not GitLab

@silverwind
Copy link
Member

silverwind commented Sep 15, 2023 •
edited

I'm a strong proponent of always using plural on REST API URLs. It's the only sane design. It allows construction of URLs, eases URL-based permission scheme, allows grouping by endpoint in swagger, etc. There are numerous benefits of avoiding these singulars.

https://stackoverflow.com/a/21809963/808699

The only valid case for singular is if the resource is a singleton, e.g./users/id/name.

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 15, 2023

This is the first and only endpoint, so there is no difference to other endpoints.

As I said, there are other endpoints to edit the settings of a User

Yep, in the admin area where they should not be.

Gitlab follows the same scheme:

This is Gitea, not GitLab

They did not make the mistake.

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 15, 2023

I'm a strong proponent of always using plural on REST API URLs.

We do that, just not for the /user/ endpoint which uses the calling user as context.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 15, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 16, 2023
@puni9869
Copy link
Member

puni9869 commented Sep 16, 2023 •
edited

Please run the make generate-swagger once again.

@CaiCandong
Copy link
Member Author

Please run the make generate-swagger once again.

No new changes were made to Swagger, and the re-execution has nothing to commit.

@silverwind silverwind enabled auto-merge (squash) September 17, 2023 23:50
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 17, 2023
@silverwind silverwind merged commit f93ee59 into go-gitea:main Sep 18, 2023
26 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 18, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 18, 2023
@lunny lunny modified the milestones: 1.22.0, 1.21.0 Sep 18, 2023
@CaiCandong
Copy link
Member Author

Do we need to backport to 1.20? @lunny

@puni9869
Copy link
Member

No I guess

silverwind added a commit to silverwind/gitea that referenced this pull request Sep 18, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into primary
9a23d22 
* origin/main:
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 20, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
add8afe 
* giteaofficial/main:
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
@CaiCandong CaiCandong deleted the bugfix/ignore-specified-account branch October 3, 2023 03:21
@evantobin evantobin mentioned this pull request Oct 13, 2023
Merged
6543 pushed a commit that referenced this pull request Oct 14, 2023
@evantobin
Fix permissions for Token DELETE endpoint to match GET and POST (#27610)
ae419fa 
Fixes #27598

In #27080, the logic for the tokens endpoints were updated to allow
admins to create and view tokens in other accounts. However, the same
functionality was not added to the DELETE endpoint. This PR makes the
DELETE endpoint function the same as the other token endpoints and adds unit tests
@GiteaBot GiteaBot mentioned this pull request Nov 17, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 17, 2023
@evantobin @GiteaBot
Fix permissions for Token DELETE endpoint to match GET and POST (go-g…
99b18f8 
…itea#27610)

Fixes go-gitea#27598

In go-gitea#27080, the logic for the tokens endpoints were updated to allow
admins to create and view tokens in other accounts. However, the same
functionality was not added to the DELETE endpoint. This PR makes the
DELETE endpoint function the same as the other token endpoints and adds unit tests
lunny pushed a commit that referenced this pull request Nov 17, 2023
@GiteaBot @evantobin
Fix permissions for Token DELETE endpoint to match GET and POST (#27610…
93ede4b 
…) (#28099)

Backport #27610 by @evantobin

Fixes #27598

In #27080, the logic for the tokens endpoints were updated to allow
admins to create and view tokens in other accounts. However, the same
functionality was not added to the DELETE endpoint. This PR makes the
DELETE endpoint function the same as the other token endpoints and adds
unit tests

Co-authored-by: Evan Tobin <me@evantob.in>
@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.
Reviewers

@silverwind silverwind silverwind approved these changes

@yp05327 yp05327 yp05327 left review comments

@KN4CK3R KN4CK3R KN4CK3R approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.

Some endpoints ignore specified account and use auth account instead
8 participants
@CaiCandong @JakobDev @KN4CK3R @silverwind @puni9869 @yp05327 @lunny @GiteaBot