Skip to content

Preparing Microsoft.Identity.Web for being distributed as a NuGet package #153

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
Merged
merged 22 commits into from
Sep 3, 2019
Merged

Preparing Microsoft.Identity.Web for being distributed as a NuGet package #153

merged 22 commits into from
Sep 3, 2019

Conversation

jmprieur
Copy link
Contributor

@jmprieur jmprieur commented Aug 5, 2019

Purpose

Preparing Microsoft.Identity.Web for being disributed as a NuGet package

  1. Merging the content from microsoft-authentication-extensions-for-dotnet Web
  2. Aligning the namespaces with the folder strucuture
  3. Renaming files and classes per the style used in MSAL.NET
  • ...

Does this introduce a breaking change?

[ x] Yes: 
- the Microsoft.Identity.Web.Client namespace has disapeared (only Microsoft.Identity.Web), and the token cache implementations are now respectively in Microsoft.Identity.Web.Client.TokenCache.Session, Microsoft.Identity.Web.Client.TokenCache.Session.InMemory, Microsoft.Identity.Web.Client.TokenCache.Session.Sql
- The methods of ITokenAcquisition / TokenAcquisition are now Async (renamed)
[ ] No

Pull Request Type

What kind of change does this Pull Request introduce?

Code clean-up.

[ ] Bugfix
[ ] Feature
[ x] Code style update (formatting, local variables)
[ x ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

@jmprieur
Preparing Microsoft.Identity.Web for being disributed as a NuGet package
b6506d7 
1) Merging the content from microsoft-authentication-extensions-for-dotnet Web
2) Aligning the namespaces with the folder strucuture
3) Renaming files and classes per the style used in MSAL.NET
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
bgavrilMS
bgavrilMS reviewed Aug 5, 2019
View reviewed changes
Microsoft.Identity.Web/AccountExtensions.cs
new ClaimsIdentity(new Claim[]
{
new Claim(ClaimConstants.Oid, account.HomeAccountId.ObjectId),
new Claim(ClaimConstants.Tid, account.HomeAccountId.TenantId),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work for B2C?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to work before, and I have not changed the semantics. This should. Do you have a concern here, @bgavrilMS ?

Copy link
Contributor

@TiagoBrenck TiagoBrenck Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@jmprieur I don't know if this was Bogdan's concern, but TID is not presented on B2C tokens.

jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
jmprieur
jmprieur commented Aug 5, 2019
View reviewed changes
bgavrilMS
bgavrilMS approved these changes Aug 5, 2019
View reviewed changes
Copy link
Contributor

@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.

LGTM - there are many "mechanical" changes done, so if you want a review on a specific part, please ask. I'd recommend creating an E2E test (e.g. stand up a website, then login) before publishing nuget.

@jmprieur
Copy link
Contributor Author

jmprieur commented Aug 6, 2019 •
edited
Loading

Thanks @bgavrilMS
I re-tested most of the samples in the sub-folders (and fixed 2 regressions I had introduced, BTW)
This is looking good

Also added documentation for the library. See Microsoft.Identity.Web/README.md

TiagoBrenck
TiagoBrenck reviewed Aug 13, 2019
View reviewed changes
Copy link
Contributor

@TiagoBrenck TiagoBrenck 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.

@jmprieur I believe we will face many challenges after releasing this package, but the community will help us to improve it and adjust it to be more generic. Having that said, I have a few considerations that I would like to point:

  • We are expecting in many places that the config file have a section called AzureAD. This requirement should be documented somewhere.
  • Is it possible to have more unit tests?
  • The method GetMsalAccountId on ClaimsPrincipalExtensions wont work for B2C apps because their tokens don't have tid. Hence, none of the token cache providers would be able to save the token cache. So, while we think about the B2C scenario, we should mention that the library wont be compatible to B2C yet.
  • On MsalAppSqlTokenCacheProviderExtension we have the config: options.UseSqlServer(sqlTokenCacheOptions.SqlConnectionString)). This is enforcing the users to use SqlServer. What should be our approach here? Is there a way for the developer to configure it with whatever DB tool they want?
  • If we use the config JwtSecurityTokenHandler.DefaultMapInboundClaims = false then options.TokenValidationParameters.NameClaimType must be set to name on JwtBearerOptions
  • If the developers want to use KeyVault to store their client secret for example, this code wouldn't allow them to use it out of the box, because it expects the client secret to be in the appsetting.json. We need to find a strategy that decouples the IConfidentialClientApplication build from the appsetting.json section but this is a tricky one.

Great work Jean-Marc!

@deisterhold
Copy link
Contributor

deisterhold commented Aug 14, 2019 •
edited by kalyankrishna1
Loading

FYI. I think there is a bug in MsalAppMemoryTokenCacheProvider where option != null should be option == null.

See #162. #Resolved

kalyankrishna1
kalyankrishna1 reviewed Aug 14, 2019
View reviewed changes
Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs
/// <param name="acceptedScopes">Scopes accepted by this web API</param>
/// <exception cref="HttpRequestException"/> with a <see cref="HttpResponse.StatusCode"/> set to
/// <see cref="HttpStatusCode.Unauthorized"/>
public static void VerifyUserHasAnyAcceptedScope(this HttpContext context, params string[] acceptedScopes)
Copy link
Contributor

Choose a reason for hiding this comment

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

VerifyUserHasAnyAcceptedScope [](start = 27, length = 29)

This pattern requires a call to this method to be placed in each and every one of every controller methods, which is very excessive. A check for needed scopes can be done in middleware, preferably after the token has been validated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on this one, @kalyankrishna1 : you are assuming that the web app / web api will have only one controller / action. I believe that real life applications have several actions, which would be each protected by their scope (at least the developer should chose)

MarkZuber
MarkZuber reviewed Aug 26, 2019
View reviewed changes
MarkZuber
MarkZuber reviewed Aug 26, 2019
View reviewed changes
Microsoft.Identity.Web/TokenCacheProviders/Sql/MsalAppSqlTokenCacheProvider.cs Outdated
}

