-
Notifications
You must be signed in to change notification settings - Fork 88
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
Confidential client requires authority #394
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Is the "client with a custom token provider" the current workaround for caching Managed Identity tokens? Then indeed it is an uncommon scenario which is expected to be replaced by MSAL's coming-soon native Managed Identity support. In the "specifying a tenant for every authentication" scenario, semantically we may encourage customer to provide an Here comes a thought experiment. After making authority required, what if an app developer still pass in (By the way, does AAD's OBO automatically ignored the tenant in token endpoint, and use the incoming assertion's tid instead?) |
Yes, that's the one. Definitely an advanced feature and the Azure SDK may be its only user, so I don't mind it being a little awkward.
I think the constructor shouldn't have an opinion about whether a specific authority is valid. That's up to AAD. Also, it seems unkind for the constructor to reject an arbitrary value that won't ever be used in some scenarios.
I guess so. It works for the integration test and also in my test tenant, provided I don't auth a guest user. So, here's a scenario in which "lmo/common" is a valid authority for a confidential client. |
So, it becomes clear that, "#394 making authority required" itself is irrelevant to "#348 requires a specific tenant". If you want to reject ".../common" as per #348, you will have to have a runtime check, which would not play well either with this new ctor or with the old default ".../common" value. That being said, it does not harm to make this ctor change here. In that sense I think this PR is fine. It is just that it does not necessarily address #348. |
It isn't irrelevant, because today authority is optional for the confidential client constructor and "../common" is the default value. I don't intend to prevent applications from specifying "common" as the authority, at least not at construction. (Doing so is an error in method calls e.g. |
This makes authority a required parameter of the confidential client constructor (closes #348). The value must be a valid HTTPS URL with at least one path segment; anything else is a runtime error. Something that makes me hesitant here is that we have two scenarios in which authority really is optional: specifying a tenant for every authentication, and using Client with a custom token provider. I doubt these are common scenarios but it's unfortunate that this change requires their users to pass some value, possibly a bogus one.