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

OBO for SP with RT support #3266

Merged
merged 4 commits into from
Apr 14, 2022
Merged

OBO for SP with RT support #3266

merged 4 commits into from
Apr 14, 2022

Conversation

bgavrilMS
Copy link
Member

@bgavrilMS bgavrilMS commented Apr 6, 2022 •
edited

Fixes #2817

Changes proposed in this request

  • OBO for SP + RT fixes
  • integration test that exercises the flow

Testing
integration test

Performance impact
similar to user obo

Based on @trwalke 's research.

@bgavrilMS bgavrilMS merged commit aad1331 into master Apr 14, 2022
@bgavrilMS bgavrilMS deleted the bogavril/spobo branch April 14, 2022 10:46
bgavrilMS
bgavrilMS commented Apr 14, 2022
View reviewed changes
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
@@ -505,7 +493,7 @@ private static string GetPreferredUsernameFromIdToken(bool isAdfsAuthority, IdTo
}

MsalAccessTokenCacheItem msalAccessTokenCacheItem = GetSingleToken(accessTokens, requestParams);
msalAccessTokenCacheItem = FilterTokensByKeyId(msalAccessTokenCacheItem, requestParams);
msalAccessTokenCacheItem = FilterTokensByPopKeyId(msalAccessTokenCacheItem, requestParams);
Copy link
Member Author

Choose a reason for hiding this comment

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

Thx!

bgavrilMS added a commit that referenced this pull request Apr 14, 2022
@bgavrilMS
Revert "OBO for SP with RT support (#3266)"
c4738dd 
This reverts commit aad1331.
pmaytak
pmaytak reviewed Apr 14, 2022
View reviewed changes
src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs

var msalTokenResponse = await SilentRequestHelper.RefreshAccessTokenAsync(cachedRefreshToken, this, AuthenticationRequestParameters, cancellationToken)
.ConfigureAwait(false);

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

if (AcquireTokenInLongRunningOboWasCalled())
{
AuthenticationRequestParameters.RequestContext.Logger.Error("[FindAccessTokenAsync] AcquireTokenInLongRunningProcess was called and OBO token was not found in the cache.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Change log to "[OBO request] AcquireTokenInLongRunningProcess was called and no access or refresh tokens were found in the cache."
Need to verify that MsalErrorMessage.OboCacheKeyNotInCache is still accurate.

src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs
// Returns whether AcquireTokenInLongRunningProcess was called (user assertion is null in this case)
private bool AcquireTokenInLongRunningOboWasCalled()
{
return AuthenticationRequestParameters.ApiId == ApiEvent.ApiIds.AcquireTokenOnBehalfOf &&
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ApiId check can be removed.

@trwalke trwalke mentioned this pull request Apr 14, 2022
Merged
bgavrilMS added a commit that referenced this pull request Apr 15, 2022
@bgavrilMS
OBO for SP with RT support (#3266)
dd5b963 
* OBO for SP with RT support

* Address PR

* Resolving OBO for long running process error when different scopes are used.

Co-authored-by: trwalke <trwalke@microsoft.com>
bgavrilMS added a commit that referenced this pull request Apr 21, 2022
@bgavrilMS @trwalke @pmaytak
OBO for SP with RT support (#3266) (#3280)
c34cc1f 
* OBO for SP with RT support (#3266)

* OBO for SP with RT support

* Address PR

* Resolving OBO for long running process error when different scopes are used.

Co-authored-by: trwalke <trwalke@microsoft.com>

* Update src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs

Co-authored-by: Peter M <34331512+pmaytak@users.noreply.github.com>

* Pr Feedback

* Fix build

Co-authored-by: trwalke <trwalke@microsoft.com>
Co-authored-by: Travis Walker <travis.walker@microsoft.com>
Co-authored-by: Peter M <34331512+pmaytak@users.noreply.github.com>
Csaba8472 added a commit to Csaba8472/microsoft-authentication-library-for-dotnet that referenced this pull request Apr 24, 2022
@Csaba8472
Merge branch 'master' of github.com:AzureAD/microsoft-authentication-…
0004f65 
…library-for-dotnet

* 'master' of github.com:AzureAD/microsoft-authentication-library-for-dotnet: (39 commits)
  OBO for SP with RT support (AzureAD#3266) (AzureAD#3280)
  Update some projects to net48 (AzureAD#3269)
  Bump Source Link version (tool change) (AzureAD#3275)
  Update changelog.txt (AzureAD#3282)
  Default to WebView1 instead of WebView2 when using AAD or ADFS authorities (AzureAD#3276)
  Revert "OBO for SP with RT support (AzureAD#3266)"
  OBO for SP with RT support (AzureAD#3266)
  Add perf tests for Acquire Token calls with serialization cache and Builders (AzureAD#3250)
  Minor fix to SuggestedCacheKey comment and update NuGet icon in Readme. (AzureAD#3268)
  MSAL changelog 4.43.0 (AzureAD#3263)
  Bogavril/3251 (AzureAD#3255)
  Fix for AzureAD#3248 - use the correct plugin to sing out (AzureAD#3253)
  Performance project improvements (AzureAD#3064)
  Reuse lists in token cache filtering logic. (AzureAD#3233)
  Update andorid install script to use andorid 30 (AzureAD#3243)
  Fix for UWP packaging (AzureAD#3239)
  Pass account to Auth result (AzureAD#3199)
  App ported to Lab4 (AzureAD#3229)
  Trwalke/ruleset updates (AzureAD#3189)
  Conditional Access for Android (AzureAD#3210)
  ...
pmaytak pushed a commit that referenced this pull request May 12, 2022
@bgavrilMS @pmaytak
OBO for SP with RT support (#3266)
ccec41b 
* OBO for SP with RT support

* Address PR

* Resolving OBO for long running process error when different scopes are used.

Co-authored-by: trwalke <trwalke@microsoft.com>
pmaytak pushed a commit that referenced this pull request May 12, 2022
@bgavrilMS @pmaytak
Revert "OBO for SP with RT support (#3266)"
1ed5a3c 
This reverts commit aad1331.
pmaytak added a commit that referenced this pull request May 12, 2022
@bgavrilMS @trwalke @pmaytak
OBO for SP with RT support (#3266) (#3280)
42abcd9 
* OBO for SP with RT support (#3266)

* OBO for SP with RT support

* Address PR

* Resolving OBO for long running process error when different scopes are used.

Co-authored-by: trwalke <trwalke@microsoft.com>

* Update src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs

Co-authored-by: Peter M <34331512+pmaytak@users.noreply.github.com>

* Pr Feedback

* Fix build

Co-authored-by: trwalke <trwalke@microsoft.com>
Co-authored-by: Travis Walker <travis.walker@microsoft.com>
Co-authored-by: Peter M <34331512+pmaytak@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmaytak pmaytak pmaytak approved these changes

@trwalke trwalke trwalke approved these changes

@gladjohn gladjohn Awaiting requested review from gladjohn

@SameerK-MSFT SameerK-MSFT Awaiting requested review from SameerK-MSFT

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

Successfully merging this pull request may close these issues.

[Feature Request] [SPIKE] OBO for SP does not work when refreshing the RT
3 participants
@bgavrilMS @trwalke @pmaytak