Delete GUN functionality, requires * #819
Conversation
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
riyazdf
force-pushed
the
admin-create-delete
branch
from
fbdb608
to
dc3c9a8
Compare
| require.NoError(t, repo.Publish()) | ||
|
|
||
| // Create another repo to ensure it stays intact | ||
| livingGun := "stayingAlive" |
There was a problem hiding this comment.
👍 Yay, thanks for ensuring that everything in the server isn't blown away. Also awesome name.
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
| require.NotNil(t, auth) | ||
| } | ||
|
|
||
| func TestAdminTokenAuth200Status(t *testing.T) { |
There was a problem hiding this comment.
Totally non-blocking: good to have these here, but I'm not sure our existing auth tests actually do anything either other than reflect the 401 unauthorized handler. We don't actually test which permissions are asked for - I'm wondering if it's worth writing a mock jwt authorizer, so we can ensure that notary is asking for the correct permissions. However, a lot of that stuff is tested in the distribution library we depend on too.
What do you think? I can file an issue if that plumbing would be a useful thing for us to test.
There was a problem hiding this comment.
Yeah, definitely - I was wondering how we could better test for this, and @endophage might have more context around the token auth code and tests in distribution. I think a tracking issue would be good 👍
|
LGTM pending CI! Thanks for adding this so quickly! |
|
|
||
| // getAdminTransport returns an admin http.RoundTripper to be used for all http requests, | ||
| // and can be used for deleting repos | ||
| func getAdminTransport(config *viper.Viper, gun string) (http.RoundTripper, error) { |
There was a problem hiding this comment.
Unless I'm missing something this is a dupe of getTransport immediately above with the only difference being readOnly is implicitly false. Seems unnecessary, especially as it's untested.
There was a problem hiding this comment.
oh, sorry, saw the last arg for tokenAuth inverted too. Maybe we just update getTransport to take another arg?
There was a problem hiding this comment.
On third thoughts, maybe define an enum and make readOnly of that type, then get rid of adminAccess in tokenAuth
type httpAccess int
const (
readOnly httpAccess = iota
readWrite
adminReadOnly
adminReadWrite
)
or something like that
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
riyazdf
force-pushed
the
admin-create-delete
branch
from
eba415e
to
3efab29
Compare
|
Love how this came out with the enum! LGTM! |
Hooks up the handler for deleting a gun to the remote store and client library. Also changes the handler to require the admin
*permission in the auth token.I did some reworking of the transport code to make getting an admin permission more explicit, which should help with making creating a notary repo require the
*permission as well. This PR as-is just defines thegetAdminTransportfunction to be used if one was ever to want to put together a valid request with the*permission, but does not use it directly -- I figured it would be nice to include this, but I'm happy to expand this PR to include CLI commands for deletion, or remove that function for the time being if we'd prefer.Partially addresses #789 and provides groundwork for #710 in the CLI