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

Redirect URI should not be required for Confidential Client #426

Closed
clairernovotny opened this issue May 19, 2017 · 15 comments
Closed

Redirect URI should not be required for Confidential Client #426

clairernovotny opened this issue May 19, 2017 · 15 comments

Comments

@clairernovotny
Copy link

Some flows, like the On-behalf-of flow don't require a redirect URI, so that value shouldn't be required in the ConfidentialClient constructor.

@kpanwar
Copy link

kpanwar commented May 19, 2017

You should be able to pass null and not run into issues.

@clairernovotny
Copy link
Author

@kpanwar can that be documented? Right now the param comments say that it's required, so people may get confused

@kpanwar
Copy link

kpanwar commented May 19, 2017

Sure! I will kee this issue open to track the work.

@htuomola
Copy link

htuomola commented Nov 1, 2017

Actually passing redirect uri as null causes an exception when calling AcquireTokenOnBehalfOfAsync(scopes, UserAssertion), on creating request params.

System.ArgumentNullException : Value cannot be null.
Parameter name: uriString
   at System.Uri..ctor(String uriString)
   at Microsoft.Identity.Client.ClientApplicationBase.CreateRequestParameters(Authority authority, IEnumerable`1 scopes, IUser user, TokenCache cache)
   at Microsoft.Identity.Client.ConfidentialClientApplication.CreateRequestParameters(Authority authority, IEnumerable`1 scopes, IUser user, TokenCache cache)
   at Microsoft.Identity.Client.ConfidentialClientApplication.<AcquireTokenOnBehalfCommonAsync>d__16.MoveNext()

I think it comes down to this, which won't work unless RedirectUri defaults to some valid uri:

Code itself:

var scopes = new[] {"https://graph.microsoft.com/user.read"};
var userTokenCache = new TokenCache();
var cca = new ConfidentialClientApplication(clientSettings.ClientId, null, new ClientCredential(clientSettings.AppKey), userTokenCache, null);
await cca.AcquireTokenOnBehalfOfAsync(scopes, GetUserAssertion(userInformation))

@jmprieur
Copy link
Contributor

jmprieur commented Jan 8, 2018

We need a redirect URL for the authorization code grant, but not for client creds, OBO, ...
The idea is to make it optional by adding overrides, or accept null and throw when using the authorization code throw.

@jmprieur
Copy link
Contributor

@bgavrilMS : this one is poachable

@bgavrilMS
Copy link
Member

@jmprieur - I propose that we make sure the docs emphasize where redirect URI can be null and fix the exception for OBO flow and client creds.

Alternatively, I can obsolete the calls requiring a redirectUri so that we can remove them in when we get rid of the -preview tag?

@jmprieur
Copy link
Contributor

@bgavrilMS : I'd prefer the first option (allow null, fix the exception for Client credentials. Not sure for OBO?)
The alternative would touch the constructor … and most confidential client flow require the redirect URI. I would think OBO does?

@MSAppsDev
Copy link

Hi,
Is this fixed?

@bgavrilMS
Copy link
Member

@MSAppsDev - no, the issue is still open.

@jmprieur jmprieur moved this from To do to In progress in MSAL.NET (legacy) Jan 11, 2019
@jmprieur
Copy link
Contributor

@MarkZuber : done part of the confidential client builder based app configuration

@bgavrilMS bgavrilMS moved this from In progress to Committed in MSAL.NET (legacy) Jan 22, 2019
@bgavrilMS bgavrilMS moved this from Committed to In progress in MSAL.NET (legacy) Jan 22, 2019
@MarkZuber
Copy link
Contributor

In 3.x, constructing with null will set the default redirecturi to be urn:ietf:wg:oauth:2.0:oob and that redirecturi will get used during any requests as needed.

I've added unit tests to explicitly cover this case in dev3x.

@jennyf19
Copy link
Collaborator

@MarkZuber just for confidential clients?

@jmprieur
Copy link
Contributor

jmprieur commented Jan 23, 2019

yes, just for Client Credentials, actually (daemon apps)
So the default replyUri for confidential client apps could rather be https://replyUriNotSet or something like that. For public client apps it's more subtil than that (do what we used to do)

@jennyf19
Copy link
Collaborator

Fixed in MSAL 3.0.0-preview release

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

8 participants