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

Remove check on username when using AccessToken authentication for the API #11015

Merged
merged 7 commits into from
Apr 14, 2020
Merged

Remove check on username when using AccessToken authentication for the API #11015

merged 7 commits into from
Apr 14, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Apr 8, 2020 •
edited
Loading

As highlighted by #10902 the current situation of authentication with access tokens is inconsistent.

This PR is an alternative to #11014, and simply removes the username check making this consistent with Oauth2 tokens.

Closes #10902
Closes #11014
Fixes #10903

Signed-off-by: Andrew Thornton [email protected]

This was referenced Apr 8, 2020
Closed
Closed
6543
6543 approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@6543 6543 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 ok with this since it wont make a huge diff in securety

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 9, 2020
@6543
Copy link
Member

6543 commented Apr 9, 2020

@zeripath can you add the specivic labels to this pull :)

guillep2k
guillep2k approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

Code looks good to me, but I don't know much about the several auth options required by different usage scenarios, so I don't have an opinion whether this or #11014 should prevail.

@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 Apr 9, 2020
@guillep2k guillep2k added modifies/api This PR adds API routes or modifies them pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Apr 9, 2020
@guillep2k guillep2k added this to the 1.12.0 milestone Apr 9, 2020
@jolheiser
Copy link
Member

Ping lgtm

@jolheiser
Copy link
Member

Ping lgtm

@jolheiser jolheiser merged commit 7c48085 into go-gitea:master Apr 14, 2020
@zeripath zeripath changed the title Remove check on username if AccessToken authentication Remove check on username when using AccessToken authentication for the API May 17, 2020
@zeripath zeripath mentioned this pull request May 17, 2020
Merged
@twoequaldots twoequaldots mentioned this pull request Jun 22, 2020
Closed
8 tasks
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@zeripath
Remove check on username if AccessToken authentication (go-gitea#11015)
cb8cc11 
Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath deleted the no-username-check-if-token branch September 13, 2020 12:34
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@guillep2k guillep2k guillep2k approved these changes

@6543 6543 6543 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 pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

can not use "https://oauth2:<token>@url" to clone code
6 participants
@zeripath @6543 @jolheiser @guillep2k @GiteaBot @lunny