/// <summary>
/// Reads the cache data from the backend database.
/// </summary>
private void ReadCacheForSignedInApp(TokenCacheNotificationArgs args)
{
if (this.InMemoryCache == null) // first time access
if (_inMemoryCache == null) // first time access
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure waht value the in memory cache is providing because in order to determine whether you need to invalidate the cache, you have to query the DB anyway so you're already incurring the sql round trip and request cost. The only thing you're saving is the additional data coming across the wire which is very minimal in this case. I'd propose to simplify this code and remove the in memory cache for SQL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalyankrishna1 : what did you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Took me a while to remember, I couldn't believe this myself . This is a carry over from a perf improvement when Sql cache was sort of singleton for the whole app and this updated the cache when MSAL updated the cache. Not needed any more as the cache is scoped in aspnet lifecycle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalyankrishna1 : shall I propose a PR to get rid of it?
cc: @MarkZuber

Copy link
Contributor

Choose a reason for hiding this comment

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

I already have a PR going, expect it today

MarkZuber
MarkZuber reviewed Aug 26, 2019
View reviewed changes
MarkZuber
MarkZuber reviewed Aug 26, 2019
View reviewed changes
MarkZuber
MarkZuber reviewed Aug 26, 2019
View reviewed changes
MarkZuber
MarkZuber suggested changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@MarkZuber MarkZuber left a comment

Choose a reason for hiding this comment

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

🕐

@jmprieur jmprieur mentioned this pull request Aug 27, 2019
Closed
MarkZuber
MarkZuber reviewed Aug 29, 2019
View reviewed changes
MarkZuber
MarkZuber reviewed Aug 29, 2019
View reviewed changes
MarkZuber
MarkZuber reviewed Aug 29, 2019
View reviewed changes
MarkZuber
MarkZuber approved these changes Aug 29, 2019
View reviewed changes
Copy link
Contributor

@MarkZuber MarkZuber left a comment

Choose a reason for hiding this comment

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

A few additional nits, but I"m going to pre-approve so you can get this submitted in "tomorrow-france" time from now.

@jmprieur
AddAzureAdV2Authentication, AddProtectedApiCallsWebApis; AddMsal now …
a557a64 
…takes the configuration (and optional parameters to let the developer choose the options section) => all the Startup.cs file using it are modified.

Simplifying the implementation of the token cache serializers by creating a IMsalTokenCacheProvider interface and a MsalAbstractTokenCacheProvider class implementing the common code in all the token cache providers.

Fixing a few code smells (including ConfigureAwait(false) where needed.
Chaning the CallGraph solution to be a Visual Studio 2019 solution.
@jmprieur
Addressing Mark's additional suggestions
aacdde8
@jmprieur
Updating the token caches
6eda77f
@jmprieur
Renaming the AbstractMsalTokenCacheProvider into MsaAbstractTokenCach…
4daa8e2 
…eProvider, adding a class diagram and improving a README.md
@jmprieur
- Renamed AddAzureAdV2Authentication into AddMicrosoftIdentityPlatfor…
d7d8b38 
…mAuthentication

- Renamed AddProtectedWebApiWithMicrosoftIdentityPlatformV2 into AddProtectedWebApi
@jmprieur
Provide migration help for the Sql Token cache provider
3899ca2 
Remove the AsyncUsageAnalyzers
Improves the build.bat and buildAllSln.proj to add more target (restore and clean)
@jmprieur
Merging the current state of master
63efe7f
@jmprieur
Fixing a merged project
6abbd85
@jmprieur
Renaming MsalUiRequiredExceptionFilterAttribute to AuthorizeForScopes…
ada7226 
…Attribute
@jmprieur
Copy link
Contributor Author

jmprieur commented Sep 3, 2019

@MarkZuber @TiagoBrenck
I've taken into account all your feedback, except, @TiagoBrenck for B2C (we need to discuss what to do here)

@jmprieur jmprieur merged commit d5166bb into master Sep 3, 2019
@jmprieur jmprieur changed the title Preparing Microsoft.Identity.Web for being disributed as a NuGet package Preparing Microsoft.Identity.Web for being distributed as a NuGet package Sep 3, 2019
@jmprieur jmprieur deleted the PrepareForMsIndentityWebAsNuget branch September 27, 2019 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalyankrishna1 kalyankrishna1 kalyankrishna1 left review comments

@TiagoBrenck TiagoBrenck TiagoBrenck left review comments

@MarkZuber MarkZuber MarkZuber approved these changes

@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.
6 participants
@jmprieur @deisterhold @kalyankrishna1 @MarkZuber @bgavrilMS @TiagoBrenck