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

Writable deploy keys (closes #671) #3225

Merged
merged 16 commits into from Jan 6, 2018

Conversation

6 participants
@vtemian
Contributor

vtemian commented Dec 18, 2017

Add support for read/write deploy key (as github has).

It introduces a new option for write access and an info note that will show the user key's access mode (read/write).
Those options are supported by the API as well.
deploys
add-ssh

@@ -600,6 +600,9 @@ type DeployKey struct {
Fingerprint string
Content string `xorm:"-"`
Mode AccessMode

This comment has been minimized.

@lafriks

lafriks Dec 18, 2017

Member

Why is access mode needed, why not just use ReadOnly?

This comment has been minimized.

@lafriks

lafriks Dec 18, 2017

Member

Also as table is changed it needs migration to Sync2 that type

This comment has been minimized.

@vtemian

vtemian Dec 18, 2017

Contributor

It's a little tricky...

DeployKey seems to be just a proxy table, with some cached property (like Name and Fingerprint) of PublicKey. Using the same reasoning, I thought that it will be better to have a Mode column (like Name and Fingerprint).

Another way to do this will be to have retrieved the Mode from PublicKey (similar to how Content is retrieved), but this means an extra query for each access (most probably in AfterLoad function, since xorm doesn't that for you...maybe go-xorm/xorm#41 will solve it).

ReadOnly is just a property so I don't have to compare Mode with AccessRead, more like HasRecentActivity.

@@ -31,7 +31,7 @@
{{.Fingerprint}}
</div>
<div class="activity meta">
<i>{{$.i18n.Tr "settings.add_on"}} <span>{{.CreatedUnix.FormatShort}}</span> — <i class="octicon octicon-info"></i> {{if .HasUsed}}{{$.i18n.Tr "settings.last_used"}} <span {{if .HasRecentActivity}}class="green"{{end}}>{{.UpdatedUnix.FormatShort}}</span>{{else}}{{$.i18n.Tr "settings.no_activity"}}{{end}}</i>
<i>{{$.i18n.Tr "settings.add_on"}} <span>{{.CreatedUnix.FormatShort}}</span> — <i class="octicon octicon-info"></i> {{if .HasUsed}}{{$.i18n.Tr "settings.last_used"}} <span {{if .HasRecentActivity}}class="green"{{end}}>{{.UpdatedUnix.FormatShort}}</span>{{else}}{{$.i18n.Tr "settings.no_activity"}}{{end}} - <span>Read{{if not .ReadOnly}} / Write {{end}}</i>

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 24, 2017

Member

text in templates should use i18n locales

@@ -37,3 +39,54 @@ func TestDeleteDeployKeyNoLogin(t *testing.T) {
req := NewRequest(t, "DELETE", "/api/v1/repos/user2/repo1/keys/1")
MakeRequest(t, req, http.StatusUnauthorized)
}
func TestCreateReadOnlyDeployKey(t *testing.T) {

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 24, 2017

Member

Thank you very much for adding tests!

However, our suite of integration tests is becoming quite unwieldy. Would you consider instead writing equivalent unit tests for the DeployKeysPosthandler? See routers/repo/issue_label_test.go for some examples.

This comment has been minimized.

@vtemian

vtemian Dec 28, 2017

Contributor

I wrote the unit tests, but I don't know if it's ok to dump the integration tests.
Those are testing the API and the unit tests the interface.

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 29, 2017

Member

Okay, let's leave the integration tests. Eventually, it might make sense to convert our API integration tests to unit tests, but that's for another day. Thanks for adding unit tests!

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch 5 times, most recently from 95c1675 to a1f29cd Dec 28, 2017

@vtemian

This comment has been minimized.

Contributor

vtemian commented Dec 28, 2017

@lafriks @ethantkoenig Thanks for code review!
Can you guys take a second look?

return fmt.Errorf("Sync2: %v", err)
}
_, err := x.Cols("mode").Update(&DeployKey{

This comment has been minimized.

@lafriks

lafriks Dec 28, 2017

Member

Is this really needed as NOT NULL DEFAULT when adding column should automatically add default value?

settings.no_deploy_keys = You haven't added any deploy keys.
settings.title = Title
settings.deploy_key_content = Content
settings.is_writable = Allow write access
settings.is_writable_info = Can this key be used to <strong>push</strong> to this repository? Deploy keys always have pull access.

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 29, 2017

Member

Duplicate of lines 991-992?

@@ -600,6 +600,9 @@ type DeployKey struct {
Fingerprint string
Content string `xorm:"-"`
Mode AccessMode `xorm:"NOT NULL DEFAULT 1"`
ReadOnly bool `xorm:"-"`

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 29, 2017

Member

nit: could we add a ReadOnly() method instead of adding a field?

ID: newDeployKey.ID,
Name: rawKeyBody.Title,
Content: rawKeyBody.Key,
ReadOnly: true,

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 29, 2017

Member

The ReadOnly field is not stored in the DB, so this condition has no effect. You should instead check the Mode field (which is in the DB). Likewise for the unit test.

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch from fa1a1a1 to dd22f0f Dec 29, 2017

@lunny

This comment has been minimized.

Member

lunny commented Dec 30, 2017

CI failed

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch 2 times, most recently from bb97518 to 0f6ae7c Dec 30, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Jan 3, 2018

@vtemian To fix tests please add deploy_key.yml in fixtures folder with content like here: https://github.com/go-gitea/gitea/blob/master/models/fixtures/gpg_key.yml

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch from 3081099 to 4771472 Jan 3, 2018

@vtemian

This comment has been minimized.

Contributor

vtemian commented Jan 3, 2018

Thanks @lafriks

@codecov-io

This comment has been minimized.

codecov-io commented Jan 3, 2018

Codecov Report

Merging #3225 into master will increase coverage by 0.41%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3225      +/-   ##
==========================================
+ Coverage   34.67%   35.08%   +0.41%     
==========================================
  Files         278      279       +1     
  Lines       40506    40522      +16     
==========================================
+ Hits        14044    14217     +173     
+ Misses      24394    24203     -191     
- Partials     2068     2102      +34
Impacted Files Coverage Δ
models/migrations/migrations.go 2.89% <ø> (ø) ⬆️
modules/auth/user_form.go 25% <ø> (ø) ⬆️
models/migrations/v55.go 0% <0%> (ø)
routers/repo/setting.go 7.34% <100%> (+2.67%) ⬆️
routers/api/v1/repo/key.go 20.49% <100%> (+20.49%) ⬆️
models/ssh_key.go 39.2% <81.81%> (+7.58%) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/lfs.go 28.26% <0%> (+2.17%) ⬆️
... and 5 more

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 70b6c07...b3e094e. Read the comment docs.

@lafriks

lafriks approved these changes Jan 3, 2018

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Jan 3, 2018

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Jan 4, 2018

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Jan 4, 2018

LGTM

@lafriks

This comment has been minimized.

Member

lafriks commented Jan 6, 2018

@vtemian you need to rename migration to v55 as other migration was merged already. As soon as you do that this PR can be merged

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch from 5d232ac to 691820b Jan 6, 2018

@lafriks

This comment has been minimized.

Member

lafriks commented Jan 6, 2018

@vtemian please force push to get tests pass, seems to be random failure not related to this PR

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch from 691820b to 440cd76 Jan 6, 2018

@lafriks

This comment has been minimized.

Member

lafriks commented Jan 6, 2018

@vtemian you have renamed existing v54.go that comes from master not yours

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch 4 times, most recently from 3b0686c to c5f6b56 Jan 6, 2018

@vtemian vtemian force-pushed the vtemian:writable-deploy-keys-671 branch from c5f6b56 to b3e094e Jan 6, 2018

@vtemian

This comment has been minimized.

Contributor

vtemian commented Jan 6, 2018

@lafriks It should be good now. Thanks!

@lafriks lafriks merged commit e78786e into go-gitea:master Jan 6, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment