-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Sign in with Claims Challenge #28193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Yeming Liu <[email protected]>
Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
Updates are aleady reviewed and the PR can be squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for claims challenge authentication to Connect-AzAccount
and improves error handling for MFA policy violations.
- Introduces a new
-ClaimsChallenge
parameter and propagates it through authentication flows. - Implements parsing, formatting, and processing of claims challenges in MSAL and HTTP handlers.
- Refines exception types and error messages, including localized resources and updated tests.
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Accounts/Authenticators/MsalAccessToken.cs | Implemented IClaimsChallengeProcessor and OnClaimsChallenageAsync . |
src/Accounts/Authenticators/InteractiveWamAuthenticator.cs | Pass claimsChallenge into TokenRequestContext . |
src/Accounts/Authenticators/InteractiveUserAuthenticator.cs | Pass claimsChallenge into TokenRequestContext . |
src/Accounts/Authentication/Utilities/ClaimsChallengeUtilities.cs | Added parsing and formatting utilities for claims challenges. |
src/Accounts/Authentication/Properties/Resources.resx | Added localized error message for invalid claims challenge format. |
src/Accounts/Authentication/Factories/AuthenticationFactory.cs | Extended factory overloads to accept claimsChallenge . |
src/Accounts/Authentication/ClaimsChallengeHandler.cs | Updated HTTP handler to process claims challenges and throw AzPSAuthenticationFailedException . |
src/Accounts/Authentication/Authentication/Parameters/InteractiveWamParameters.cs | Added ClaimsChallenge property. |
src/Accounts/Authentication/Authentication/Parameters/InteractiveParameters.cs | Added ClaimsChallenge property. |
src/Accounts/Authentication/Authentication/IClaimsChallengeProcessor.cs | Updated documentation for OnClaimsChallenageAsync . |
src/Accounts/Accounts/help/Connect-AzAccount.md | Updated help examples and parameter listings for -ClaimsChallenge . |
src/Accounts/Accounts/Properties/Resources.resx | Added InvalidClaimsChallenge resource entry. |
src/Accounts/Accounts/Models/RMProfileClient.cs | Extended AcquireAccessToken and Login to accept claimsChallenge . |
src/Accounts/Accounts/CommonModule/ContextAdapter.cs | Adapted context authentication to handle claims challenges. |
src/Accounts/Accounts/ChangeLog.md | Updated changelog for upcoming release. |
src/Accounts/Accounts/Account/ConnectAzureRmAccount.cs | Added parameter and parsing logic for -ClaimsChallenge . |
src/Accounts/Accounts.Test/SilentReAuthByTenantCmdletTest.cs | Updated tests to expect AzPSAuthenticationFailedException . |
Files not reviewed (2)
- src/Accounts/Accounts/Properties/Resources.Designer.cs: Language not supported
- src/Accounts/Authentication/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (6)
src/Accounts/Authenticators/MsalAccessToken.cs:136
- The method name 'OnClaimsChallenageAsync' contains a typo (Challenage → Challenge). Consider renaming it to 'OnClaimsChallengeAsync' for clarity.
public async ValueTask<bool> OnClaimsChallenageAsync(HttpRequestMessage request, string claimsChallenge, CancellationToken cancellationToken)
src/Accounts/Authentication/Factories/AuthenticationFactory.cs:224
- [nitpick] Typo in variable name 'authParamters'. It should be 'authParameters' to match standard naming conventions.
var authParamters = GetAuthenticationParameters(tokenCacheProvider, account, environment, tenant, password, promptBehavior, promptAction, claimsChallenge, tokenCache, resourceId);
src/Accounts/Authentication/Authentication/Parameters/InteractiveWamParameters.cs:24
- [nitpick] Add an XML doc-comment describing the purpose of the new
ClaimsChallenge
property.
public string ClaimsChallenge { get; set; }
src/Accounts/Authentication/Authentication/Parameters/InteractiveParameters.cs:24
- [nitpick] Add an XML doc-comment describing the purpose of the new
ClaimsChallenge
property.
public string ClaimsChallenge { get; set; }
src/Accounts/Accounts/help/Connect-AzAccount.md:21
- [nitpick] The
[<CommonParameters>]
line is detached from the main parameter list. Merge it into the previous line to maintain proper code example formatting.
[<CommonParameters>]
src/Accounts/Accounts/Properties/Resources.resx:640
- [nitpick] The new resource 'InvalidClaimsChallenge' is missing a
<comment>
element. Consider adding one to clarify usage or placeholders.
<data name="InvalidClaimsChallenge" xml:space="preserve">
/// <param name="request"></param> | ||
/// <param name="claimsChallenge"></param> | ||
/// <param name="cancellationToken"></param> | ||
/// <returns>A boolean indicated whether the request should be retried. Throws if the reauth fails.</returns> |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Grammar in the <returns>
comment should use 'indicates' instead of 'indicated' to read: 'A boolean indicates whether…'.
/// <returns>A boolean indicated whether the request should be retried. Throws if the reauth fails.</returns> | |
/// <returns>A boolean indicates whether the request should be retried. Throws if the reauth fails.</returns> |
Copilot uses AI. Check for mistakes.
{ | ||
return Enumerable.Repeat(response, 1) | ||
.Where(r => r.MatchClaimsChallengePattern()) | ||
.Select(r => r.Headers.WwwAuthenticate.FirstOrDefault().ToString()) |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling .ToString()
on FirstOrDefault()
can throw a NullReferenceException when there is no WWW-Authenticate header. Add a filter or null-check before invoking .ToString()
.
.Select(r => r.Headers.WwwAuthenticate.FirstOrDefault().ToString()) | |
.Select(r => r.Headers.WwwAuthenticate.FirstOrDefault()?.ToString() ?? string.Empty) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This pull request introduces support for claims challenge authentication in the
Connect-AzAccount
cmdlet and refines error handling for policy violations related to Multi-Factor Authentication (MFA). Key changes include the addition of a new parameter, improved error messages, and updates to authentication logic across multiple files.Claims Challenge Authentication Enhancements:
-ClaimsChallenge
parameter to theConnect-AzAccount
cmdlet to support claims challenge authentication for MFA. [1] [2]RMProfileClient
andContextAdapter
to handle claims challenges, including parsing and processing claims challenge strings. [1] [2]Error Handling Improvements:
AuthenticationFailedException
withAzPSAuthenticationFailedException
for better clarity. [1] [2]Resources.resx
. [1] [2]Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.