Skip to content

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

@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

@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
Collaborator

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
@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
@wxiaoguang
Copy link
Contributor

-> Avoid GCM OAuth2 attempt when OAuth2 is disabled #36000

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

6 participants