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

Set PusherName and PusherID to owner on deploy key to fix pushing with deploy keys #5935

Merged
merged 7 commits into from Feb 3, 2019

Conversation

@zeripath
Copy link
Contributor

zeripath commented Feb 2, 2019

The gitea PushUpdate function requires that the PusherID and PusherName
are real users.

Fix #3795

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Feb 2, 2019

I've initially set the EnvPusherName and EnvPusherID to that of the owner. This is probably a reasonable first step towards mitigating the bug in #3795 however, probably we should probably consider deploy keys slightly differently.

@GiteaBot GiteaBot added the lgtm/need 2 label Feb 2, 2019

@zeripath zeripath added the kind/bug label Feb 2, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 2, 2019

Codecov Report

Merging #5935 into master will increase coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5935      +/-   ##
==========================================
+ Coverage   38.28%   38.53%   +0.25%     
==========================================
  Files         330      330              
  Lines       48518    48518              
==========================================
+ Hits        18576    18698     +122     
+ Misses      27268    27113     -155     
- Partials     2674     2707      +33
Impacted Files Coverage Δ
models/unit.go 0% <0%> (-14.29%) ⬇️
routers/api/v1/api.go 73.54% <0%> (+0.19%) ⬆️
models/issue_comment.go 46.33% <0%> (+0.38%) ⬆️
models/action.go 61.03% <0%> (+0.71%) ⬆️
models/repo_indexer.go 58.05% <0%> (+1.27%) ⬆️
models/ssh_key.go 42.76% <0%> (+2.52%) ⬆️
models/repo.go 47.46% <0%> (+3.04%) ⬆️
modules/indexer/repo.go 68.7% <0%> (+5.34%) ⬆️
routers/api/v1/repo/repo.go 56.63% <0%> (+5.6%) ⬆️
routers/private/key.go 43.63% <0%> (+23.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2902b3a...e5c5094. Read the comment docs.

@techknowlogick techknowlogick added this to the 1.8.0 milestone Feb 2, 2019

@zeripath zeripath changed the title Set PusherName and PusherID to owner on deploy key Set PusherName and PusherID to owner on deploy key to fix pushing with deploy keys Feb 2, 2019

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Feb 3, 2019

@lunny

lunny approved these changes Feb 3, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 3, 2019

@lunny lunny referenced this pull request Feb 3, 2019

Closed

Pushes via deployment write keys do not trigger reindex #3795

1 of 7 tasks complete
@@ -250,6 +250,9 @@ func runServ(c *cli.Context) error {
if err = private.UpdateDeployKeyUpdated(key.ID, repo.ID); err != nil {
fail("Internal error", "UpdateDeployKey: %v", err)
}

os.Setenv(models.EnvPusherName, username)

This comment has been minimized.

@kolaente

kolaente Feb 3, 2019

Member

What about using repo.Owner.NameToLower (I think), just for consistency?

This comment has been minimized.

@zeripath

zeripath Feb 3, 2019

Author Contributor

username is repo.Owner.NameToLower - it's the lowercase'd repo username that's provided to gitea serv.

In the case of the user pushes we use user.Name for this so we probably shouldn't be lowercasing at all, and in fact we should probably change this so it's clear that the "user" pushing this is a deploy key not the owner of the repository at all. However, this is the minimal change that works when previously we didn't have a working system at all previously.

I have added a FIXME note to mark that further thought is needed here.

zeripath added some commits Feb 2, 2019

Set PusherName and PusherID to owner on deploy key
The gitea PushUpdate function requires that the PusherID and PusherName
are real users.

Signed-off-by: Andrew Thornton <art27@cantab.net>

@zeripath zeripath force-pushed the zeripath:issue-3795-deploy-key-settings branch from c9cfc76 to bc2b6ad Feb 3, 2019

zeripath added some commits Feb 3, 2019

use repo.Owner.Name instead of "username"
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Feb 3, 2019

OK I've added an integration test to show that this actually solves the problem in #3795

@zeripath zeripath merged commit 13c0f7d into go-gitea:master Feb 3, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@zeripath zeripath deleted the zeripath:issue-3795-deploy-key-settings branch Feb 3, 2019

zeripath added a commit to zeripath/gitea that referenced this pull request Feb 3, 2019

Fix notifications on pushing with deploy keys by setting hook environ…
…ment variables (go-gitea#5935)

The gitea prerecieve and postrecieve hooks and the gitea PushUpdate function require that the PusherID and PusherName are real users. Previously, these environment variables were not being set when using a deploy key - the main result being that pushing to empty repositories meant that is_empty status was not changed.

I've also added an integration test to ensure that the is_empty status is updated on pushing with a deploy key.

There is a slight issue in that the deploy key is now considered a proxy for the owner - we don't have a way of separating out the deploy key from the owner at present. This can be fixed in another PR.

Fix go-gitea#3795 

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath zeripath added backport/done and removed backport/v1.7 labels Feb 3, 2019

aswild added a commit to aswild/gitea that referenced this pull request Feb 3, 2019

Fix notifications on pushing with deploy keys by setting hook environ…
…ment variables (go-gitea#5935)

The gitea prerecieve and postrecieve hooks and the gitea PushUpdate function require that the PusherID and PusherName are real users. Previously, these environment variables were not being set when using a deploy key - the main result being that pushing to empty repositories meant that is_empty status was not changed.

I've also added an integration test to ensure that the is_empty status is updated on pushing with a deploy key.

There is a slight issue in that the deploy key is now considered a proxy for the owner - we don't have a way of separating out the deploy key from the owner at present. This can be fixed in another PR.

Fix go-gitea#3795

Signed-off-by: Andrew Thornton art27@cantab.net
Cherry-pick: 13c0f7d

techknowlogick added a commit that referenced this pull request Feb 3, 2019

Fix notifications on pushing with deploy keys by setting hook environ…
…ment variables (#5935) (#5944)

The gitea prerecieve and postrecieve hooks and the gitea PushUpdate function require that the PusherID and PusherName are real users. Previously, these environment variables were not being set when using a deploy key - the main result being that pushing to empty repositories meant that is_empty status was not changed.

I've also added an integration test to ensure that the is_empty status is updated on pushing with a deploy key.

There is a slight issue in that the deploy key is now considered a proxy for the owner - we don't have a way of separating out the deploy key from the owner at present. This can be fixed in another PR.

Fix #3795 

Signed-off-by: Andrew Thornton art27@cantab.net
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment