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.

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

Adding a sample using the Singleton Token acquisition and the GraphServiceClient #159

Merged
merged 4 commits into from
May 26, 2020

Conversation

jmprieur
Copy link
Collaborator

@jmprieur jmprieur commented May 13, 2020

This is a sample using the singleton TokenAcquisition. I think, however, that I did not parent the branch to Jenny when I created it ... bah ... you get the idea.

@jmprieur jmprieur changed the base branch from master to jennyf/singleton May 13, 2020 20:11
@jmprieur jmprieur changed the base branch from jennyf/singleton to master May 13, 2020 20:12
@jennyf19
Copy link
Collaborator

Looks fine. Will you add something similar to the external sample? I guess wait to merge until i merge the singleton.

Copy link
Collaborator

@jennyf19 jennyf19 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 things, like a style thing and license stuff. i'll do a PR into this for those.

@jmprieur
Copy link
Collaborator Author

Yes @jennyf19 we'll update all the samples to use this pattern once we have agreed of the right one. @navyasric is also preparing a user study
@kalyankrishna1 @TiagoBrenck and @Shama-K were asked to do it.


namespace WebAppCallsMicrosoftGraph
{
public class WebSignInCredential : IAuthenticationProvider

Choose a reason for hiding this comment

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

Would the WebSignInCredential be defined in the Microsoft.Identity.Web library or the Graph SDK eventually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@navyasric , as discussed
This would require Microsoft.Identity.Web to depend on the Graph SDK.
What do you think, @jennyf19 @pmaytak @henrik-me @bgavrilMS ?

I would think that have another NuGet package Microsoft.Identity.Web.MsGraph that would depend on both Microsoft.Identity.Web and Graph SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a query regarding the use of National clouds through Graph SDK.
By default, the configured endpoint uses MS graph global service i.e., graph.microsoft.com. To use other national cloud services, do we need to pass the specific URL or do we have any method to handle this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually this is a good point. I had thought of it yesterday, and then forgot. The AddMicrosoftGraph method should (optionally) take a base URL for Microsoft Graph

Copy link
Contributor

Choose a reason for hiding this comment

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

Although creating a whole separate Nuget package seems like too much, I think it makes sense in this case to put this class and also the Graph service extensions into a separate package. Not every user will want to use Graph, so it'll keep the main package clean, plus maybe we might end up putting more Graph related code in there anyway.

Copy link

@navyasric navyasric May 15, 2020

Choose a reason for hiding this comment

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

@jmprieur We reviewed this proposal and discussed with the working team today and I'm channeling some feedback here. We would like to avoid Graph specific dependencies and separate package. We want to design for Microsoft.Identity.Web to work with multiple APIs and have that flexibility. Please also see my comment below on AddMicrosoftGraph abstraction.

Copy link
Member

Choose a reason for hiding this comment

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

I'm under the impression that there exists an URI endpoint that you can query to find out national resources - AAD authority, Graph endpoint and others. Using a web service to discover endpoints is better than a package because it's easier to update values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bgavrilMS : I don't think it exists yet for 3P.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pmaytak : I'm completely confused. I read one thing and its contrary? What is your recommendation? it's too much? or not?

Although creating a whole separate Nuget package seems like too much, I think it makes sense in this case to put this class and also the Graph service extensions into a separate package. Not every user will want to use Graph, so it'll keep the main package clean, plus maybe we might end up putting more Graph related code in there anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmprieur I wasn't sure of we could convince Azure.Identity to take a dependency on Microsoft.Identity.Web and then implement the WebSignInCredential there. However, the .net 3.1 dependency is a problem to make that happen.
Alternatively, Microsoft.Identity.Web could take a dependency on Azure.Core for the TokenCredential class and implement the WebSignInCredential there. Microsoft.Identity.Web would not need to depend on the Graph SDK.

@bgavrilMS You can get the Graph endpoint from the OpenID Connect discovery URL for some sovereign clouds. If you look here https://login.microsoftonline.com/microsoft.onmicrosoft.com/v2.0/.well-known/openid-configuration it is a property called msgraph_host

tests/WebAppCallsMicrosoftGraph/Startup.cs Outdated Show resolved Hide resolved

namespace WebAppCallsMicrosoftGraph
{
public class WebSignInCredential : IAuthenticationProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename the class to something that ends in provider. That seems to be the pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@darrelmiller : What do we need to implement so that the AddScope can work with the provider?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmprieur Nothing. The WithScope works on our Request object and it puts the scope information into an object in the HttpRequestMessage Properties bag. The AuthHandler middleware pulls it out just before calling MSAL.

Also, I see that WebSignInCredential is implementing IAuthenticationProvider. Now I understand why you thought you needed to take a dependency on the Graph SDK. As we try and align with the Azure SDK, we are trying to make IAuthenticationProvider go away and use the TokenCredential class as the main interface. If Microsoft.Identity.Web could take a dependency on Identity.Core then we can use that as the "base class" for all credentials and we can make IAuthenticationProvider a Graph implementation detail.

Copy link
Collaborator Author

@jmprieur jmprieur May 15, 2020

Choose a reason for hiding this comment

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

@darrelmiller: I think we'd want to depend on Identity.Core indeed. We'll need it anyway to get certs from KeyVaults using managed identity.
cc: @navyasric : this would not be an embedded WebSignInCredentials


namespace WebAppCallsMicrosoftGraph
{
public class WebSignInCredential : IAuthenticationProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

Although creating a whole separate Nuget package seems like too much, I think it makes sense in this case to put this class and also the Graph service extensions into a separate package. Not every user will want to use Graph, so it'll keep the main package clean, plus maybe we might end up putting more Graph related code in there anyway.

.AddWebAppCallsProtectedWebApi(Configuration, new string[] { "user.read" })
.AddInMemoryTokenCaches();

services.AddMicrosoftGraph(Configuration);

Choose a reason for hiding this comment

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

@jmprieur , @jennyf19 We reviewed this proposal with the working team today and I'm channeling some feedback here. Since the AddMicrosoftGraph abstraction is introducing a dependency on the Graph SDK, we feel this implementation can stay out of the Microsoft.Identity.Web and the developer can use the AddSingleton pattern which is pretty standard in ASP.NET.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks @navyasric

// This method gets called by the runtime. Use this method to add services to the container.
public void ConfigureServices(IServiceCollection services)
{
services.AddSignIn(Configuration)
Copy link

@navyasric navyasric May 15, 2020

Choose a reason for hiding this comment

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

@jmprieur , @jennyf19 Question about AddSignIn. Would the developer need to do this even when they configure EasyAuth and have signed in the user? What is the minimum configuration required?
If they need to configure sign in again with client creds, what is the benefit of EasyAuth? Does the integration still make sense?

Copy link

@peterson4ts peterson4ts May 15, 2020

Choose a reason for hiding this comment

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

I have a question too, regarding AddSignIn and the other service collection extenions, there is a motive to we are not using PKCE? After all, its the actual recomentation from the last BCP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IT's not yet implemented in MSAL.NET, @peterson4ts. When it is, we'll use it.
If you don't need to call a Web API, feel free to use it.

jmprieur added 2 commits May 26, 2020 14:01
- GraphServiceClient instead of IGraphServiceClient per Graph SDK guidance (@darrelmiller)
- Adding the handling of the incremental consent in the Razor model
- Moving the default scopes to AddMicrosoftGraph and adding a setting to express the Graph base URL (for national clouds for instance)
- Renaming the token credential provider
@jmprieur jmprieur merged commit dbb91d6 into master May 26, 2020
@jmprieur jmprieur deleted the sampleUsingSingleton branch May 27, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants