-
Notifications
You must be signed in to change notification settings - Fork 245
Fix OidcIdpSignedAssertionLoader to remove hard dependency on IConfiguration registration #3414
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
…tion dependency fix Co-authored-by: jmprieur <[email protected]>
…Configuration dependency Co-authored-by: jmprieur <[email protected]>
jmprieur
left a comment
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.
Great start
- we should use the high performance logging in .NET
- we need to add unit test for the case where the configuration is null!
src/Microsoft.Identity.Web.OidcFIC/OidcIdpSignedAssertionLoader.cs
Outdated
Show resolved
Hide resolved
…sertionLoader Co-authored-by: jmprieur <[email protected]>
Both items addressed in commit 62ef492:
All tests are passing and the implementation follows the high-performance logging patterns used elsewhere in the codebase. |
tests/Microsoft.Identity.Web.Test/OidcIdpSignedAssertionLoaderTests.cs
Outdated
Show resolved
Hide resolved
jmprieur
left a comment
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
I've some questions and suggestions @copilot
src/Microsoft.Identity.Web.OidcFIC/OidcIdpSignedAssertionLoader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web.OidcFIC/Properties/InternalsVisibleTo.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Web.Test/OidcIdpSignedAssertionLoaderTests.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Web.Test/OidcIdpSignedAssertionLoaderTests.cs
Outdated
Show resolved
Hide resolved
…and test code Co-authored-by: jmprieur <[email protected]>
|
@copilot you removed too much:
|
… project Co-authored-by: jmprieur <[email protected]>
You're absolutely right - I accidentally removed the InternalsVisibleTo.cs file when addressing the previous feedback. The file has been restored with the exact same content as before in commit 84f632b. The assembly attribute is required for the test projects to access internal members of the OidcFIC assembly. |
jmprieur
left a comment
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
Thanks
Problem
The
OidcIdpSignedAssertionLoaderclass requiredIConfigurationto be explicitly registered in the service collection, causing aSystem.InvalidOperationExceptionwhen trying to resolveICustomSignedAssertionProviderimplementations:Error message:
Solution
Following the design proposal from @jmprieur, this PR implements:
OidcIdpSignedAssertionLoaderconstructor to acceptIServiceProviderinstead ofIConfigurationIConfigurationfrom service provider only when neededmicrosoftIdentityApplicationOptions.Instanceis null or emptymicrosoftIdentityApplicationOptions.Authorityequals "//v2.0"IConfigurationis needed but not availableChanges Made
Before
After
Benefits
IConfigurationregistrationTesting
TokenAcquirerFactory.GetDefaultInstance()registersIConfigurationIConfigurationis not registered will work unless configuration binding is specifically neededIConfigurationis not available, a helpful error message with troubleshooting link is providedFixes #3411.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.