Skip to content

WIP: Rationalise Disable Basic Authentication and SSO #15186

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

Closed
wants to merge 19 commits into from

Conversation

zeripath
Copy link
Contributor

This PR adds a few changes:

  • Extend disable Basic Authentication to git and LFS
  • Extend authentication of LFS to use OAuth2
  • Extend basic authentication of LFS to use user.SignIn
  • Change disable BasicAuthentication to only disable user authentication with password
  • Fix Bug: Users authenticated via proxy-auth can change username #2407 by preventing ReverseProxy authenticated users from changing username
  • Default reverseproxy users to have an empty password not the username

@zeripath zeripath added the pr/wip This PR is not ready for review label Mar 28, 2021
@zeripath
Copy link
Contributor Author

Need to make the basic authentication only get checked once on git and lfs endpoints

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 31, 2021
@zeripath zeripath force-pushed the disable-basic-authentication branch 3 times, most recently from 6c987fa to 5a2624a Compare April 5, 2021 18:48
zeripath added 17 commits April 5, 2021 20:27
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
The recovery handler should not attempt to reauthenticate the user -
as this could have been the site of the panic!
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
…that reverseproxy creates a session

Signed-off-by: Andrew Thornton <[email protected]>
zeripath added 2 commits April 5, 2021 21:20
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
@silverwind
Copy link
Member

Extend disable Basic Authentication to git and LFS

Care to explain? This does not disable basic auth on git endpoints, right?

@zeripath
Copy link
Contributor Author

zeripath commented Apr 6, 2021

Extend disable Basic Authentication to git and LFS

Care to explain? This does not disable basic auth on git endpoints, right?

Actually if DisableBasicAuthentication is set, it would.

There is an extremely good reason as to why we should do that but we should move this discussion to #15303.

@zeripath zeripath changed the title WIP: Rationalise Disable Basic Authentication WIP: Rationalise Disable Basic Authentication and SSO Apr 6, 2021
@silverwind
Copy link
Member

silverwind commented Apr 6, 2021

Ah, I see as long as token auth is still supported on HTTPS, I'm fine. Basic auth with username/password should be something that is discouraged.

@zeripath
Copy link
Contributor Author

zeripath commented May 1, 2021

Closing as all of these changes have been split out into other PRs.

(Which aren't being reviewed...)

@zeripath zeripath closed this May 1, 2021
@zeripath zeripath deleted the disable-basic-authentication branch May 1, 2021 23:17
zeripath added a commit that referenced this pull request May 4, 2021
…15301)

Since the move to Chi the number of stack frames has proliferated somewhat catastrophically and we're up to 96 frames with multiple tests of the url outside of a trie which is inefficient.

This PR reduces the number of stack frames by 6 through careful use of Route, moves Captcha into its own router so that it only fires on Captcha routes, similarly for avatars and repo-avatars.

The robots.txt, / and apple-touch-icon.png are moved out of requiring Contexter.

It moves access logger higher in the stack frame because there is no reason why it can't be higher.

Extract from #15186
Contains #15292
@go-gitea go-gitea locked and limited conversation to collaborators Jun 4, 2021
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. pr/wip This PR is not ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Users authenticated via proxy-auth can change username
3 participants