diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index ae33d9ce3..a3e627852 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -48,8 +48,7 @@ type httpClient interface { } type Client interface { - GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) - GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) + GetByDigest(params FetchParams) ([]*Attestation, error) GetTrustDomain() (string, error) } @@ -69,16 +68,28 @@ func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClien } } -// GetByRepoAndDigest fetches the attestation by repo and digest -func (c *LiveClient) GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) { - url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) - return c.getByURL(url, params) -} - -// GetByOwnerAndDigest fetches attestation by owner and digest -func (c *LiveClient) GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) { - url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) - return c.getByURL(url, params) +// GetByDigest fetches the attestation by digest and either owner or repo +// depending on which is provided +func (c *LiveClient) GetByDigest(params FetchParams) ([]*Attestation, error) { + if params.Repo == "" && params.Owner == "" { + return nil, fmt.Errorf("owner or repo must be provided") + } else if params.Repo != "" { + // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. + // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. + url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, params.Repo, params.Digest) + attestations, err := c.getByURL(url, params) + if err != nil { + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) + } + return attestations, nil + } else { + url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, params.Owner, params.Digest) + attestations, err := c.getByURL(url, params) + if err != nil { + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) + } + return attestations, nil + } } func (c *LiveClient) getByURL(url string, params FetchParams) ([]*Attestation, error) { diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index f77b39d08..fb2e36b4d 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -51,7 +51,7 @@ var testFetchParams = FetchParams{ func TestGetByDigest(t *testing.T) { c := NewClientWithMockGHClient(false) testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 5, len(attestations)) @@ -59,7 +59,7 @@ func TestGetByDigest(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 5, len(attestations)) @@ -74,7 +74,7 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { // The method should return five results when the limit is not set testFetchParams.Limit = limit testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, 3, len(attestations)) @@ -82,7 +82,7 @@ func TestGetByDigestGreaterThanLimit(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -94,7 +94,7 @@ func TestGetByDigestWithNextPage(t *testing.T) { c := NewClientWithMockGHClient(true) testFetchParams.Repo = testRepo testFetchParams.Limit = 30 - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), 10) @@ -102,7 +102,7 @@ func TestGetByDigestWithNextPage(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), 10) @@ -117,7 +117,7 @@ func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { // The method should return five results when the limit is not set testFetchParams.Limit = limit testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -125,7 +125,7 @@ func TestGetByDigestGreaterThanLimitWithNextPage(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) require.Equal(t, len(attestations), limit) @@ -148,13 +148,13 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) { } testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.Error(t, err) require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) @@ -173,12 +173,12 @@ func TestGetByDigest_Error(t *testing.T) { } testFetchParams.Repo = testRepo - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.Error(t, err) require.Nil(t, attestations) testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.Error(t, err) require.Nil(t, attestations) } @@ -385,7 +385,7 @@ func TestGetAttestationsRetries(t *testing.T) { testFetchParams.Repo = testRepo testFetchParams.Limit = 30 - attestations, err := c.GetByRepoAndDigest(testFetchParams) + attestations, err := c.GetByDigest(testFetchParams) require.NoError(t, err) // assert the error path was executed; because this is a paged @@ -397,9 +397,9 @@ func TestGetAttestationsRetries(t *testing.T) { bundle := (attestations)[0].Bundle require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") - // same test as above, but for GetByOwnerAndDigest: + // same test as above, but for GetByDigest: testFetchParams.Owner = testOwner - attestations, err = c.GetByOwnerAndDigest(testFetchParams) + attestations, err = c.GetByDigest(testFetchParams) require.NoError(t, err) // because we haven't reset the mock, we have added 2 more failed requests @@ -426,7 +426,7 @@ func TestGetAttestationsMaxRetries(t *testing.T) { } testFetchParams.Repo = testRepo - _, err := c.GetByRepoAndDigest(testFetchParams) + _, err := c.GetByDigest(testFetchParams) require.Error(t, err) fetcher.AssertNumberOfCalls(t, "OnREST500Error", 4) diff --git a/pkg/cmd/attestation/api/mock_client.go b/pkg/cmd/attestation/api/mock_client.go index be2b9b76d..efcedc8b5 100644 --- a/pkg/cmd/attestation/api/mock_client.go +++ b/pkg/cmd/attestation/api/mock_client.go @@ -6,58 +6,44 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/test/data" ) +func makeTestAttestation() Attestation { + return Attestation{Bundle: data.SigstoreBundle(nil), BundleURL: "https://example.com"} +} + type MockClient struct { - OnGetByRepoAndDigest func(params FetchParams) ([]*Attestation, error) - OnGetByOwnerAndDigest func(params FetchParams) ([]*Attestation, error) - OnGetTrustDomain func() (string, error) + OnGetByDigest func(params FetchParams) ([]*Attestation, error) + OnGetTrustDomain func() (string, error) } -func (m MockClient) GetByRepoAndDigest(params FetchParams) ([]*Attestation, error) { - return m.OnGetByRepoAndDigest(params) -} - -func (m MockClient) GetByOwnerAndDigest(params FetchParams) ([]*Attestation, error) { - return m.OnGetByOwnerAndDigest(params) +func (m MockClient) GetByDigest(params FetchParams) ([]*Attestation, error) { + return m.OnGetByDigest(params) } func (m MockClient) GetTrustDomain() (string, error) { return m.OnGetTrustDomain() } -func makeTestAttestation() Attestation { - return Attestation{Bundle: data.SigstoreBundle(nil), BundleURL: "https://example.com"} -} - -func OnGetByRepoAndDigestSuccess(params FetchParams) ([]*Attestation, error) { +func OnGetByDigestSuccess(params FetchParams) ([]*Attestation, error) { att1 := makeTestAttestation() att2 := makeTestAttestation() return []*Attestation{&att1, &att2}, nil } -func OnGetByRepoAndDigestFailure(params FetchParams) ([]*Attestation, error) { - return nil, fmt.Errorf("failed to fetch by repo and digest") -} - -func OnGetByOwnerAndDigestSuccess(params FetchParams) ([]*Attestation, error) { - att1 := makeTestAttestation() - att2 := makeTestAttestation() - return []*Attestation{&att1, &att2}, nil -} - -func OnGetByOwnerAndDigestFailure(params FetchParams) ([]*Attestation, error) { +func OnGetByDigestFailure(params FetchParams) ([]*Attestation, error) { + if params.Repo != "" { + return nil, fmt.Errorf("failed to fetch by repo and digest") + } return nil, fmt.Errorf("failed to fetch by owner and digest") } func NewTestClient() *MockClient { return &MockClient{ - OnGetByRepoAndDigest: OnGetByRepoAndDigestSuccess, - OnGetByOwnerAndDigest: OnGetByOwnerAndDigestSuccess, + OnGetByDigest: OnGetByDigestSuccess, } } func NewFailTestClient() *MockClient { return &MockClient{ - OnGetByRepoAndDigest: OnGetByRepoAndDigestFailure, - OnGetByOwnerAndDigest: OnGetByOwnerAndDigestFailure, + OnGetByDigest: OnGetByDigestFailure, } } diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 65b6f83df..2a297bc9a 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -133,7 +133,7 @@ func runDownload(opts *Options) error { Owner: opts.Owner, Repo: opts.Repo, } - attestations, err := verification.GetRemoteAttestations(opts.APIClient, params) + attestations, err := opts.APIClient.GetByDigest(params) if err != nil { if errors.Is(err, api.ErrNoAttestationsFound) { fmt.Fprintf(opts.Logger.IO.Out, "No attestations found for %s\n", opts.ArtifactPath) diff --git a/pkg/cmd/attestation/download/download_test.go b/pkg/cmd/attestation/download/download_test.go index 629de4a66..11872daf9 100644 --- a/pkg/cmd/attestation/download/download_test.go +++ b/pkg/cmd/attestation/download/download_test.go @@ -275,7 +275,7 @@ func TestRunDownload(t *testing.T) { t.Run("no attestations found", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(params api.FetchParams) ([]*api.Attestation, error) { + OnGetByDigest: func(params api.FetchParams) ([]*api.Attestation, error) { return nil, api.ErrNoAttestationsFound }, } @@ -291,7 +291,7 @@ func TestRunDownload(t *testing.T) { t.Run("failed to fetch attestations", func(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ - OnGetByOwnerAndDigest: func(params api.FetchParams) ([]*api.Attestation, error) { + OnGetByDigest: func(params api.FetchParams) ([]*api.Attestation, error) { return nil, fmt.Errorf("failed to fetch attestations") }, } diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 33d8b18b8..9757b72c5 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -82,31 +82,6 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(client api.Client, params api.FetchParams) ([]*api.Attestation, error) { - if client == nil { - return nil, fmt.Errorf("api client must be provided") - } - // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. - // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. - var attestations []*api.Attestation - var err error - var owner string - if params.Repo != "" { - attestations, err = client.GetByRepoAndDigest(params) - owner = params.Repo - } else if params.Owner != "" { - attestations, err = client.GetByOwnerAndDigest(params) - owner = params.Owner - } else { - return nil, fmt.Errorf("owner or repo must be provided") - } - - if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", owner, err) - } - return attestations, err -} - func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { attestations, err := client.GetAttestations(artifact.NameRef(), artifact.DigestWithAlg()) if err != nil { diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go index f956e82b8..f91526bbd 100644 --- a/pkg/cmd/attestation/verify/attestation.go +++ b/pkg/cmd/attestation/verify/attestation.go @@ -29,7 +29,7 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio Repo: o.Repo, } - attestations, err := verification.GetRemoteAttestations(o.APIClient, params) + attestations, err := o.APIClient.GetByDigest(params) if err != nil { msg := "✗ Loading attestations from GitHub API failed" return nil, msg, err