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

Ensure deploy key with write access can push #19010

Merged
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
cmd/serv.go Show resolved Hide resolved
@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
@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
routers/api/v1/repo/key.go Outdated Show resolved Hide resolved
models/asymkey/ssh_key_deploy.go Outdated Show resolved Hide resolved
@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

@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

@silverwind
Copy link
Member

Backport necessary?

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 22, 2022
* 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
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 <[email protected]>
@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
Backport #19010

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

Fix #19009

Co-authored-by: wxiaoguang <[email protected]>
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 23, 2022
 ## [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 <[email protected]>
@zeripath zeripath mentioned this pull request Mar 23, 2022
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
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.
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
Development

Successfully merging this pull request may close these issues.

Deploy key with write access can not push
7 participants