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

Identity web templates #24065

Closed
wants to merge 17 commits into from
Closed

Identity web templates #24065

wants to merge 17 commits into from

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Jul 17, 2020 •
edited
Loading

Fixes #24024
Fixes #22726

FYI @jmprieur

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 17, 2020
@jmprieur jmprieur mentioned this pull request Jul 17, 2020
Closed
@JunTaoLuo
Copy link
Contributor Author

FYI, I plan to make the fixes in this PR so I don't think we need tracking issues in microsoft-identity-web. I assume after we merge the templates here there's no reason to keep the templates in microsoft-identity-web?

@jmprieur
Copy link
Contributor

@JunTaoLuo: I don't know about not having them in Microsoft.Identity.Web any longer.
We'll bring more innovation and therefore would probably want to improve the templates as the library improves (think of certificate management etc ...).
But for sure I'd make sure they don't diverge

@JunTaoLuo
Copy link
Contributor Author

@jmprieur Looking at the templates test and I want to double check my understanding with the possible configurations

Valid scenarios:

  1. OrganizationAuth specified with CalledApiUrl or CalledApiScopes configured
  2. OrganizationAuth specified with CallsMicrosoftGraph configured
  3. IndividualB2CAuth specified with CalledApiUrl or CalledApiScopes configured

Invalid scenarios:

  1. CalledApiUrl or CalledApiScopes configured without OrganizationAuth or IndividualB2CAuth specified
  2. CallsMicrosoftGraph configured without OrganizationAuth or IndividualB2CAuth specified
  3. CallsMicrosoftGraph and (CalledApiUrl or CalledApiScopes) configured

Uncertain scenarios:

  1. OrganizationAuth specified without CalledApiUrl or CalledApiScopes or CallsMicrosoftGraph configured
  2. IndividualB2CAuth specified without CalledApiUrl or CalledApiScopes or CallsMicrosoftGraph configured
  3. OrganizationAuth specified with CallsMicrosoftGraph configured

I want to ensure I add tests for all valid scenarios.

@jmprieur
Copy link
Contributor

jmprieur commented Jul 18, 2020 •
edited
Loading

@JunTaoLuo :

@jmprieur Looking at the templates test and I want to double check my understanding with the possible configurations

Valid scenarios:

  1. OrganizationAuth specified with CalledApiUrl or CalledApiScopes configured

Actually both CalledApiUrl and CalledApiScopes need to be specified (they go together). I've provided a default which is graph, but it's not relevant in general

  1. OrganizationAuth specified with CallsMicrosoftGraph configured

Yes

  1. IndividualB2CAuth specified with CalledApiUrl or CalledApiScopes configured

Same both CalledApiUrl and CalledApiScopes need to be specified (they go together)

Invalid scenarios:

  1. CalledApiUrl or CalledApiScopes configured without OrganizationAuth or IndividualB2CAuth specified

Indeed, this is invalid

  1. CallsMicrosoftGraph configured without OrganizationAuth or IndividualB2CAuth specified

Indeed, this is invalid

  1. CallsMicrosoftGraph and (CalledApiUrl or CalledApiScopes) configured

It could be valid, but the templates don't support it today. They could (a bit more work, and more properties as Scopes is used for Graph and for the Web API)

Uncertain scenarios:

  1. OrganizationAuth specified without CalledApiUrl or CalledApiScopes or CallsMicrosoftGraph configured

It's a valid and very important (and legacy) scenario: "Web app that signs in users with Azure AD", and "Protected Web API with Azure AD". Developers don't always need to call a downstream API

  1. IndividualB2CAuth specified without CalledApiUrl or CalledApiScopes or CallsMicrosoftGraph configured

It's a valid and very important (and legacy) scenario: "Web app that signs in users with Azure AD B2C", and "Protected Web API with Azure AD B2C". Developers don't always need to call a downstream API

  1. OrganizationAuth specified with CallsMicrosoftGraph configured

It's a valid scenario.

I want to ensure I add tests for all valid scenarios.

jmprieur
jmprieur reviewed Jul 18, 2020
View reviewed changes
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

@JunTaoLuo, LGTM.
I left a few comments

@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Jul 18, 2020 •
edited
Loading

@jmprieur I had a typo in my previous comment but to clarify:

