Skip to content

Ensure deploy key with write access can push #19010

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
Merged
merged 12 commits into from
Mar 22, 2022
Merged

Ensure deploy key with write access can push #19010

merged 12 commits into from
Mar 22, 2022

Conversation

wxiaoguang
Copy link
Contributor

Fix:

In this PR, I use DeployKeyID to replace the IsDeployKey, then CanWriteCode can use the DeployKeyID to check the permission.

@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Mar 5, 2022
@wxiaoguang wxiaoguang linked an issue Mar 5, 2022 that may be closed by this pull request
Deploy key with write access can not push #19009
Closed
lunny
lunny reviewed Mar 6, 2022
View reviewed changes
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 6, 2022
@wxiaoguang wxiaoguang requested review from lunny, zeripath and 6543 March 10, 2022 04:03
Gusted
Gusted reviewed Mar 10, 2022
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 Mar 20, 2022
@wxiaoguang wxiaoguang force-pushed the fix-deploy-key-write branch from 1b04680 to 60ac321 Compare March 20, 2022 19:03
6543
6543 reviewed Mar 22, 2022
View reviewed changes
@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 Mar 22, 2022
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 22, 2022 •
edited
Loading

@6543 are you sure the APIContext can be used as db.Context?

Hmm, checked, db.GetEngine works like this. Thanks ~~

// GetEngine will get a db Engine from this context or return an Engine restricted to this context
func GetEngine(ctx context.Context) Engine {
	if engined, ok := ctx.(Engined); ok {
		return engined.Engine()
	}
	enginedInterface := ctx.Value(EnginedContextKey)
	if enginedInterface != nil {
		return enginedInterface.(Engined).Engine()
	}
	return x.Context(ctx)
}

@wxiaoguang
Copy link
Contributor Author

There are a lot of code using db.DefaultContext in routers, maybe they should all be refactored to the new framework.

$ grep -r "db\.DefaultContext" ./routers | wc -l
56

@wxiaoguang wxiaoguang mentioned this pull request Mar 22, 2022
Merged
@wxiaoguang wxiaoguang merged commit 2b55422 into go-gitea:main Mar 22, 2022
@wxiaoguang wxiaoguang deleted the fix-deploy-key-write branch March 22, 2022 09:29
@silverwind
Copy link
Member

