-
Notifications
You must be signed in to change notification settings - Fork 330
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
Implementing sendX5C on silent client credential call #1169
Conversation
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Client/ApiConfig/AcquireTokenSilentParameterBuilder.cs
Outdated
Show resolved
Hide resolved
|
||
|
||
|
||
_tokenCacheHelper.PopulateCache(app.UserTokenCacheInternal.Accessor); |
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.
Would be better to mock an actual call instead of populating the token cache artificially.
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.
Not sure I understand the reason why? The functionality that is being testing is whether or not the x5c claim is present in the header of the acquire token silent request. How the user token made its way to the cache is irrelevant. Also, there are other acquire token silent tests that do the same thing.
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.
Not blocking the fix on this, but the arguments would be:
- artificially populating the cache makes it difficult to the test reader to understand what's in the cache
- cache population logic is quite likely to become different than how MSAL populates the cache (e.g. pretty sure this won't add AppMetadata which is a FOCI concept).
- overall test logic is cleaner because it's closer to what the user would do, i.e. fetch a token, fetch smth silent. The user does not "populate the cache".
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Show resolved
Hide resolved
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.
Looks good, minor improvements in test needed.
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.
Reviewed code and Bogdan already commented on areas I had feedback. I'll take a look again once that round of changes is done.
adding additional test validation.
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ClientCredentialWithCertTest.cs
Outdated
Show resolved
Hide resolved
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.
🕐
ok, from me. however still open comments |
refactoring for PR comments.
adding withsendX5C() on acquire token silent for the confidential client application
#1149