Skip to content

Silently ignore the obsolete sudo token scope #25156

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
Closed
wants to merge 2 commits into from
Closed

Silently ignore the obsolete sudo token scope #25156

wants to merge 2 commits into from

Conversation

@earl-warren
Copy link
Contributor

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2023
delvh
delvh previously approved these changes Jun 8, 2023
View reviewed changes
@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 Jun 8, 2023
@earl-warren @delvh
Update models/auth/token_scope.go
0c8c33c 
Co-authored-by: delvh <dev.lh@web.de>
@delvh delvh changed the title [GITEA] silently ignore obsolete sudo scope Silently ignore the obsolete sudo token scope Jun 8, 2023
techknowlogick
techknowlogick requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure of this change, as it silently hides an error. Users should be alerted if they are using incorrect scopes even if it were previously accepted. By hiding the error it adds the possibility that users are expecting sudo to work, or introduce hidden issues with external tools.
@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 8, 2023
@jackHay22
Copy link
Contributor

Why is sudo a special case here? The sudo scope is included in the migration that updates the scopes for existing tokens. Is there a reason that only sudo needs to be ignored?
@delvh
Copy link
Member

delvh commented Jun 8, 2023

If I had to guess, https://discord.com/channels/322538954119184384/322910365237248000/1116339032033603584 encountered the same problem.
So, my guess is there's a bug in the migration regarding sudo scopes?
@jackHay22
Copy link
Contributor

So, my guess is there's a bug in the migration regarding sudo scopes?

My understanding of the issue is that a user tried to create a new access token using the sudo scope which is no longer supported. The same logic could be applied to any of the old scopes that are no longer supported. It's not clear to me that this is an issue with the migration.
@delvh delvh dismissed their stale review June 8, 2023 16:52

Hmm… I do agree that the mentioned issue seems to be a bit different than the discord message I posted above. The issue sounds indeed like someone still tries to use the no longer accepted scope. The message I posted, on the other hand, said that existing tokens don't work anymore after the upgrade…

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged topic/authentication type/bug

5 participants

@earl-warren @jackHay22 @delvh @techknowlogick @GiteaBot