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

[Feature Request] Allow configuration of instance metadata end-point #1603

Closed
henrik-me opened this issue Jan 30, 2020 · 7 comments
Closed
Assignees
Milestone

Comments

@henrik-me
Copy link
Contributor

henrik-me commented Jan 30, 2020

Today MSAL.NET supports specifying your own instance metadata allowing you to provide the cached metadata.

Additional details here

Customers are asking for us to provide a way to configure the instance metadata end-point on top of this. E.g. in certain environments they would like to have a service which handles getting the instance metadata and caches it. Each local service will then call that end-point instead of the global end-point.

Suggested API:

var intanceDiscoveryUri = new Uri("https://myservice.contoso.com/aad-instance-discovery");

and in the construction of the client application:

.WithInstanceDicoveryMetadata(intanceDiscoveryUri) 

Example usage:

var authorityUri = new Uri("https://login.microsoftonline.com/common/"
var intanceDiscoveryUri = new Uri("https://myservice.contoso.com/aad-instance-discovery")
var app = PublicClientApplicationBuilder
    .Create(MsalTestConstants.ClientId)
    .WithAuthority(authorityUri, false) 
    .WithInstanceDicoveryMetadata(intanceDiscoveryUri) 
    .Build();

To be defined:
Authority validation is disabled! We may want to consider that if we specify this we ignore the authority validation flag.

@bgavrilMS
Copy link
Member

This is pretty easy to implement. From a user / dev perspective, authority validation has no meaning since they give us a list of "valid" authorities.

@jmprieur
Copy link
Contributor

jmprieur commented Feb 3, 2020

Suggesting:

Example usage:

var authorityUri = new Uri("https://login.microsoftonline.com/common/"
var intanceDiscoveryUri = new Uri("https://myservice.contoso.com/aad-instance-discovery")
var app = PublicClientApplicationBuilder
    .Create(MsalTestConstants.ClientId)
    .WithAuthority(authorityUri) 
    .WithInstanceDicoveryMetadata(intanceDiscoveryUri) 
    .Build();

as .WithInstanceDicoveryMetadata(intanceDiscoveryUri) would assign validate authority to false.

@henrik-me
Copy link
Contributor Author

I believe you are voting for defaulting to "no" authority validation (as it's what is passed in anyway). Seems like we have 3 votes on this setting validate authority to false!

(Aside: we should change that to knownauthorities instead of having the bool for validateAuthority.)

@jmprieur
Copy link
Contributor

jmprieur commented Feb 4, 2020

I agree with @henrik-me that we should change the boolean to knownAuthority. It's a breaking change though (as customers could write validateAuthority: true). I think we can leave with it.

@jmprieur jmprieur added the P1 label Feb 6, 2020
@jmprieur
Copy link
Contributor

jmprieur commented Mar 2, 2020

I also agree that .WithInstanceDiscoveryMetadata() should set validateAuthrority to false automatically.

@bgavrilMS
Copy link
Member

bgavrilMS commented Mar 9, 2020

@jmprieur @henrik-me - on the topic of defaulting validateAuthority to false.

I can't really distinguish if the user has passed the flag ValidateAuthority or not, because all the WithAuthority methods all take in a default param like this:

public T WithAuthority(Uri authorityUri, bool validateAuthority = true)

To overcome this, we would need to split these methods into 2, where the second method actually means "MSAL chooses the value of the flag for me".

public T WithAuthority(Uri authorityUri, bool validateAuthority)
public T WithAuthority(Uri authorityUri)

This is not a semantic breaking change (it just adds more meaning), but it is a binary breaking change. Propose to treat it as separate issue if you'd like this implemented.

@henrik-me
Copy link
Contributor Author

This would change defaults I believe so a behavioral breaking change?

How about a new internal property for handling this case during object build time (or perhaps build time can take a look at whether the uri has been passed in)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants