Bypassing redirect URI validation for nil URI if developers set bypassRedirectUriValidation property #2175
Conversation
…ation config to silent token parameters.
![azure-pipelines[bot]](https://github.com/avatars/in/9426?s=60&v=4){kind=small}
| @@ -879,6 +879,7 @@ - (void)acquireTokenSilentWithParameters:(MSALSilentTokenParameters *)parameters | |||
| // Nested auth protocol | |||
| msidParams.nestedAuthBrokerClientId = self.internalConfig.nestedAuthBrokerClientId; | |||
| msidParams.nestedAuthBrokerRedirectUri = self.internalConfig.nestedAuthBrokerRedirectUri; | |||
| msidParams.skipSendRedirectUri = [NSStringFromClass([self class]) isEqualToString:@"MSAL.MSALNativeAuthPublicClientApplication"]; | |||
There was a problem hiding this comment.
We can't only check the class name to set this parameter, what if the native auth user sets the redirect URI? In that case we should send it.
Here's my suggestion for this issue: We should make the redirect URI optional in MSAL. Currently, the redirect URI is always validated, even with the "bypassRedirectUriValidation" flag set to true, and this causes the MSAL public client application to fail the initialisation. See this code in IdentityCore here. If we change this behaviour, we can skip the default value for the redirect URI, and only send it when it's specified. We can start by looking at the IdentityCore code I mentioned and assess the impact of making the redirect URI optional.
There was a problem hiding this comment.
Fair point, right now is not possible to pass nil as the redirect URI, so is impossible to know for certain that the developer wants or not to use it. I'll look into your suggestion, but I think that is not the only validation that we need to surpass. We need to understand why is not possible to have a nil redirect URI on MSAL Objective-C, if this is possible in Android. Do we have a design/conceptual reason for that @antrix1989 ?
There was a problem hiding this comment.
Changing the mentioned code won't have the intended effect, since this method will return a nil instance of MSIDRedirectUri, which is what we want if developers don't pass a redirect URI. There are other points in MSAL and IdentityCore that are not taking in consideration the bypassRedirectUriValidation parameter. I think that the latest change in this PR addresses that.
There was a problem hiding this comment.
I updated the IdentityCore PR as well.
…ic client config Bypassing nil redirect URI if developer sets "bypassRedirectURIValidation" Passing configuration to silent token request parameters
borgesmb
changed the title
…er passes a nil redirect URI - Testing `bypassRedirectURIValidation` when copying MSALPublicClientApplicationConfig
nilo-ms
force-pushed
the
borgesmb/optional-redirect-uri
branch
from
e480d9a
to
acf907c
Compare
This test was having this behaviour only because the redirectURI in info plist was not matching the default redirect uri (the behaviour was not related to the byPassRedirectURIValidation param)
* dev: share ownership of CHANGELOG file (#2194) Add platform sequence param. (#2192) Submodule update (#2189) Feature/multiple tokens 4 (#2159) Bypassing redirect URI validation for nil URI if developers set bypassRedirectUriValidation property (#2175) Update submodule Update submodule Add new error code when SSO ext config doesn't contain valid Error Handling config # Conflicts: # MSAL/IdentityCore # MSAL/src/MSALErrorConverter.m
Proposed changes
Bypassing redirect URI validation for nil URI if developers set
bypassRedirectUriValidationproperty. This PR has a dependency on: AzureAD/microsoft-authentication-library-common-for-objc#1375.Type of change
Risk
Additional information