Skip to content

Commit d690e53

Browse files
committed
Fix PR comments.
Improve error handling for AAD app config checks Switch to fail-closed logic in client app validation methods to ensure users are prompted on errors, preventing silent misconfiguration. Use GraphApiService with correct permissions for service principal propagation checks to avoid 403s and unnecessary retries. Update log messages to clarify intentional fail-closed behavior for safer, more robust error handling.
1 parent 8b62981 commit d690e53

2 files changed

Lines changed: 18 additions & 10 deletions

File tree

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1060,12 +1060,16 @@ public static async Task<bool> EnsureDelegatedConsentWithRetriesAsync(
10601060
var spPropagated = await retryHelper.ExecuteWithRetryAsync(
10611061
async ct =>
10621062
{
1063-
// Probe oauth2PermissionGrants directly — a 200 (even empty list) confirms
1064-
// the SP's clientId is visible to the grants API replication layer.
1065-
// GET /servicePrincipals resolves too fast and gives false confidence.
1066-
using var checkResp = await httpClient.GetAsync(
1067-
$"{Constants.GraphApiConstants.BaseUrl}/v1.0/oauth2PermissionGrants?$filter=clientId eq '{servicePrincipalId}'", ct);
1068-
return checkResp.IsSuccessStatusCode;
1063+
// Probe oauth2PermissionGrants via GraphApiService (which acquires a token
1064+
// with DelegatedPermissionGrant.ReadWrite.All scope). A non-null response —
1065+
// even an empty list — confirms the SP's clientId is visible to the grants
1066+
// API replication layer. Using the raw httpClient here (Application.ReadWrite.All
1067+
// scope only) caused 403s on every probe, wasting 8+ minutes of retries.
1068+
using var checkDoc = await graphApiService.GraphGetAsync(
1069+
setupConfig.TenantId!,
1070+
$"/v1.0/oauth2PermissionGrants?$filter=clientId eq '{servicePrincipalId}'",
1071+
ct);
1072+
return checkDoc != null;
10691073
},
10701074
result => !result,
10711075
maxRetries: 12,

src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -659,8 +659,10 @@ private async Task<List<string>> CollectMissingRedirectUrisAsync(
659659
}
660660
catch (Exception ex)
661661
{
662-
_logger.LogDebug("CollectMissingRedirectUrisAsync failed (non-fatal): {Message}", ex.Message);
663-
return new List<string>();
662+
// On error, assume all redirect URIs are missing so the prompt still appears.
663+
// Failing closed (prompt) is safer than failing open (silent mutation without disclosure).
664+
_logger.LogDebug("CollectMissingRedirectUrisAsync failed — assuming all redirect URIs missing: {Message}", ex.Message);
665+
return AuthenticationConstants.GetRequiredRedirectUris(clientAppId).ToList();
664666
}
665667
}
666668

@@ -689,8 +691,10 @@ private async Task<bool> IsPublicClientFlowsDisabledAsync(
689691
}
690692
catch (Exception ex)
691693
{
692-
_logger.LogDebug("IsPublicClientFlowsDisabledAsync failed (non-fatal): {Message}", ex.Message);
693-
return false;
694+
// On error, assume public client flows need enabling so the prompt still appears.
695+
// Failing closed (prompt) is safer than failing open (silent mutation without disclosure).
696+
_logger.LogDebug("IsPublicClientFlowsDisabledAsync failed — assuming public client flows need enabling: {Message}", ex.Message);
697+
return true;
694698
}
695699
}
696700

0 commit comments

Comments
 (0)