diff --git a/cli/azd/pkg/account/credentials.go b/cli/azd/pkg/account/credentials.go index 6eda579a5ae..a4443b40d27 100644 --- a/cli/azd/pkg/account/credentials.go +++ b/cli/azd/pkg/account/credentials.go @@ -5,11 +5,20 @@ package account import ( "context" + "errors" + "fmt" + "regexp" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/azure/azure-dev/cli/azd/internal" "github.com/azure/azure-dev/cli/azd/pkg/auth" ) +var ( + // Matches AADSTS70043 (refresh token expired due to sign-in frequency) and AADSTS700082 (refresh token expired) + aadRefreshTokenExpiredRegex = regexp.MustCompile(`AADSTS(70043|700082)`) +) + // SubscriptionCredentialProvider provides an [azcore.TokenCredential] configured // to use the tenant id that corresponds to the tenant the given subscription // is located in. @@ -38,8 +47,52 @@ func (p *subscriptionCredentialProvider) CredentialForSubscription( ) (azcore.TokenCredential, error) { tenantId, err := p.subResolver.LookupTenant(ctx, subscriptionId) if err != nil { + // If we can't resolve the tenant for this subscription, it might be because: + // 1. User manually set AZURE_SUBSCRIPTION_ID in .env + // 2. User called `azd env set AZURE_SUBSCRIPTION_ID` instead of selecting from azd's cache + // In these cases, suggest they also set AZURE_TENANT_ID + return nil, fmt.Errorf( + "%w\n\n"+ + "If you manually set the subscription ID (e.g., via AZURE_SUBSCRIPTION_ID in .env or `azd env set`), "+ + "you must also set AZURE_TENANT_ID to the tenant ID that contains this subscription. "+ + "Alternatively, run `azd auth login --tenant-id ` "+ + "to allow azd to discover subscriptions in that tenant.", + err, + ) + } + + cred, err := p.credProvider.GetTokenCredential(ctx, tenantId) + if err != nil { + // If this is an AADSTS refresh token error, enhance it with tenant-specific login guidance + if aadRefreshTokenExpiredRegex.MatchString(err.Error()) { + // Check if the error already has a suggestion (ErrorWithSuggestion from auth layer) + var errWithSuggestion *internal.ErrorWithSuggestion + if errors.As(err, &errWithSuggestion) { + // Enhance the existing suggestion with tenant-specific guidance + enhancedSuggestion := fmt.Sprintf( + "%s To re-authenticate specifically to this tenant, run `azd auth login --tenant-id %s`.", + errWithSuggestion.Suggestion, + tenantId, + ) + return nil, &internal.ErrorWithSuggestion{ + Err: errWithSuggestion.Err, + Suggestion: enhancedSuggestion, + } + } + + // If it's not wrapped yet, create a new ErrorWithSuggestion + return nil, &internal.ErrorWithSuggestion{ + Err: err, + Suggestion: fmt.Sprintf( + "Access to tenant '%s' has expired or requires re-authentication. "+ + "Run `azd auth login --tenant-id %s` to re-authenticate to this tenant.", + tenantId, + tenantId, + ), + } + } return nil, err } - return p.credProvider.GetTokenCredential(ctx, tenantId) + return cred, nil } diff --git a/cli/azd/pkg/account/credentials_test.go b/cli/azd/pkg/account/credentials_test.go index 7ba177eb6dd..3a7bab4f35f 100644 --- a/cli/azd/pkg/account/credentials_test.go +++ b/cli/azd/pkg/account/credentials_test.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/azure/azure-dev/cli/azd/internal" "github.com/stretchr/testify/assert" ) @@ -66,6 +67,79 @@ func TestSubscriptionCredentialProvider(t *testing.T) { }) } +func TestSubscriptionCredentialProvider_AADSTSErrors(t *testing.T) { + t.Parallel() + + tenantId := "fafbff54-b655-4648-98a2-dc3ada4df86e" + subscriptionId := "d0a01878-d7f8-41ce-a4bc-2ead16199965" + + t.Run("AADSTS70043_WithoutExistingSuggestion", func(t *testing.T) { + provider := NewSubscriptionCredentialProvider( + subscriptionTenantResolverFunc(func(ctx context.Context, subId string) (string, error) { + return tenantId, nil + }), + multiTenantCredentialProviderFunc(func(ctx context.Context, tid string) (azcore.TokenCredential, error) { + return nil, errors.New("AADSTS70043: The refresh token has expired") + }), + ) + + _, err := provider.CredentialForSubscription(context.Background(), subscriptionId) + assert.Error(t, err) + + // The error should be wrapped in an ErrorWithSuggestion + var errWithSuggestion *internal.ErrorWithSuggestion + assert.True(t, errors.As(err, &errWithSuggestion), "error should be wrapped in ErrorWithSuggestion") + + // Check that the suggestion includes tenant-specific guidance + assert.Contains(t, errWithSuggestion.Suggestion, tenantId) + assert.Contains(t, errWithSuggestion.Suggestion, "azd auth login --tenant-id") + + // The underlying error should contain AADSTS70043 + assert.Contains(t, errWithSuggestion.Error(), "AADSTS70043") + }) + + t.Run("AADSTS700082_RefreshTokenExpired", func(t *testing.T) { + provider := NewSubscriptionCredentialProvider( + subscriptionTenantResolverFunc(func(ctx context.Context, subId string) (string, error) { + return tenantId, nil + }), + multiTenantCredentialProviderFunc(func(ctx context.Context, tid string) (azcore.TokenCredential, error) { + return nil, errors.New("AADSTS700082: The refresh token has expired") + }), + ) + + _, err := provider.CredentialForSubscription(context.Background(), subscriptionId) + assert.Error(t, err) + + // The error should be wrapped in an ErrorWithSuggestion + var errWithSuggestion *internal.ErrorWithSuggestion + assert.True(t, errors.As(err, &errWithSuggestion), "error should be wrapped in ErrorWithSuggestion") + + // Check that the suggestion includes tenant-specific guidance + assert.Contains(t, errWithSuggestion.Suggestion, tenantId) + assert.Contains(t, errWithSuggestion.Suggestion, "azd auth login --tenant-id") + + // The underlying error should contain AADSTS700082 + assert.Contains(t, errWithSuggestion.Error(), "AADSTS700082") + }) + + t.Run("TenantLookupFailure_EnhancedError", func(t *testing.T) { + provider := NewSubscriptionCredentialProvider( + subscriptionTenantResolverFunc(func(ctx context.Context, subId string) (string, error) { + return "", errors.New("failed to resolve tenant") + }), + multiTenantCredentialProviderFunc(func(ctx context.Context, tid string) (azcore.TokenCredential, error) { + return &dummyCredential{}, nil + }), + ) + + _, err := provider.CredentialForSubscription(context.Background(), subscriptionId) + assert.Error(t, err) + assert.Contains(t, err.Error(), "AZURE_TENANT_ID") + assert.Contains(t, err.Error(), "manually set the subscription ID") + }) +} + // subscriptionTenantResolverFunc implements [SubscriptionTenantResolver] using a provided function. type subscriptionTenantResolverFunc func(ctx context.Context, subscriptionId string) (string, error) diff --git a/cli/azd/pkg/account/manager_test.go b/cli/azd/pkg/account/manager_test.go index ed3015977a9..4c4bfa54aef 100644 --- a/cli/azd/pkg/account/manager_test.go +++ b/cli/azd/pkg/account/manager_test.go @@ -735,6 +735,10 @@ func (b *BypassSubscriptionsCache) Save(ctx context.Context, key string, save [] return nil } +func (b *BypassSubscriptionsCache) Merge(ctx context.Context, key string, save []Subscription) error { + return nil +} + func (b *BypassSubscriptionsCache) Clear(ctx context.Context) error { return nil } diff --git a/cli/azd/pkg/account/subscriptions_cache.go b/cli/azd/pkg/account/subscriptions_cache.go index 2a04eb1ca00..418889b13e3 100644 --- a/cli/azd/pkg/account/subscriptions_cache.go +++ b/cli/azd/pkg/account/subscriptions_cache.go @@ -117,6 +117,70 @@ func (s *subscriptionsCache) Save(ctx context.Context, key string, subscriptions return err } +// Merge merges the given subscriptions with the existing cache for the specified key. +// For each subscription in the new list: +// - If it already exists (by ID), it is updated with the new values +// - If it doesn't exist, it is added to the cache +// +// Subscriptions in the cache that are not present in the new list are preserved. +// This prevents losing tenant-to-subscription mappings when a tenant is temporarily inaccessible. +func (s *subscriptionsCache) Merge(ctx context.Context, key string, subscriptions []Subscription) error { + s.inMemoryLock.Lock() + defer s.inMemoryLock.Unlock() + + // Read the file if it exists + cacheFile, err := os.ReadFile(filepath.Join(s.cacheDir, subscriptionsCacheFile)) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + + // unmarshal cache, ignoring the error if the cache was upgraded or corrupted + cache := map[string][]Subscription{} + if cacheFile != nil { + err = json.Unmarshal(cacheFile, &cache) + if err != nil { + log.Printf("failed to unmarshal %s, ignoring: %v", subscriptionsCacheFile, err) + } + } + + // Get existing subscriptions for this key + existing := cache[key] + + // Build a map of existing subscriptions by ID for quick lookup + existingMap := make(map[string]Subscription, len(existing)) + for _, sub := range existing { + existingMap[sub.Id] = sub + } + + // Update or add new subscriptions + for _, sub := range subscriptions { + existingMap[sub.Id] = sub + } + + // Convert map back to slice + merged := make([]Subscription, 0, len(existingMap)) + for _, sub := range existingMap { + merged = append(merged, sub) + } + + // Apply the merged result + cache[key] = merged + + // save new cache + content, err := json.Marshal(cache) + if err != nil { + return fmt.Errorf("failed to marshal subscriptions: %w", err) + } + + err = os.WriteFile(filepath.Join(s.cacheDir, subscriptionsCacheFile), content, osutil.PermissionFile) + if err != nil { + return fmt.Errorf("failed to write file: %w", err) + } + + s.inMemoryCopy = cache + return err +} + // Clear removes all stored cache items. Returns an error if a filesystem error other than ErrNotExist occurred. func (s *subscriptionsCache) Clear(ctx context.Context) error { s.inMemoryLock.Lock() diff --git a/cli/azd/pkg/account/subscriptions_cache_test.go b/cli/azd/pkg/account/subscriptions_cache_test.go index 985bf97fcb5..e253d2c76ff 100644 --- a/cli/azd/pkg/account/subscriptions_cache_test.go +++ b/cli/azd/pkg/account/subscriptions_cache_test.go @@ -6,6 +6,7 @@ package account import ( "context" "os" + "sort" "testing" "github.com/stretchr/testify/require" @@ -66,3 +67,196 @@ func TestSubscriptionsCache(t *testing.T) { _, err = s.Load(ctx, "key2") require.ErrorIs(t, err, os.ErrNotExist) } + +func TestSubscriptionsCache_Merge(t *testing.T) { + t.Run("MergeIntoEmptyCache", func(t *testing.T) { + dir := t.TempDir() + s := &subscriptionsCache{ + cacheDir: dir, + inMemoryCopy: map[string][]Subscription{}, + } + ctx := context.Background() + + // Merge into empty cache should add all subscriptions + err := s.Merge(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1", TenantId: "tenant1"}, + {Id: "sub2", Name: "Subscription 2", TenantId: "tenant2"}, + }) + require.NoError(t, err) + + load, err := s.Load(ctx, "key1") + require.NoError(t, err) + require.Len(t, load, 2) + + // Sort by ID for consistent comparison + sort.Slice(load, func(i, j int) bool { return load[i].Id < load[j].Id }) + require.Equal(t, "sub1", load[0].Id) + require.Equal(t, "sub2", load[1].Id) + }) + + t.Run("MergeUpdatesExistingSubscriptions", func(t *testing.T) { + dir := t.TempDir() + s := &subscriptionsCache{ + cacheDir: dir, + inMemoryCopy: map[string][]Subscription{}, + } + ctx := context.Background() + + // Initial cache state + err := s.Save(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1 Old", TenantId: "tenant1"}, + {Id: "sub2", Name: "Subscription 2 Old", TenantId: "tenant2"}, + }) + require.NoError(t, err) + + // Merge with updated subscription + err = s.Merge(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1 New", TenantId: "tenant1"}, + }) + require.NoError(t, err) + + load, err := s.Load(ctx, "key1") + require.NoError(t, err) + require.Len(t, load, 2) + + // Sort by ID for consistent comparison + sort.Slice(load, func(i, j int) bool { return load[i].Id < load[j].Id }) + require.Equal(t, "sub1", load[0].Id) + require.Equal(t, "Subscription 1 New", load[0].Name) + require.Equal(t, "sub2", load[1].Id) + require.Equal(t, "Subscription 2 Old", load[1].Name) + }) + + t.Run("MergePreservesUnchangedSubscriptions", func(t *testing.T) { + dir := t.TempDir() + s := &subscriptionsCache{ + cacheDir: dir, + inMemoryCopy: map[string][]Subscription{}, + } + ctx := context.Background() + + // Initial cache with subscriptions from two tenants + err := s.Save(ctx, "key1", []Subscription{ + {Id: "subA", Name: "Subscription A", TenantId: "tenant1", UserAccessTenantId: "tenant1"}, + {Id: "subB", Name: "Subscription B", TenantId: "tenant2", UserAccessTenantId: "tenant2"}, + }) + require.NoError(t, err) + + // Merge with only tenant1's subscription (simulating tenant2 being temporarily inaccessible) + err = s.Merge(ctx, "key1", []Subscription{ + {Id: "subA", Name: "Subscription A Updated", TenantId: "tenant1", UserAccessTenantId: "tenant1"}, + }) + require.NoError(t, err) + + load, err := s.Load(ctx, "key1") + require.NoError(t, err) + require.Len(t, load, 2, "Both subscriptions should be preserved") + + // Sort by ID for consistent comparison + sort.Slice(load, func(i, j int) bool { return load[i].Id < load[j].Id }) + + // subA should be updated + require.Equal(t, "subA", load[0].Id) + require.Equal(t, "Subscription A Updated", load[0].Name) + + // subB should be preserved with original values + require.Equal(t, "subB", load[1].Id) + require.Equal(t, "Subscription B", load[1].Name) + require.Equal(t, "tenant2", load[1].TenantId) + require.Equal(t, "tenant2", load[1].UserAccessTenantId) + }) + + t.Run("MergeAddsNewSubscriptions", func(t *testing.T) { + dir := t.TempDir() + s := &subscriptionsCache{ + cacheDir: dir, + inMemoryCopy: map[string][]Subscription{}, + } + ctx := context.Background() + + // Initial cache with one subscription + err := s.Save(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1", TenantId: "tenant1"}, + }) + require.NoError(t, err) + + // Merge with two subscriptions (one existing, one new) + err = s.Merge(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1", TenantId: "tenant1"}, + {Id: "sub3", Name: "Subscription 3", TenantId: "tenant3"}, + }) + require.NoError(t, err) + + load, err := s.Load(ctx, "key1") + require.NoError(t, err) + require.Len(t, load, 2) + + // Sort by ID for consistent comparison + sort.Slice(load, func(i, j int) bool { return load[i].Id < load[j].Id }) + require.Equal(t, "sub1", load[0].Id) + require.Equal(t, "sub3", load[1].Id) + }) + + t.Run("MergeWithEmptyList", func(t *testing.T) { + dir := t.TempDir() + s := &subscriptionsCache{ + cacheDir: dir, + inMemoryCopy: map[string][]Subscription{}, + } + ctx := context.Background() + + // Initial cache with subscriptions + err := s.Save(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1", TenantId: "tenant1"}, + {Id: "sub2", Name: "Subscription 2", TenantId: "tenant2"}, + }) + require.NoError(t, err) + + // Merge with empty list (simulating all tenants being temporarily inaccessible) + err = s.Merge(ctx, "key1", []Subscription{}) + require.NoError(t, err) + + // Existing subscriptions should be preserved + load, err := s.Load(ctx, "key1") + require.NoError(t, err) + require.Len(t, load, 2, "Existing subscriptions should be preserved when merging empty list") + }) + + t.Run("MergeMultipleKeys", func(t *testing.T) { + dir := t.TempDir() + s := &subscriptionsCache{ + cacheDir: dir, + inMemoryCopy: map[string][]Subscription{}, + } + ctx := context.Background() + + // Save subscriptions for key1 + err := s.Save(ctx, "key1", []Subscription{ + {Id: "sub1", Name: "Subscription 1", TenantId: "tenant1"}, + }) + require.NoError(t, err) + + // Save subscriptions for key2 + err = s.Save(ctx, "key2", []Subscription{ + {Id: "sub2", Name: "Subscription 2", TenantId: "tenant2"}, + }) + require.NoError(t, err) + + // Merge into key1 shouldn't affect key2 + err = s.Merge(ctx, "key1", []Subscription{ + {Id: "sub3", Name: "Subscription 3", TenantId: "tenant3"}, + }) + require.NoError(t, err) + + // key1 should have both subscriptions + load, err := s.Load(ctx, "key1") + require.NoError(t, err) + require.Len(t, load, 2) + + // key2 should remain unchanged + load, err = s.Load(ctx, "key2") + require.NoError(t, err) + require.Len(t, load, 1) + require.Equal(t, "sub2", load[0].Id) + }) +} diff --git a/cli/azd/pkg/account/subscriptions_manager.go b/cli/azd/pkg/account/subscriptions_manager.go index 01fb12eac3d..25d084a23a3 100644 --- a/cli/azd/pkg/account/subscriptions_manager.go +++ b/cli/azd/pkg/account/subscriptions_manager.go @@ -37,6 +37,7 @@ type principalInfoProvider interface { type subCache interface { Load(ctx context.Context, key string) ([]Subscription, error) Save(ctx context.Context, key string, save []Subscription) error + Merge(ctx context.Context, key string, save []Subscription) error Clear(ctx context.Context) error } @@ -80,6 +81,7 @@ func (m *SubscriptionsManager) ClearSubscriptions(ctx context.Context) error { } // Updates stored cached subscriptions. +// Uses merge semantics to preserve tenant-to-subscription mappings for tenants that are temporarily inaccessible. func (m *SubscriptionsManager) RefreshSubscriptions(ctx context.Context) error { claims, err := m.principalInfo.ClaimsForCurrentUser(ctx, nil) if err != nil { @@ -91,7 +93,7 @@ func (m *SubscriptionsManager) RefreshSubscriptions(ctx context.Context) error { return fmt.Errorf("fetching subscriptions: %w", err) } - err = m.cache.Save(ctx, uid, subs) + err = m.cache.Merge(ctx, uid, subs) if err != nil { return fmt.Errorf("storing subscriptions: %w", err) } @@ -131,8 +133,9 @@ func (m *SubscriptionsManager) LookupTenant(ctx context.Context, subscriptionId return "", fmt.Errorf( "failed to resolve user '%s' access to subscription with ID '%s'. "+ "If you recently gained access to this subscription, run `azd auth login` again to reload subscriptions.\n"+ - "Otherwise, visit this subscription in Azure Portal using the browser, "+ - "then run `azd auth login` ", + "If you have lost access to a tenant containing this subscription, "+ + "you may need to run `azd auth login --tenant-id ` to re-authenticate to that specific tenant. "+ + "Otherwise, visit this subscription in Azure Portal using the browser, then run `azd auth login`.", res.userClaims.DisplayUsername(), subscriptionId) }