Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for 3000 - improve error messages when credentials fail to load #3003

Merged
merged 11 commits into from
Sep 24, 2024
Merged

Fix for 3000 - improve error messages when credentials fail to load #3003

merged 11 commits into from
Sep 24, 2024

Conversation

bgavrilMS
Copy link
Member

@bgavrilMS bgavrilMS commented Sep 3, 2024 •
edited
Loading

Fixes #3000

Function changes:

  • DefaultCredentialLoader is now responsible for setting the Skip flag, instead of individual requests
  • Added more logging
  • If all credentails fail, the exception messages now contains all the detail (similar to how AzureDefaultCredential has all the details of failing creds)

Testing

  • Unit test that the correct assertion gets into MSAL from many combinations of credentials
  • Manual test (and unit test) for using a certificate chain of MSI and cert, where MSI fails
  • Manual test with cred chain - "AssertionFromFile" and then "CertificateFromKV".

@bgavrilMS bgavrilMS requested a review from a team as a code owner September 3, 2024 13:44
@bgavrilMS bgavrilMS marked this pull request as draft September 4, 2024 16:03
@bgavrilMS bgavrilMS marked this pull request as ready for review September 6, 2024 13:14
@bgavrilMS bgavrilMS force-pushed the bogavril/3000 branch 2 times, most recently from da44a2d to 9103068 Compare September 9, 2024 15:17
jmprieur
jmprieur reviewed Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be great to create an end to end test

src/Microsoft.Identity.Web.Certificateless/ManagedIdentityClientAssertion.cs Outdated Show resolved Hide resolved
src/Microsoft.Identity.Web.Certificateless/ManagedIdentityClientAssertion.cs Outdated Show resolved Hide resolved
@bgavrilMS bgavrilMS mentioned this pull request Sep 17, 2024
Open
@jennyf19
Copy link
Collaborator

@bgavrilMS can you update w/master and respond to comments or mark them as resolved? thanks.

@jmprieur jmprieur self-requested a review September 19, 2024 15:55
@jmprieur
Copy link
Collaborator

Started doing end to end testing, and there seem to be a regression on #2675

@jmprieur
Copy link
Collaborator

Started doing end to end testing, and there seem to be a regression on #2675

Actually no. It was a test error

@jmprieur
Copy link
Collaborator

I ran regression tests, which passed

image

jennyf19
jennyf19 approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jennyf19 jennyf19 merged commit aed3c16 into master Sep 24, 2024
5 checks passed
@jennyf19 jennyf19 deleted the bogavril/3000 branch September 24, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@jmprieur jmprieur jmprieur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the error message and logs if step1 of FIC fails
3 participants
@bgavrilMS @jennyf19 @jmprieur