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

Add token source enum #1933

Merged
merged 8 commits into from
Jul 17, 2020
Merged

Add token source enum #1933

merged 8 commits into from
Jul 17, 2020

Conversation

neha-bhargava
Copy link
Contributor

trwalke
trwalke reviewed Jul 13, 2020
View reviewed changes
Copy link
Member

@trwalke trwalke left a comment

Choose a reason for hiding this comment

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

Where are you setting the broker token source enum?

bgavrilMS
bgavrilMS requested changes Jul 13, 2020
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Looks good, only small issue is to remove the changes to the csproj file. I think you need to add the changes needed for the broker before committing - see the IBroker interface.

@jennyf19
Copy link
Collaborator

using System;

license info.

Refers to: src/client/Microsoft.Identity.Client/TokenSource.cs:1 in f6dd77f. [](commit_id = f6dd77f, deletion_comment = False)

jennyf19
jennyf19 reviewed Jul 14, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/TokenSource.cs Outdated
/// The source of the access and Id token is Identity Provider - Azure Active Directory (AAD), ADFS or AAD B2C.
/// </summary>
IdentityProvider,
/// <summary>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would you want to separate b2c from AAD and ADFS or does it matter? Should it be called AuthorizationServer? When i think of an Identity provider, I think of Google or FB or Microsoft...really just a question

Copy link
Member

Choose a reason for hiding this comment

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

The goal of this small improvement is for developers, especially on Identity.Web, to monitor their token cache hit ratio. There are other proposals around providing more details such as "which broker (name and version) was used?", but will treat them separately.

Not sure about the naming, I proposed the OAuth2 term. @jmprieur - what do you think?

jennyf19
jennyf19 reviewed Jul 14, 2020
View reviewed changes
tests/Microsoft.Identity.Test.Integration.net45/HeadlessTests/SilentAuthTests.cs
@@ -155,6 +155,7 @@ public async Task SilentAuth_TokenCacheRemainsPersistent_Async()
Assert.IsFalse(at1.Equals(at2, System.StringComparison.InvariantCultureIgnoreCase));
Assert.IsFalse(at1.Equals(at3, System.StringComparison.InvariantCultureIgnoreCase));
Assert.IsFalse(at2.Equals(at3, System.StringComparison.InvariantCultureIgnoreCase));
Assert.AreEqual(TokenSource.IdentityProvider, authResult.AuthenticationResultMetadata.TokenSource);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you assert earlier, like after line 133, that the TokenSource is TokenSource.Cache...I guess finding one that does AT silent and pulls from the cache. I would just add this check in all the tests, what about the interactive tests and broker tests?

Copy link
Member

Choose a reason for hiding this comment

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

Or after line 145, that AcquireTokenSilent gets a token from teh cache.

Copy link
Contributor Author

@neha-bhargava neha-bhargava Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@bgavrilMS That one is withForceRefresh(True) so it would not get the token from the cache. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in the other method where force refresh was false.

Copy link
Member

Choose a reason for hiding this comment

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

the assertion at line 158 is correct - force_refresh means "do not look for an AT in the cache".

jennyf19
jennyf19 approved these changes Jul 14, 2020
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:

bgavrilMS
bgavrilMS requested changes Jul 14, 2020
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

We should not expose TokenSource.None and we need a unit test or two aroun broker.

@trwalke
Copy link
Member

trwalke commented Jul 15, 2020 •
edited
Loading 

        return await _silentRequest.CacheTokenResponseAndCreateAuthenticationResultAsync(msalTokenResponse).ConfigureAwait(false);

If the access token expires the refresh token is used to refresh the token and acquire a new access token. Thus, the token is actually coming from AAD in this case. are you accounting for this scenario somewhere?

Edit: NVM, found it #Resolved

Refers to: src/client/Microsoft.Identity.Client/Internal/Requests/Silent/SilentClientAuthStrategy.cs:119 in 0e4447b. [](commit_id = 0e4447b, deletion_comment = False)

trwalke
trwalke approved these changes Jul 15, 2020
View reviewed changes
Copy link
Member

@trwalke trwalke left a comment

Choose a reason for hiding this comment

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

I agree with all of the previous comments and it looks like you covered all of the broker scenarios.
LGTM

@trwalke trwalke closed this Jul 15, 2020
@trwalke trwalke reopened this Jul 15, 2020
@neha-bhargava neha-bhargava merged commit bee0a01 into master Jul 17, 2020
@neha-bhargava neha-bhargava deleted the nebharg/enumTokenSource branch July 17, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmprieur jmprieur jmprieur left review comments

@bgavrilMS bgavrilMS bgavrilMS approved these changes

@trwalke trwalke trwalke approved these changes

@jennyf19 jennyf19 jennyf19 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.
5 participants
@neha-bhargava @jennyf19 @trwalke @bgavrilMS @jmprieur