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

Pre-register OAuth2 applications for git credential helpers #26291

Merged
merged 24 commits into from Aug 9, 2023

Conversation

denyskon
Copy link
Member

@denyskon denyskon commented Aug 2, 2023

This PR is an extended implementation of #25189 and builds upon the proposal by @hickford in #25653, utilizing some ideas proposed internally by @wxiaoguang.

Mainly, this PR consists of a mechanism to pre-register OAuth2 applications on startup, which can be enabled or disabled by modifying the [oauth2].DEFAULT_APPLICATIONS parameter in app.ini. The OAuth2 applications registered this way are being marked as "locked" and neither be deleted nor edited over UI to prevent confusing/unexpected behavior. Instead, they're being removed if no longer enabled in config.

grafik

The implemented mechanism can also be used to pre-register other OAuth2 applications in the future, if wanted.

Co-authored-by: hickford mirth.hickford@gmail.com
Co-authored-by: wxiaoguang wxiaoguang@gmail.com

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 2, 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 2, 2023
@denyskon denyskon added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Aug 2, 2023
@denyskon denyskon added this to the 1.21.0 milestone Aug 2, 2023
Copy link
Contributor

@hickford hickford left a comment

Choose a reason for hiding this comment

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

LGTM. Nice solution, great work.

models/auth/oauth2.go Outdated Show resolved Hide resolved
modules/util/slice.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Aug 3, 2023

Maybe we need to check the delete functions to don't allow delete OAuth2 Applications from both UI and API.

@denyskon
Copy link
Member Author

denyskon commented Aug 3, 2023

@lunny done

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 4, 2023

Actually, the "ClientID" can be any string if I understand correctly. If so, maybe it's even better to use "builtin-git-cred-oauth" / "builtin-git-cred-manager", it brings consistent user experience for the config too:

DEFAULT_APPLICATIONS = builtin-git-cred-oauth, builtin-git-cred-manager

And the pre-defined ClientIDs could avoid the database migration (no need to add the Locked field, just check the ClientID)

		{
			Name:         "Git Credential Manager",
			ClientID:     "builtin-git-cred-manager",
			RedirectURIs: []string{"http://127.0.0.1", "https://127.0.0.1"},
		},

@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 Aug 4, 2023
@wxiaoguang
Copy link
Contributor

If you don't mind, could I push some changes for idea #26291 (comment) ?

@denyskon
Copy link
Member Author

denyskon commented Aug 4, 2023

@wxiaoguang Feel free to make any changes if you want

@denyskon
Copy link
Member Author

denyskon commented Aug 8, 2023

@wxiaoguang Do you want any more changes to be made?

@wxiaoguang
Copy link
Contributor

@wxiaoguang Do you want any more changes to be made?

Nope. But I am not sure whether allowing site admin editing the name/URL/confidential is good ... Personally I would agree more with hickford: allowing editing would confuse users.

@denyskon
Copy link
Member Author

denyskon commented Aug 8, 2023

Even though I think there could be use cases for it, I'm fine with disallowing edits as the concerns are valid and changes to confidential/redirect URIs could break the whole functionality

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I think the client IDs could be written into the document, to make them could be found easily.

@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 Aug 8, 2023
@denyskon
Copy link
Member Author

denyskon commented Aug 8, 2023

I changed the behavior to disallow editing and also changed the UI, as now we don't need any buttons (and as there is no proper way to disable link buttons 😆)

grafik

@denyskon
Copy link
Member Author

denyskon commented Aug 8, 2023

I think the client IDs could be written into the document, to make them could be found easily.

Done

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 8, 2023
@GiteaBot
Copy link
Contributor

GiteaBot commented Aug 8, 2023

@denyskon please fix the merge conflicts. 🍵

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 8, 2023
@denyskon denyskon added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 8, 2023
@KN4CK3R KN4CK3R merged commit 63ab92d into go-gitea:main Aug 9, 2023
24 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 9, 2023
@denyskon denyskon deleted the git-credential-oauth branch August 9, 2023 19:51
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 10, 2023
* upstream/main:
  Pre-register OAuth2 applications for git credential helpers (go-gitea#26291)
  Make `user-content-* ` consistent with github (go-gitea#26388)
  Add pull request review request webhook event (go-gitea#26401)
  Introduce ctx.PathParamRaw to avoid incorrect unescaping (go-gitea#26392)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants