From 9179528e01ad500327799f28d313b9b07d0779be Mon Sep 17 00:00:00 2001 From: Keith Yao Date: Thu, 5 Mar 2026 22:01:39 +0000 Subject: [PATCH 1/3] Implemented abac support for tag and manifest' --- cmd/acr/annotate.go | 2 +- cmd/acr/cssc.go | 2 +- cmd/acr/manifest.go | 4 +-- cmd/acr/purge.go | 2 +- cmd/acr/tag.go | 4 +-- internal/api/acrsdk.go | 62 +++++++++++++++++++++++++++---------- internal/api/acrsdk_test.go | 2 +- 7 files changed, 53 insertions(+), 25 deletions(-) diff --git a/cmd/acr/annotate.go b/cmd/acr/annotate.go index d934e74a..10c84442 100644 --- a/cmd/acr/annotate.go +++ b/cmd/acr/annotate.go @@ -72,7 +72,7 @@ func newAnnotateCmd(rootParams *rootParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient with authentication is generated, if the authentication cannot be resolved an error is returned. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, annotateParams.username, annotateParams.password, annotateParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, annotateParams.username, annotateParams.password, annotateParams.configs, "") if err != nil { return err } diff --git a/cmd/acr/cssc.go b/cmd/acr/cssc.go index 116e244b..ec0e1dc4 100644 --- a/cmd/acr/cssc.go +++ b/cmd/acr/cssc.go @@ -82,7 +82,7 @@ func newPatchFilterCmd(csscParams *csscParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) resolveRegistryCredentials(csscParams, loginURL) - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, csscParams.username, csscParams.password, csscParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, csscParams.username, csscParams.password, csscParams.configs, "") if err != nil { return err } diff --git a/cmd/acr/manifest.go b/cmd/acr/manifest.go index 05480327..fc89306b 100644 --- a/cmd/acr/manifest.go +++ b/cmd/acr/manifest.go @@ -66,7 +66,7 @@ func newManifestListCmd(manifestParams *manifestParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient is created to make the http requests to the registry. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs, manifestParams.repoName) if err != nil { return err } @@ -122,7 +122,7 @@ func newManifestDeleteCmd(manifestParams *manifestParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs, manifestParams.repoName) if err != nil { return err } diff --git a/cmd/acr/purge.go b/cmd/acr/purge.go index 803739bc..66aa29ec 100644 --- a/cmd/acr/purge.go +++ b/cmd/acr/purge.go @@ -138,7 +138,7 @@ func newPurgeCmd(rootParams *rootParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient with authentication is generated, if the authentication cannot be resolved an error is returned. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, purgeParams.username, purgeParams.password, purgeParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, purgeParams.username, purgeParams.password, purgeParams.configs, "") if err != nil { return err } diff --git a/cmd/acr/tag.go b/cmd/acr/tag.go index ba31fda3..1165aca3 100644 --- a/cmd/acr/tag.go +++ b/cmd/acr/tag.go @@ -66,7 +66,7 @@ func newTagListCmd(tagParams *tagParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient is created to make the http requests to the registry. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs, tagParams.repoName) if err != nil { return err } @@ -99,7 +99,7 @@ func newTagDeleteCmd(tagParams *tagParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs, tagParams.repoName) if err != nil { return err } diff --git a/internal/api/acrsdk.go b/internal/api/acrsdk.go index 30631a90..350a7786 100644 --- a/internal/api/acrsdk.go +++ b/internal/api/acrsdk.go @@ -58,8 +58,11 @@ type AcrCLIClient struct { // This is detected by checking if the refresh token contains the "aad_identity" claim. isAbac bool // currentRepositories holds the repository names for which the current ABAC token has permissions. - // This is used for dynamic token refresh when the token expires during operations. + // This is used for dynamic token refresh when the token expires during operations (purge batching). currentRepositories []string + // repoName stores the target repository for repo-scoped token requests on ABAC registries. + // Used by tag/manifest commands where a single repo is known upfront. + repoName string } // LoginURL returns the FQDN for a registry. @@ -99,16 +102,26 @@ func newAcrCLIClientWithBasicAuth(loginURL string, username string, password str } // newAcrCLIClientWithBearerAuth creates a client that uses bearer token authentication. -// It detects ABAC-enabled registries via the "aad_identity" claim in the refresh token. -// It always requests both catalog and wildcard repository access; on ABAC registries the -// wildcard is silently ignored by the server, so callers must use RefreshTokenForAbac to -// request repository-specific scopes before accessing individual repositories. -func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string) (AcrCLIClient, error) { +// When repoName is provided and the registry uses ABAC, a repo-scoped token is requested +// instead of the wildcard scope (which ABAC registries silently ignore). +func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string, repoName string) (AcrCLIClient, error) { newAcrCLIClient := newAcrCLIClient(loginURL) - newAcrCLIClient.isAbac = hasAadIdentityClaim(refreshToken) - ctx := context.Background() - scope := "registry:catalog:* repository:*:*" + + // Detect ABAC by checking for the aad_identity claim in the refresh token. + isAbac := hasAadIdentityClaim(refreshToken) + newAcrCLIClient.isAbac = isAbac + newAcrCLIClient.repoName = repoName + + // Build token scope based on ABAC status and whether a repo is known. + var scope string + if isAbac && repoName != "" { + // For ABAC registries with a known repo, request repo-scoped permissions. + scope = fmt.Sprintf("repository:%s:pull,delete,metadata_read,metadata_write", repoName) + } else { + // For non-ABAC registries (or when no repo is known yet), use wildcard scope. + scope = "registry:catalog:* repository:*:*" + } accessTokenResponse, err := newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, scope, refreshToken) if err != nil { @@ -129,8 +142,9 @@ func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string) (AcrCLI return newAcrCLIClient, nil } -// GetAcrCLIClientWithAuth obtains a client that has authentication for making ACR http requests -func GetAcrCLIClientWithAuth(loginURL string, username string, password string, configs []string) (*AcrCLIClient, error) { +// GetAcrCLIClientWithAuth obtains a client that has authentication for making ACR http requests. +// repoName is optional; when provided for ABAC registries, the token is scoped to that repository. +func GetAcrCLIClientWithAuth(loginURL string, username string, password string, configs []string, repoName string) (*AcrCLIClient, error) { if username == "" && password == "" { // If both username and password are empty then the docker config file will be used, it can be found in the default // location or in a location specified by the configs string array @@ -158,7 +172,7 @@ func GetAcrCLIClientWithAuth(loginURL string, username string, password string, if username == "" || username == "00000000-0000-0000-0000-000000000000" { // If the username is empty an ACR refresh token was used. var err error - acrClient, err = newAcrCLIClientWithBearerAuth(loginURL, password) + acrClient, err = newAcrCLIClientWithBearerAuth(loginURL, password, repoName) if err != nil { return nil, errors.Wrap(err, "error resolving authentication") } @@ -187,7 +201,8 @@ func buildAbacScope(repositories []string) string { // refreshAcrCLIClientToken obtains a new token and gets its expiration time. // For non-ABAC registries, this uses the wildcard scope. -// For ABAC registries, this uses the currentRepositories to refresh with the appropriate scope. +// For ABAC registries, this uses the currentRepositories (set by purge batching) +// or the stored repoName (set by tag/manifest commands) to refresh with the appropriate scope. func refreshAcrCLIClientToken(ctx context.Context, c *AcrCLIClient, repoName string) error { var scope string if c.isAbac { @@ -196,17 +211,30 @@ func refreshAcrCLIClientToken(ctx context.Context, c *AcrCLIClient, repoName str for _, repo := range c.currentRepositories { repoSet[repo] = true } + // Ensure the current repoName is in the set if repoName != "" { repoSet[repoName] = true } - repos := make([]string, 0, len(repoSet)) + // Fall back to the stored repoName (used by tag/manifest commands) + if len(repoSet) == 0 && c.repoName != "" { + repoSet[c.repoName] = true + } + var scopeParts []string + // "catalog" is a sentinel value meaning "include registry:catalog:* scope" + // (access to the catalog API for listing repositories). It must NOT be + // treated as a repository name, since "repository:catalog:..." would try + // to access a repository literally named "catalog". + if repoSet["catalog"] { + scopeParts = append(scopeParts, "registry:catalog:*") + delete(repoSet, "catalog") + } for repo := range repoSet { - repos = append(repos, repo) + scopeParts = append(scopeParts, fmt.Sprintf("repository:%s:pull,delete,metadata_read,metadata_write", repo)) } - scope = buildAbacScope(repos) - if scope == "" { + if len(scopeParts) == 0 { return errors.New("ABAC registry requires repository scope but none specified") } + scope = strings.Join(scopeParts, " ") } else { // For non-ABAC registries, use the wildcard scope scope = "repository:*:*" diff --git a/internal/api/acrsdk_test.go b/internal/api/acrsdk_test.go index 10b0f272..f86de956 100644 --- a/internal/api/acrsdk_test.go +++ b/internal/api/acrsdk_test.go @@ -212,7 +212,7 @@ func TestGetAcrCLIClientWithAuth(t *testing.T) { t.Errorf("cannot create test config file: %v", err) return } - got, err := GetAcrCLIClientWithAuth(tt.loginURL, tt.username, tt.password, []string{configFilePath}) + got, err := GetAcrCLIClientWithAuth(tt.loginURL, tt.username, tt.password, []string{configFilePath}, "") if (err != nil) != tt.wantErr { t.Errorf("GetAcrCLIClientWithAuth() error = %v, wantErr %v", err, tt.wantErr) return From ca849706d48ac62942d834631861e796a1f54341 Mon Sep 17 00:00:00 2001 From: Keith Yao Date: Tue, 10 Mar 2026 22:32:18 +0000 Subject: [PATCH 2/3] Fixed GetAcrCLICLientWithAtuh and newAcrCLIClientWithBearerAuth --- cmd/acr/annotate.go | 2 +- cmd/acr/cssc.go | 2 +- cmd/acr/manifest.go | 16 ++++++++-- cmd/acr/purge.go | 2 +- cmd/acr/tag.go | 16 ++++++++-- internal/api/acrsdk.go | 63 ++++++++++--------------------------- internal/api/acrsdk_test.go | 2 +- internal/tag/tag.go | 4 ++- 8 files changed, 52 insertions(+), 55 deletions(-) diff --git a/cmd/acr/annotate.go b/cmd/acr/annotate.go index 10c84442..d934e74a 100644 --- a/cmd/acr/annotate.go +++ b/cmd/acr/annotate.go @@ -72,7 +72,7 @@ func newAnnotateCmd(rootParams *rootParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient with authentication is generated, if the authentication cannot be resolved an error is returned. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, annotateParams.username, annotateParams.password, annotateParams.configs, "") + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, annotateParams.username, annotateParams.password, annotateParams.configs) if err != nil { return err } diff --git a/cmd/acr/cssc.go b/cmd/acr/cssc.go index ec0e1dc4..116e244b 100644 --- a/cmd/acr/cssc.go +++ b/cmd/acr/cssc.go @@ -82,7 +82,7 @@ func newPatchFilterCmd(csscParams *csscParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) resolveRegistryCredentials(csscParams, loginURL) - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, csscParams.username, csscParams.password, csscParams.configs, "") + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, csscParams.username, csscParams.password, csscParams.configs) if err != nil { return err } diff --git a/cmd/acr/manifest.go b/cmd/acr/manifest.go index fc89306b..4b855152 100644 --- a/cmd/acr/manifest.go +++ b/cmd/acr/manifest.go @@ -66,10 +66,16 @@ func newManifestListCmd(manifestParams *manifestParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient is created to make the http requests to the registry. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs, manifestParams.repoName) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs) if err != nil { return err } + // For ABAC registries, scope the token to the target repository. + if acrClient.IsAbac() { + if err := acrClient.RefreshTokenForAbac(context.Background(), []string{manifestParams.repoName}); err != nil { + return err + } + } ctx := context.Background() err = listManifests(ctx, acrClient, loginURL, manifestParams.repoName) if err != nil { @@ -122,10 +128,16 @@ func newManifestDeleteCmd(manifestParams *manifestParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs, manifestParams.repoName) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs) if err != nil { return err } + // For ABAC registries, scope the token to the target repository. + if acrClient.IsAbac() { + if err := acrClient.RefreshTokenForAbac(context.Background(), []string{manifestParams.repoName}); err != nil { + return err + } + } ctx := context.Background() err = deleteManifests(ctx, acrClient, loginURL, manifestParams.repoName, args) if err != nil { diff --git a/cmd/acr/purge.go b/cmd/acr/purge.go index 66aa29ec..803739bc 100644 --- a/cmd/acr/purge.go +++ b/cmd/acr/purge.go @@ -138,7 +138,7 @@ func newPurgeCmd(rootParams *rootParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient with authentication is generated, if the authentication cannot be resolved an error is returned. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, purgeParams.username, purgeParams.password, purgeParams.configs, "") + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, purgeParams.username, purgeParams.password, purgeParams.configs) if err != nil { return err } diff --git a/cmd/acr/tag.go b/cmd/acr/tag.go index 1165aca3..cfbfbcdd 100644 --- a/cmd/acr/tag.go +++ b/cmd/acr/tag.go @@ -66,10 +66,16 @@ func newTagListCmd(tagParams *tagParameters) *cobra.Command { } loginURL := api.LoginURL(registryName) // An acrClient is created to make the http requests to the registry. - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs, tagParams.repoName) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs) if err != nil { return err } + // For ABAC registries, scope the token to the target repository. + if acrClient.IsAbac() { + if err := acrClient.RefreshTokenForAbac(context.Background(), []string{tagParams.repoName}); err != nil { + return err + } + } ctx := context.Background() tagList, err := tag.ListTags(ctx, acrClient, tagParams.repoName) if err != nil { @@ -99,10 +105,16 @@ func newTagDeleteCmd(tagParams *tagParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) - acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs, tagParams.repoName) + acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs) if err != nil { return err } + // For ABAC registries, scope the token to the target repository. + if acrClient.IsAbac() { + if err := acrClient.RefreshTokenForAbac(context.Background(), []string{tagParams.repoName}); err != nil { + return err + } + } ctx := context.Background() err = tag.DeleteTags(ctx, acrClient, loginURL, tagParams.repoName, args) if err != nil { diff --git a/internal/api/acrsdk.go b/internal/api/acrsdk.go index 350a7786..e5dc417a 100644 --- a/internal/api/acrsdk.go +++ b/internal/api/acrsdk.go @@ -58,11 +58,8 @@ type AcrCLIClient struct { // This is detected by checking if the refresh token contains the "aad_identity" claim. isAbac bool // currentRepositories holds the repository names for which the current ABAC token has permissions. - // This is used for dynamic token refresh when the token expires during operations (purge batching). + // This is used for dynamic token refresh when the token expires during operations. currentRepositories []string - // repoName stores the target repository for repo-scoped token requests on ABAC registries. - // Used by tag/manifest commands where a single repo is known upfront. - repoName string } // LoginURL returns the FQDN for a registry. @@ -102,26 +99,16 @@ func newAcrCLIClientWithBasicAuth(loginURL string, username string, password str } // newAcrCLIClientWithBearerAuth creates a client that uses bearer token authentication. -// When repoName is provided and the registry uses ABAC, a repo-scoped token is requested -// instead of the wildcard scope (which ABAC registries silently ignore). -func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string, repoName string) (AcrCLIClient, error) { +// It detects ABAC-enabled registries via the "aad_identity" claim in the refresh token. +// It always requests both catalog and wildcard repository access; on ABAC registries the +// wildcard is silently ignored by the server, so callers must use RefreshTokenForAbac to +// request repository-specific scopes before accessing individual repositories. +func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string) (AcrCLIClient, error) { newAcrCLIClient := newAcrCLIClient(loginURL) - ctx := context.Background() - - // Detect ABAC by checking for the aad_identity claim in the refresh token. - isAbac := hasAadIdentityClaim(refreshToken) - newAcrCLIClient.isAbac = isAbac - newAcrCLIClient.repoName = repoName + newAcrCLIClient.isAbac = hasAadIdentityClaim(refreshToken) - // Build token scope based on ABAC status and whether a repo is known. - var scope string - if isAbac && repoName != "" { - // For ABAC registries with a known repo, request repo-scoped permissions. - scope = fmt.Sprintf("repository:%s:pull,delete,metadata_read,metadata_write", repoName) - } else { - // For non-ABAC registries (or when no repo is known yet), use wildcard scope. - scope = "registry:catalog:* repository:*:*" - } + ctx := context.Background() + scope := "registry:catalog:* repository:*:*" accessTokenResponse, err := newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, scope, refreshToken) if err != nil { @@ -142,9 +129,8 @@ func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string, repoNam return newAcrCLIClient, nil } -// GetAcrCLIClientWithAuth obtains a client that has authentication for making ACR http requests. -// repoName is optional; when provided for ABAC registries, the token is scoped to that repository. -func GetAcrCLIClientWithAuth(loginURL string, username string, password string, configs []string, repoName string) (*AcrCLIClient, error) { +// GetAcrCLIClientWithAuth obtains a client that has authentication for making ACR http requests +func GetAcrCLIClientWithAuth(loginURL string, username string, password string, configs []string) (*AcrCLIClient, error) { if username == "" && password == "" { // If both username and password are empty then the docker config file will be used, it can be found in the default // location or in a location specified by the configs string array @@ -172,7 +158,7 @@ func GetAcrCLIClientWithAuth(loginURL string, username string, password string, if username == "" || username == "00000000-0000-0000-0000-000000000000" { // If the username is empty an ACR refresh token was used. var err error - acrClient, err = newAcrCLIClientWithBearerAuth(loginURL, password, repoName) + acrClient, err = newAcrCLIClientWithBearerAuth(loginURL, password) if err != nil { return nil, errors.Wrap(err, "error resolving authentication") } @@ -201,40 +187,25 @@ func buildAbacScope(repositories []string) string { // refreshAcrCLIClientToken obtains a new token and gets its expiration time. // For non-ABAC registries, this uses the wildcard scope. -// For ABAC registries, this uses the currentRepositories (set by purge batching) -// or the stored repoName (set by tag/manifest commands) to refresh with the appropriate scope. +// For ABAC registries, this uses the currentRepositories to refresh with the appropriate scope. func refreshAcrCLIClientToken(ctx context.Context, c *AcrCLIClient, repoName string) error { var scope string if c.isAbac { - // For ABAC registries, build scope from currentRepositories and ensure repoName is included repoSet := make(map[string]bool) for _, repo := range c.currentRepositories { repoSet[repo] = true } - // Ensure the current repoName is in the set if repoName != "" { repoSet[repoName] = true } - // Fall back to the stored repoName (used by tag/manifest commands) - if len(repoSet) == 0 && c.repoName != "" { - repoSet[c.repoName] = true - } - var scopeParts []string - // "catalog" is a sentinel value meaning "include registry:catalog:* scope" - // (access to the catalog API for listing repositories). It must NOT be - // treated as a repository name, since "repository:catalog:..." would try - // to access a repository literally named "catalog". - if repoSet["catalog"] { - scopeParts = append(scopeParts, "registry:catalog:*") - delete(repoSet, "catalog") - } + repos := make([]string, 0, len(repoSet)) for repo := range repoSet { - scopeParts = append(scopeParts, fmt.Sprintf("repository:%s:pull,delete,metadata_read,metadata_write", repo)) + repos = append(repos, repo) } - if len(scopeParts) == 0 { + scope = buildAbacScope(repos) + if scope == "" { return errors.New("ABAC registry requires repository scope but none specified") } - scope = strings.Join(scopeParts, " ") } else { // For non-ABAC registries, use the wildcard scope scope = "repository:*:*" diff --git a/internal/api/acrsdk_test.go b/internal/api/acrsdk_test.go index f86de956..10b0f272 100644 --- a/internal/api/acrsdk_test.go +++ b/internal/api/acrsdk_test.go @@ -212,7 +212,7 @@ func TestGetAcrCLIClientWithAuth(t *testing.T) { t.Errorf("cannot create test config file: %v", err) return } - got, err := GetAcrCLIClientWithAuth(tt.loginURL, tt.username, tt.password, []string{configFilePath}, "") + got, err := GetAcrCLIClientWithAuth(tt.loginURL, tt.username, tt.password, []string{configFilePath}) if (err != nil) != tt.wantErr { t.Errorf("GetAcrCLIClientWithAuth() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/internal/tag/tag.go b/internal/tag/tag.go index c1876d7b..1325372c 100644 --- a/internal/tag/tag.go +++ b/internal/tag/tag.go @@ -32,7 +32,9 @@ func ListTags(ctx context.Context, acrClient api.AcrCLIClientInterface, repoName } var tagList []acr.TagAttributesBase - tagList = append(tagList, *resultTags.TagsAttributes...) + if resultTags.TagsAttributes != nil { + tagList = append(tagList, *resultTags.TagsAttributes...) + } // A for loop is used because the GetAcrTags method returns by default only 100 tags and their attributes. for resultTags != nil && resultTags.TagsAttributes != nil { From 869a67e8f06f548c22f34bdc275577f77b0048b7 Mon Sep 17 00:00:00 2001 From: Keith Yao Date: Wed, 11 Mar 2026 17:49:10 +0000 Subject: [PATCH 3/3] Fixed bugs and added tests --- cmd/acr/manifest.go | 8 +-- cmd/acr/manifest_test.go | 108 +++++++++++++++++++++++++++++++++++++ cmd/acr/tag.go | 8 +-- cmd/acr/tag_test.go | 113 +++++++++++++++++++++++++++++++++++++++ internal/api/acrsdk.go | 1 + 5 files changed, 230 insertions(+), 8 deletions(-) diff --git a/cmd/acr/manifest.go b/cmd/acr/manifest.go index 4b855152..5a3d2813 100644 --- a/cmd/acr/manifest.go +++ b/cmd/acr/manifest.go @@ -65,6 +65,7 @@ func newManifestListCmd(manifestParams *manifestParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) + ctx := context.Background() // An acrClient is created to make the http requests to the registry. acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs) if err != nil { @@ -72,11 +73,10 @@ func newManifestListCmd(manifestParams *manifestParameters) *cobra.Command { } // For ABAC registries, scope the token to the target repository. if acrClient.IsAbac() { - if err := acrClient.RefreshTokenForAbac(context.Background(), []string{manifestParams.repoName}); err != nil { + if err := acrClient.RefreshTokenForAbac(ctx, []string{manifestParams.repoName}); err != nil { return err } } - ctx := context.Background() err = listManifests(ctx, acrClient, loginURL, manifestParams.repoName) if err != nil { return err @@ -128,17 +128,17 @@ func newManifestDeleteCmd(manifestParams *manifestParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) + ctx := context.Background() acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, manifestParams.username, manifestParams.password, manifestParams.configs) if err != nil { return err } // For ABAC registries, scope the token to the target repository. if acrClient.IsAbac() { - if err := acrClient.RefreshTokenForAbac(context.Background(), []string{manifestParams.repoName}); err != nil { + if err := acrClient.RefreshTokenForAbac(ctx, []string{manifestParams.repoName}); err != nil { return err } } - ctx := context.Background() err = deleteManifests(ctx, acrClient, loginURL, manifestParams.repoName, args) if err != nil { return err diff --git a/cmd/acr/manifest_test.go b/cmd/acr/manifest_test.go index a21a8369..b63e06aa 100644 --- a/cmd/acr/manifest_test.go +++ b/cmd/acr/manifest_test.go @@ -9,6 +9,7 @@ import ( "github.com/Azure/acr-cli/cmd/mocks" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func TestListManifests(t *testing.T) { @@ -67,3 +68,110 @@ func TestDeleteManifests(t *testing.T) { mockClient.AssertExpectations(t) }) } + +// TestListManifestsAbac tests the manifest list ABAC code path: after client creation, +// if IsAbac() returns true, RefreshTokenForAbac must be called before listing manifests. +func TestListManifestsAbac(t *testing.T) { + t.Run("AbacEnabledListManifestsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(nil).Once() + mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleManifestV2WithTagsResult, nil).Once() + mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:2830cc0fcddc1bc2bd4aeab0ed5ee7087dab29a49e65151c77553e46a7ed5283").Return(EmptyListManifestsResult, nil).Once() + // Simulate the ABAC code path from the manifest list command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + err := listManifests(testCtx, mockClient, testLoginURL, testRepo) + assert.Equal(nil, err, "Error should be nil") + mockClient.AssertExpectations(t) + }) + + t.Run("AbacRefreshFailureListManifestsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(errors.New("failed to refresh token for ABAC repositories")).Once() + // Simulate the ABAC code path from the manifest list command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.NotEqual(nil, err, "RefreshTokenForAbac should return an error") + } + // GetAcrManifests should NOT be called since ABAC refresh failed + mockClient.AssertExpectations(t) + }) + + t.Run("NonAbacListManifestsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(false) + mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleManifestV2WithTagsResult, nil).Once() + mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:2830cc0fcddc1bc2bd4aeab0ed5ee7087dab29a49e65151c77553e46a7ed5283").Return(EmptyListManifestsResult, nil).Once() + // Simulate the ABAC code path from the manifest list command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + err := listManifests(testCtx, mockClient, testLoginURL, testRepo) + assert.Equal(nil, err, "Error should be nil") + // RefreshTokenForAbac should NOT have been called + mockClient.AssertNotCalled(t, "RefreshTokenForAbac", mock.Anything, mock.Anything) + mockClient.AssertExpectations(t) + }) +} + +// TestDeleteManifestsAbac tests the manifest delete ABAC code path: after client creation, +// if IsAbac() returns true, RefreshTokenForAbac must be called before deleting manifests. +func TestDeleteManifestsAbac(t *testing.T) { + args := []string{"sha:123", "sha:124"} + + t.Run("AbacEnabledDeleteManifestsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(nil).Once() + mockClient.On("DeleteManifest", mock.Anything, testRepo, "sha:123").Return(&deletedResponse, nil).Once() + mockClient.On("DeleteManifest", mock.Anything, testRepo, "sha:124").Return(&deletedResponse, nil).Once() + // Simulate the ABAC code path from the manifest delete command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + err := deleteManifests(testCtx, mockClient, testLoginURL, testRepo, args) + assert.Equal(nil, err, "Error should be nil") + mockClient.AssertExpectations(t) + }) + + t.Run("AbacRefreshFailureDeleteManifestsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(errors.New("failed to refresh token for ABAC repositories")).Once() + // Simulate the ABAC code path from the manifest delete command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.NotEqual(nil, err, "RefreshTokenForAbac should return an error") + } + // DeleteManifest should NOT be called since ABAC refresh failed + mockClient.AssertExpectations(t) + }) + + t.Run("NonAbacDeleteManifestsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(false) + mockClient.On("DeleteManifest", mock.Anything, testRepo, "sha:123").Return(&deletedResponse, nil).Once() + mockClient.On("DeleteManifest", mock.Anything, testRepo, "sha:124").Return(&deletedResponse, nil).Once() + // Simulate the ABAC code path from the manifest delete command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + err := deleteManifests(testCtx, mockClient, testLoginURL, testRepo, args) + assert.Equal(nil, err, "Error should be nil") + mockClient.AssertNotCalled(t, "RefreshTokenForAbac", mock.Anything, mock.Anything) + mockClient.AssertExpectations(t) + }) +} diff --git a/cmd/acr/tag.go b/cmd/acr/tag.go index cfbfbcdd..93e6b6a5 100644 --- a/cmd/acr/tag.go +++ b/cmd/acr/tag.go @@ -65,6 +65,7 @@ func newTagListCmd(tagParams *tagParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) + ctx := context.Background() // An acrClient is created to make the http requests to the registry. acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs) if err != nil { @@ -72,11 +73,10 @@ func newTagListCmd(tagParams *tagParameters) *cobra.Command { } // For ABAC registries, scope the token to the target repository. if acrClient.IsAbac() { - if err := acrClient.RefreshTokenForAbac(context.Background(), []string{tagParams.repoName}); err != nil { + if err := acrClient.RefreshTokenForAbac(ctx, []string{tagParams.repoName}); err != nil { return err } } - ctx := context.Background() tagList, err := tag.ListTags(ctx, acrClient, tagParams.repoName) if err != nil { return err @@ -105,17 +105,17 @@ func newTagDeleteCmd(tagParams *tagParameters) *cobra.Command { return err } loginURL := api.LoginURL(registryName) + ctx := context.Background() acrClient, err := api.GetAcrCLIClientWithAuth(loginURL, tagParams.username, tagParams.password, tagParams.configs) if err != nil { return err } // For ABAC registries, scope the token to the target repository. if acrClient.IsAbac() { - if err := acrClient.RefreshTokenForAbac(context.Background(), []string{tagParams.repoName}); err != nil { + if err := acrClient.RefreshTokenForAbac(ctx, []string{tagParams.repoName}); err != nil { return err } } - ctx := context.Background() err = tag.DeleteTags(ctx, acrClient, loginURL, tagParams.repoName, args) if err != nil { return err diff --git a/cmd/acr/tag_test.go b/cmd/acr/tag_test.go index dea1874e..7de04247 100644 --- a/cmd/acr/tag_test.go +++ b/cmd/acr/tag_test.go @@ -4,9 +4,13 @@ package main import ( + "errors" "testing" + "github.com/Azure/acr-cli/cmd/mocks" + "github.com/Azure/acr-cli/internal/tag" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func TestNewTagCmd(t *testing.T) { @@ -34,3 +38,112 @@ func TestNewTagDeleteCmd(t *testing.T) { assert.Equal(t, "delete", cmd.Use) assert.Equal(t, newTagDeleteCmdLongMessage, cmd.Long) } + +// TestListTagsAbac tests the tag list ABAC code path: after client creation, +// if IsAbac() returns true, RefreshTokenForAbac must be called before listing tags. +func TestListTagsAbac(t *testing.T) { + t.Run("AbacEnabledListTagsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(nil).Once() + mockClient.On("GetAcrTags", mock.Anything, testRepo, "", "").Return(OneTagResult, nil).Once() + mockClient.On("GetAcrTags", mock.Anything, testRepo, "", "latest").Return(EmptyListTagsResult, nil).Once() + // Simulate the ABAC code path from the tag list command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + tagList, err := tag.ListTags(testCtx, mockClient, testRepo) + assert.Equal(nil, err, "Error should be nil") + assert.Equal(1, len(tagList), "Tag list should have 1 tag") + mockClient.AssertExpectations(t) + }) + + t.Run("AbacRefreshFailureListTagsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(errors.New("failed to refresh token for ABAC repositories")).Once() + // Simulate the ABAC code path from the tag list command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.NotEqual(nil, err, "RefreshTokenForAbac should return an error") + } + // GetAcrTags should NOT be called since ABAC refresh failed + mockClient.AssertExpectations(t) + }) + + t.Run("NonAbacListTagsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(false) + mockClient.On("GetAcrTags", mock.Anything, testRepo, "", "").Return(OneTagResult, nil).Once() + mockClient.On("GetAcrTags", mock.Anything, testRepo, "", "latest").Return(EmptyListTagsResult, nil).Once() + // Simulate the ABAC code path from the tag list command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + tagList, err := tag.ListTags(testCtx, mockClient, testRepo) + assert.Equal(nil, err, "Error should be nil") + assert.Equal(1, len(tagList), "Tag list should have 1 tag") + // RefreshTokenForAbac should NOT have been called + mockClient.AssertNotCalled(t, "RefreshTokenForAbac", mock.Anything, mock.Anything) + mockClient.AssertExpectations(t) + }) +} + +// TestDeleteTagsAbac tests the tag delete ABAC code path: after client creation, +// if IsAbac() returns true, RefreshTokenForAbac must be called before deleting tags. +func TestDeleteTagsAbac(t *testing.T) { + args := []string{"latest", "v1"} + + t.Run("AbacEnabledDeleteTagsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(nil).Once() + mockClient.On("DeleteAcrTag", mock.Anything, testRepo, "latest").Return(&deletedResponse, nil).Once() + mockClient.On("DeleteAcrTag", mock.Anything, testRepo, "v1").Return(&deletedResponse, nil).Once() + // Simulate the ABAC code path from the tag delete command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + err := tag.DeleteTags(testCtx, mockClient, testLoginURL, testRepo, args) + assert.Equal(nil, err, "Error should be nil") + mockClient.AssertExpectations(t) + }) + + t.Run("AbacRefreshFailureDeleteTagsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(true) + mockClient.On("RefreshTokenForAbac", mock.Anything, []string{testRepo}).Return(errors.New("failed to refresh token for ABAC repositories")).Once() + // Simulate the ABAC code path from the tag delete command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.NotEqual(nil, err, "RefreshTokenForAbac should return an error") + } + // DeleteAcrTag should NOT be called since ABAC refresh failed + mockClient.AssertExpectations(t) + }) + + t.Run("NonAbacDeleteTagsTest", func(t *testing.T) { + assert := assert.New(t) + mockClient := &mocks.AcrCLIClientInterface{} + mockClient.On("IsAbac").Return(false) + mockClient.On("DeleteAcrTag", mock.Anything, testRepo, "latest").Return(&deletedResponse, nil).Once() + mockClient.On("DeleteAcrTag", mock.Anything, testRepo, "v1").Return(&deletedResponse, nil).Once() + // Simulate the ABAC code path from the tag delete command + if mockClient.IsAbac() { + err := mockClient.RefreshTokenForAbac(testCtx, []string{testRepo}) + assert.Equal(nil, err, "RefreshTokenForAbac should not return an error") + } + err := tag.DeleteTags(testCtx, mockClient, testLoginURL, testRepo, args) + assert.Equal(nil, err, "Error should be nil") + mockClient.AssertNotCalled(t, "RefreshTokenForAbac", mock.Anything, mock.Anything) + mockClient.AssertExpectations(t) + }) +} diff --git a/internal/api/acrsdk.go b/internal/api/acrsdk.go index e5dc417a..30631a90 100644 --- a/internal/api/acrsdk.go +++ b/internal/api/acrsdk.go @@ -191,6 +191,7 @@ func buildAbacScope(repositories []string) string { func refreshAcrCLIClientToken(ctx context.Context, c *AcrCLIClient, repoName string) error { var scope string if c.isAbac { + // For ABAC registries, build scope from currentRepositories and ensure repoName is included repoSet := make(map[string]bool) for _, repo := range c.currentRepositories { repoSet[repo] = true