Skip to content

Updating Web project templates to ms.id.web 0.3.0-preview #25309

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

Merged
merged 8 commits into from
Aug 29, 2020
Merged

Updating Web project templates to ms.id.web 0.3.0-preview #25309

merged 8 commits into from
Aug 29, 2020

Conversation

jmprieur
Copy link
Contributor

Summary of the changes

  • Updates AddMicrosoftIdentityWeb* names based on what
    Identity and ASP.NET Core team agreed
  • Fixes issue with the MultiTenant case (tenantId = common)
  • Uses the new reusable interfaces (AddMicrosoftGraph, and AddDownstreamApi)

This removes several files which are now part of Microsoft.Identity.Web
The projects will need to reference Microsoft.Identity.Web 0.3.0-preview

Addresses #bugnumber (in this specific format)

@jmprieur jmprieur requested a review from a team as a code owner August 27, 2020 12:19
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 27, 2020
and quick fix to the Blasorwasm default values for
the Web api so that customers have a better experience
@jmprieur jmprieur requested a review from dougbu as a code owner August 27, 2020 16:32
Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

Some minor style feedback.

@JunTaoLuo
Copy link
Contributor

Some rough notes on how to test the new templates from a PR build. I'm omitting some small details but let me know if any of the steps are confusing.

  1. Get the latest RC1 SDK (this step might be optional) and add it to the path, alternatively run all dotnet commands from that directory:
    https://dotnetcli.azureedge.net/dotnet/Sdk/5.0.100-rc.1.20379.10/dotnet-sdk-5.0.100-rc.1.20379.10-win-x64.zip
  2. Download the templates nupkg
    The package can be found under the published artifacts of the azdo build under Windows_Packages/Release/Shipping/Microsoft.DotNet.Web.ProjectTemplates.5.0.5.0.0-ci.nupkg. For example: https://dev.azure.com/dnceng/public/_build/results?buildId=790308&view=artifacts&type=publishedArtifacts. This is an unrelated build for illustration purposes since this PR's build hasn't finished yet so don't actually use those templates.
  3. Uninstall the existing project template
    dotnet new -u Microsoft.DotNet.Web.ProjectTemplates.5.0
  4. Install new project template
    dotnet new -i C:\Users\johluo\Downloads\Microsoft.DotNet.Web.ProjectTemplates.5.0.5.0.0-ci.nupkg
  5. Run your verification script/tool with the new templates

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

Minor spacing fixes. I'm approving this now but @Tratcher @blowdart @javiercn @captainsafia should also take a look at the new API and template changes.

@@ -116,7 +119,7 @@ public void ConfigureServices(IServiceCollection services)
services.AddRazorPages();
#if (OrganizationalAuth || IndividualB2CAuth)
services.AddServerSideBlazor()
.AddMicrosoftIdentityConsentHandler();
.AddMicrosoftIdentityConsentHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Indents

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

#endif
.AddInMemoryTokenCaches();
#else
.AddMicrosoftIdentityWebApi(Configuration.GetSection("AzureAd"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

using Microsoft.Identity.Web;
using Microsoft.Identity.Web.UI;
using Microsoft.Identity.Web.TokenCacheProviders.InMemory;
using Microsoft.AspNetCore.Authorization;
Copy link
Contributor

Choose a reason for hiding this comment

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

sort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm seems like it isn't sorted, move this line to line 8

@@ -112,7 +111,7 @@ public void ConfigureServices(IServiceCollection services)
#endif
#if (OrganizationalAuth || IndividualB2CAuth)
services.AddRazorPages()
.AddMicrosoftIdentityUI();
.AddMicrosoftIdentityUI();
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, revert indentation.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

It looks in line with the agreed design.

@JunTaoLuo
Copy link
Contributor

I'll clean up the templates's indenting and react to feedback to get this merged today.

@jmprieur
Copy link
Contributor Author

I'm updating the PR right now (after testing)

@javiercn
Copy link
Member

Hey folks, seems like the changes that we did originally broke the Blazor server individual auth template. Any chance we can get that addressed as part of this? #25353

@JunTaoLuo
Copy link
Contributor

Sure, can you push a commit to this branch @javiercn

@jmprieur
Copy link
Contributor Author

Hey folks, seems like the changes that we did originally broke the Blazor server individual auth template. Any chance we can get that addressed as part of this? #25353

@javiercn: I was not aware of this issue. Let's fix it.

@JunTaoLuo
Copy link
Contributor

Ah we don't have permission to push to your branch since it's in your fork. @javiercn I think there's no benefit to piggy-backing off of this PR. Care to send a separate one?

@javiercn
Copy link
Member

@jmprieur thanks for taking care of it.

Ah we don't have permission to push to your branch since it's in your fork. @javiercn I think there's no benefit to piggy-backing off of this PR. Care to send a separate one?

I'm fine having it as a separate PR as long as we fix it, hopefully before Tuesday to avoid going through full ask-mode.

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

I realized my previous comment was misleading.

#endif
#endif
#if (OrganizationalAuth || IndividualB2CAuth)
services.AddControllersWithViews()
.AddMicrosoftIdentityUI();
.AddMicrosoftIdentityUI();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.AddMicrosoftIdentityUI();
.AddMicrosoftIdentityUI();

Sorry I mean that these extra indentations should be removed. As in revert the change to this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see :). Will do.

@@ -116,7 +119,7 @@ public void ConfigureServices(IServiceCollection services)
services.AddRazorPages();
#if (OrganizationalAuth || IndividualB2CAuth)
services.AddServerSideBlazor()
.AddMicrosoftIdentityConsentHandler();
.AddMicrosoftIdentityConsentHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@@ -105,11 +103,11 @@ public void ConfigureServices(IServiceCollection services)
options.FallbackPolicy = options.DefaultPolicy;
});
services.AddRazorPages()
.AddMvcOptions(options => {})
.AddMicrosoftIdentityUI();
.AddMvcOptions(options => {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, revert the extra indentations here and on lines 107 and 110

using Microsoft.Identity.Web;
using Microsoft.Identity.Web.UI;
using Microsoft.Identity.Web.TokenCacheProviders.InMemory;
using Microsoft.AspNetCore.Authorization;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm seems like it isn't sorted, move this line to line 8

@@ -112,7 +111,7 @@ public void ConfigureServices(IServiceCollection services)
#endif
#if (OrganizationalAuth || IndividualB2CAuth)
services.AddRazorPages()
.AddMicrosoftIdentityUI();
.AddMicrosoftIdentityUI();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, revert indentation.

@jmprieur
Copy link
Contributor Author

Ah we don't have permission to push to your branch since it's in your fork. @javiercn I think there's no benefit to piggy-backing off of this PR. Care to send a separate one?

@JunTaoLuo @javiercn : I just gave you permissions to my fork.
If anybody needs access, don't hesitate to ask (or give them. I'll make you admin when you have accepted the invite)

@JunTaoLuo JunTaoLuo merged commit 744e96b into dotnet:release/5.0 Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants