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 1738 #1743

Merged
merged 2 commits into from
Jun 3, 2022
Merged

fix for 1738 #1743

merged 2 commits into from
Jun 3, 2022

Conversation

jennyf19
Copy link
Collaborator

@jennyf19 jennyf19 commented Jun 1, 2022

@jennyf19 jennyf19 requested a review from bgavrilMS June 1, 2022 04:38
bgavrilMS
bgavrilMS approved these changes Jun 1, 2022
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, are there any unit tests that can be added? The way we choose the tenant id is quite subtle and would be worth capturing in a unit test. Maybe just this logic can be extracted in a method that is unit testable.

bgavrilMS
bgavrilMS reviewed Jun 1, 2022
View reviewed changes
src/Microsoft.Identity.Web/TokenAcquisition.cs
@@ -852,11 +852,25 @@ private IConfidentialClientApplication BuildConfidentialClientApplication(Merged
if (builder != null)
{
builder.WithSendX5C(mergedOptions.SendX5C);

ClaimsPrincipal? user = GetUserFromHttpContext();
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that IdWeb should use validatedToken's TID claim, as that is guaranteed to be there in web api scenario. Is there a ClaimsPrincipal available in web api? Where does ASP.NET Core get that from - I was under the impression that in Web API scenario there is no Id Token.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is what is happening. The ClaimsPrincipal contains the result of the validated token.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have tests for GetTenantId(), for example: GetTenantId_WithTidOrTenantIdClaims_ReturnsTenantId

Testing the entire flow will be a bit more challenging and I would need a multi-tenant web API.

bgavrilMS
bgavrilMS reviewed Jun 1, 2022
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.

Have some questions, happy to discuss offline.

@jennyf19 jennyf19 merged commit fdc81a9 into master Jun 3, 2022
@jennyf19 jennyf19 deleted the jennyf/tid branch June 3, 2022 17:11
jennyf19 added a commit that referenced this pull request Jun 8, 2022
@jennyf19 @jmprieur
@sciocoder @h3rmanj
Update POC with master (#1752)
de2940f 
* update xml (#1719)

* update lab cert thumbprint (#1718)

* changelog 1.24.1 (#1720)

* Create TokenAcquisitionAppTokenCredential.cs (#1722)

Extends Azure ADK support to include requesting tokens as the app.

This is a copy of TokenAcquisitionTokenCredential.cs with changes to class name and changes to use _tokenAcquisition.GetAuthenticationResultForAppAsync() instead of GetAuthenticationResultForUserAsync().

* fix build, add using (#1729)

* fix build, add using

* add header in test

* Adding support for .net 6 simplified templates apps (#1736)

* Jennyf/scopes roles (#1742)

* initial commit for app permissions

* add test coverage

* remove IEnumerable and use string[]

* PR comments

* IJwtBearerMiddlewareDiagnostics from singleton to transient (#1741)

* fix tests after merge from fork (#1744)

* fix for 1738 (#1743)

* changelog for 1.25 (#1747)

* update testing files to 1.25 (#1748)

* update to wilson 6.19 (#1749)

* fix: include StatusCode in HttpRequestException for .NET 5 (#1750)

* update XML (#1751)

* try 5.0.0 for config.binder in OWIN to resolve issue

* don't build owin sample

* add condition

* remove release mode

Co-authored-by: Chris Brooks <cbrooks@microsoft.com>
Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
Co-authored-by: sciocoder <sciocoder@hotmail.it>
Co-authored-by: Herman Jensen <hjanimations@hotmail.com>
jennyf19 added a commit that referenced this pull request Jul 24, 2022
@jmprieur @jennyf19
@sciocoder @h3rmanj
Initial version to support OWIN, .NET, .NET Core (#1661)
2cc4ed6 
* Initial version.
Needs a few updates to unit tests

* Updating the tests.

* - Adding a plain dotnet token acquisition host.
- AddTokenAcquisition now callable from net462
- AddGraph calllable from net462
- Adding a daemon sample using Id.Web.

* - Adding a plain dotnet token acquisition host.
- AddTokenAcquisition now callable from net462
- AddGraph calllable from net462
- Adding a daemon sample using Id.Web.

* Add a daemon-net6 sample / test

* Moving ITokenAcquirer and TokenAcquisitionOptions to
Microsoft.Identity.Web.TokenAcquisition.Abstractions

* Update

* Update

* - Simplifying the experience for the .NET 46x case (remove the need for specifying "OpendIdConnect" as an auth scheme, which didn't make sense.
- Fixing MergeOptions.Instance (was not set)
- Addressing PR feedback
- Adding missing XML comments
- Updaring tests now that there is a new interface (ITokenAcquirer)

* Updarting the OWIN sample

* Merging the OWIN implementation (only for web APIs for the moment)

* Solving build issues

* Supporting several authentication schemes in the same controller action / request
#1560

* Supporting several authentication schemes in the same controller action / request
#1560

* Adding a new assembly Microsoft.Identity.Web.TokenAcquisition
Moving types from Id.Web to the new assembly
 CONS:
- a lot of friend relationships.
- some constants should be split between id.web.tokenacquisition and Id.Web

* Fixing warnings

* Update dotnetcore.yml

* Update dotnetcore.yml

* Update dotnetcore.yml

* Update Microsoft.Identity.Web.sln

* Avoid building the OWIN web API in any CPU config

* Adding a TokenAcquirerFactory and a specialization for OWIN.

* Adding a factory from the configuration and services

* Updating the interfaces based on the discussions

* Adding the IDowstreamRestApi

* Update POC with master (#1752)

* update xml (#1719)

* update lab cert thumbprint (#1718)

* changelog 1.24.1 (#1720)

* Create TokenAcquisitionAppTokenCredential.cs (#1722)

Extends Azure ADK support to include requesting tokens as the app.

This is a copy of TokenAcquisitionTokenCredential.cs with changes to class name and changes to use _tokenAcquisition.GetAuthenticationResultForAppAsync() instead of GetAuthenticationResultForUserAsync().

* fix build, add using (#1729)

* fix build, add using

* add header in test

* Adding support for .net 6 simplified templates apps (#1736)

* Jennyf/scopes roles (#1742)

* initial commit for app permissions

* add test coverage

* remove IEnumerable and use string[]

* PR comments

* IJwtBearerMiddlewareDiagnostics from singleton to transient (#1741)

* fix tests after merge from fork (#1744)

* fix for 1738 (#1743)

* changelog for 1.25 (#1747)

* update testing files to 1.25 (#1748)

* update to wilson 6.19 (#1749)

* fix: include StatusCode in HttpRequestException for .NET 5 (#1750)

* update XML (#1751)

* try 5.0.0 for config.binder in OWIN to resolve issue

* don't build owin sample

* add condition

* remove release mode

Co-authored-by: Chris Brooks <cbrooks@microsoft.com>
Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
Co-authored-by: sciocoder <sciocoder@hotmail.it>
Co-authored-by: Herman Jensen <hjanimations@hotmail.com>

* Update names and provide a class diagram

* Addressing Brent's feedback

* Taking into account the discussions Brent/Jm from 6/13

* Attempt to reconcile ClaimsPrincipal and S2SAuthenticationTicket

* Improving the CredentialDescription.
Splitting ApplicationIdentityOptions and AadIdentityOptions

* Renaming TokenAcquirerOptions to AcquireTokenOptions

* Renaming agreed with Brent

* Adressing more feedback

* Update based on decisions made with Brent 6/17

* Fixing XML comments and adding folders in TokenAcquisition.Abstractions

* Renamings + clean-up abstractions

* Moving the MicrosotAuthenticationTicket
and MicrosoftClaimsIdentity classes to Ms.Id.Web.TokenAcquisition (out of the
abstraction) and enabling the real SecurityToken type

* Adding Graph extensibility

* Update Microsoft.Identity.Web.Certificate
to be backwards compatible in 2.0 with
1.x

* add Owin web app (#1753)

* add Owin web app

* fix build error due to package ref

* update package reference

* update web.Config

* fix pckg ref

* update web.config

* remove from release

* update new dlls with directory.build.props (#1807)

* update new dlls with directory.build.props

* Updating the Web.Config assembly bindings
in the ASP.NET OWIN Web app

* Updating the folder name for the daemon app

* PR feedback

Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>

* ID.Web.MicrosoftGraph now eferences Id.Web.TokenAcqusition (and not Id.Web) (#1810)

* - In AuthenticationOptions and MicrosoftAuthenticationOptions, move properties were they should have been in the first place (when AAD specific)
- Move LoggingOptions to Ms.Id.Web.TokenAcquisition
- MicrosoftIdentityBaseAuthenticationBuilder and MicrosoftIdentityAppCallingWebApiAuthenticationBuilder move to Ms.Id.Web.TokenAcquisition
- Session methods of MicrosoftIdentityAppCallingWebApiAuthenticationBuilder move to an extension method
- TokenAcquisitionAspNetCore inherits from TokenAcquisition instead of being a partial class implementation (Could move to another assembly if we choose to)
- AddTokenAcquisition now register ITokenAcquirer (And only this one in the case of framework others than .NET Core)

* - OwinWebApi now is 472
- OwinWebApp updates to Web.Config and project
- Ms.Id.Web.MicrosoftGraph now references Ms.Id.Web.TokenAcquisition
(no longer Ms.Id.Web)

* PR review

* Adressing PR comment

* address PR comments

* small edits

* PR feedback and renaming

* fix build issues

* address governance issues
GHSA-5crp-9r3c-p9vr

Co-authored-by: jennyf19 <jeferrie@microsoft.com>
Co-authored-by: Chris Brooks <cbrooks@microsoft.com>
Co-authored-by: sciocoder <sciocoder@hotmail.it>
Co-authored-by: Herman Jensen <hjanimations@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgavrilMS bgavrilMS bgavrilMS 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.
2 participants
@jennyf19 @bgavrilMS