Backport necessary?

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 22, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
f9c05a9 
* giteaofficial/main:
  Fix the bug: deploy key with write access can not push (go-gitea#19010)
  Renamed ctx.User to ctx.Doer. (go-gitea#19161)
  [docs] Enhance container selection in docker dump (go-gitea#14292)
  Cleanup protected branches when deleting users & teams (go-gitea#19158)
  Reorder issue templates and automatically add labels (go-gitea#18875)
  Use IterateBufferSize whilst querying repositories during adoption check (go-gitea#19140)
@zeripath
Copy link
Contributor

I'm confused as to how this was failing because our testsuite should have picked this up... It would be helpful if you could add a test to our integrations suite that replicates the bug or give us a run down of the steps necessary to reproduce the original issue.

@wxiaoguang
Copy link
Contributor Author

I have read the test cases before, if I understand correctly it seems that this case is not covered. I had the thought to write some tests at the same time however it's more complex than I thought, and I need this to be fixed in my server, so I just submitted the fix first. (although not fully covered by tests, but not too bad either)

I have a plan to do some further refactoring about the ssh/serv/internal system, and I will add more tests then.

About backport: this PR changes some env vars, it not as simple as a bug fix, so I think we can just keep it in 1.17. If anyone in 1.16 are affected, we can suggest them to use 1.17-dev or have further discussion then. Well ..... it seems that no one besides me need this feature 😂

@wxiaoguang
Copy link
Contributor Author

About "the steps necessary to reproduce the original issue.": that's quite straight, add a deploy key with write access, then you can not use the key to push. Because there was no code to check the key's permission to allow write.

@zeripath zeripath changed the title Fix the bug: deploy key with write access can not push Ensure deploy key with write access can push Mar 23, 2022
zeripath pushed a commit to zeripath/gitea that referenced this pull request Mar 23, 2022
@wxiaoguang @zeripath
Fix the bug: deploy key with write access can not push (go-gitea#19010)
08f06af 
Backport go-gitea#19010

Use DeployKeyID to replace the IsDeployKey, then CanWriteCode uses the DeployKeyID to check the write permission.

Fix go-gitea#19009

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@zeripath zeripath mentioned this pull request Mar 23, 2022
Merged
@zeripath zeripath added the backport/done All backports for this PR have been created label Mar 23, 2022
zeripath added a commit that referenced this pull request Mar 23, 2022
@zeripath @wxiaoguang
Fix the bug: deploy key with write access can not push (#19010) (#19182)
93feb1a 
Backport #19010

Use DeployKeyID to replace the IsDeployKey, then CanWriteCode uses the DeployKeyID to check the write permission.

Fix #19009

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 23, 2022
@zeripath
Changelog for 1.16.5
fb7de1c 
 ## [1.16.5](https://github.com/go-gitea/gitea/releases/tag/1.16.5) - 2022-03-23

* BREAKING
  * Bump to build with go1.18 (go-gitea#19120 et al) (go-gitea#19127)
* SECURITY
  * Prevent redirect to Host (2) (go-gitea#19175) (go-gitea#19186)
  * Try to prevent autolinking of displaynames by email readers (go-gitea#19169) (go-gitea#19183)
  * Clean paths when looking in Storage (go-gitea#19124) (go-gitea#19179)
  * Do not send notification emails to inactive users (go-gitea#19131) (go-gitea#19139)
  * Do not send activation email if manual confirm is set (go-gitea#19119) (go-gitea#19122)
* ENHANCEMENTS
  * Use the new/choose link for New Issue on project page (go-gitea#19172) (go-gitea#19176)
* BUGFIXES
  * Fix compare link in active feeds for new branch (go-gitea#19149) (go-gitea#19185)
  * Redirect .wiki/* ui link to /wiki (go-gitea#18831) (go-gitea#19184)
  * Ensure deploy keys with write access can push (go-gitea#19010) (go-gitea#19182)
  * Ensure that setting.LocalURL always has a trailing slash (go-gitea#19171) (go-gitea#19177)
  * Cleanup protected branches when deleting users & teams (go-gitea#19158) (go-gitea#19174)
  * Use IterateBufferSize whilst querying repositories during adoption check (go-gitea#19140) (go-gitea#19160)
  * Fix NPE /repos/issues/search when not signed in (go-gitea#19154) (go-gitea#19155)
  * Use custom favicon when viewing static files if it exists (go-gitea#19130) (go-gitea#19152)
  * Fix the editor height in review box (go-gitea#19003) (go-gitea#19147)
  * Ensure isSSH is set whenever DISABLE_HTTP_GIT is set (go-gitea#19028) (go-gitea#19146)
  * Fix wrong scopes caused by empty scope input (go-gitea#19029) (go-gitea#19145)
  * Make migrations SKIP_TLS_VERIFY apply to git too (go-gitea#19132) (go-gitea#19141)
  * Handle email address not exist (go-gitea#19089) (go-gitea#19121)
* MISC
  * Update json-iterator to allow compilation with go1.18 (go-gitea#18644) (go-gitea#19100)
  * Update golang.org/x/crypto (go-gitea#19097) (go-gitea#19098)

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Mar 23, 2022
Merged
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@wxiaoguang
Fix the bug: deploy key with write access can not push (go-gitea#19010)
3922f45 
Use DeployKeyID to replace the IsDeployKey, then CanWriteCode uses the DeployKeyID to check the write permission.
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 approved these changes

@lunny lunny Awaiting requested review from lunny

@zeripath zeripath Awaiting requested review from zeripath

+1 more reviewer

@Gusted Gusted Gusted approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Deploy key with write access can not push
7 participants
@wxiaoguang @silverwind @zeripath @lunny @6543 @Gusted @GiteaBot