Valid scenarios:

  1. OrganizationAuth specified with CalledApiUrl and CalledApiScopes configured
  2. OrganizationAuth specified with CallsMicrosoftGraph configured
  3. IndividualB2CAuth specified with CalledApiUrl and CalledApiScopes configured
  4. OrganizationAuth specified without CalledApiUrl or CalledApiScopes or CallsMicrosoftGraph configured
  5. IndividualB2CAuth specified without CalledApiUrl or CalledApiScopes or CallsMicrosoftGraph configured

Invalid scenarios:

  1. CalledApiUrl or CalledApiScopes configured without OrganizationAuth or IndividualB2CAuth specified
  2. CallsMicrosoftGraph configured without OrganizationAuth or IndividualB2CAuth specified
  3. IndividualB2CAuth specified with CallsMicrosoftGraph configured

Not yet supported:

  1. CallsMicrosoftGraph and (CalledApiUrl or CalledApiScopes) configured

I'll add a template test, that creates and compiles the generated template for the valid scenarios. Note that I won't be able to test any functionality since I don't know how to set that up.

@JunTaoLuo JunTaoLuo marked this pull request as ready for review July 20, 2020 16:16
@JunTaoLuo JunTaoLuo requested a review from a team as a code owner July 21, 2020 05:40
@JunTaoLuo JunTaoLuo changed the base branch from master to release/5.0-preview8 July 21, 2020 05:41
@JunTaoLuo JunTaoLuo requested a review from a team July 21, 2020 05:41
@JunTaoLuo JunTaoLuo changed the base branch from release/5.0-preview8 to master July 21, 2020 05:41
javiercn
javiercn reviewed Jul 21, 2020
View reviewed changes
src/Azure/AzureAD/Authentication.AzureADB2C.UI/src/AzureAdB2COptions.cs
@@ -9,6 +10,7 @@ namespace Microsoft.AspNetCore.Authentication.AzureADB2C.UI
/// <summary>
/// Options for configuring authentication using Azure Active Directory B2C.
/// </summary>
[Obsolete("This is obsolete and will be removed in a future version. Use Microsoft.Identity.Web instead. See https://aka.ms/ms-identity-web.")]
Copy link
Member

Choose a reason for hiding this comment

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

If we are obsoleting this APIs, we need to point to a URL within the official docs that contains details on the migration process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmprieur can you write something like that? Also, for breaking changes, we usually write an announcement like aspnet/Announcements#414. Can you create something like that so we can link to it?

javiercn
javiercn reviewed Jul 21, 2020
View reviewed changes
...ProjectTemplates/Web.ProjectTemplates/content/StarterWeb-CSharp/Services/DownstreamWebApi.cs
public interface IDownstreamWebApi
{
Task<string> CallWebApi(string relativeEndpoint = "", string[] requiredScopes = null);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason why we need an interface here?

javiercn
javiercn reviewed Jul 21, 2020
View reviewed changes
...s/Web.ProjectTemplates/content/StarterWeb-CSharp/Services/MicrosoftGraphServiceExtensions.cs
services.AddTokenAcquisition(true);
services.AddSingleton<GraphServiceClient, GraphServiceClient>(serviceProvider =>
{
var tokenAquisitionService = serviceProvider.GetService<ITokenAcquisition>();
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs to be GetRequiredService if you expect this to not be null.

@jaredpar jaredpar mentioned this pull request Jul 21, 2020
Closed
@JunTaoLuo
Copy link
Contributor Author

I'm going to open a separate PR to retarget these changes to preview 8 (rebasing was a hassle since there was a merge commit in between). Let's finish off with Javier's comment on this PR but any new discussion should be opened in the preview 8 PR instead.

@JunTaoLuo JunTaoLuo mentioned this pull request Jul 21, 2020
Merged
@JunTaoLuo JunTaoLuo closed this Jul 23, 2020
@JunTaoLuo
Copy link
Contributor Author

I'm going to transplant the remaining comment to the p8 PR.

@JunTaoLuo JunTaoLuo deleted the johluo/identity-web-templates branch September 8, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu left review comments

@javiercn javiercn javiercn left review comments

@jmprieur jmprieur jmprieur left review comments

@blowdart blowdart Awaiting requested review from blowdart

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
5 participants
@JunTaoLuo @jmprieur @dougbu @javiercn @Pilchie