Support for setting redirect-url in token-fetch portion of confidential-app authentication code flow #96
Conversation
…ed some of the options passed in via AuthCodeOptions
…ovided parameter or fill with a default (for native/device auth sequence)
|
|
harikb
changed the title
| @@ -129,4 +129,5 @@ type AcquireTokenByDeviceCodeOptions struct { | |||
| type AcquireTokenByAuthCodeOptions struct { | |||
| Code string | |||
| CodeChallenge string | |||
| RedirectURI string | |||
There was a problem hiding this comment.
I am not the original requester of that change, although our handles look similar. In my case, I was trying to use a token-fetch as an explicit call back to MS instead of implicit-token flow. As per the documentation, implicit token on auth-code flow is less secure and they recommend webapps fetch it back from MS. When we call back, this redirectURI is required, and matching original auth request.
There was a problem hiding this comment.
The recommendation is for apps to use the https://login.microsoftonline.com/common/oauth2/nativeclient reply URL, this is what I did when testing the change and it worked for me. See the conversation here. I'm far from an expert here though so somebody from the MSAL team needs to comment further.
MSAL for other languages doesn't expose a redirect URI parameter.
There was a problem hiding this comment.
@jmprieur I don't quite understand this statement that you made in #68
but MSAL stops the redirect
My use-case is a webapp that needs to redirect a user to MS auth servers and then back to the site. It would be what you consider a confidential-app and I am specifically choosing not to use 'implicit-token' checkbox in the App settings since that is not recommended.
After the user accepts the consent, the browser needs to be redirected back to the site, so I do need to supply a real redirect-url in addition to make it match with one of the configured urls on the app settings. If you are saying MS will redirect back to my real site even if I specify login.microsoftonline.com, then I am mistaken, I have not tried that.
Now the specific fix is about making it configurable, so that I can pass the same value back when I fetch the token.
My app is not opensource, but it is very similar to your sample
First place I need to use the redirect-url
https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/dev/test/devapps/confidential_auth_code_sample.go#L29
Second place I need to use the redirect-url (this fix)
https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/dev/test/devapps/confidential_auth_code_sample.go#L61
There was a problem hiding this comment.
After discussion we do need to add this back. However we shouldn't place this in the options struct as it makes it appear optional when it should be user-specified (falling back to a default value is incorrect). Can you please move this out of the options struct and make it a parameter so it's clear the caller must supply a value?
|
Kudos, SonarCloud Quality Gate passed!
|
|
@harikb I've asked an MSAL expert to review this change. |
| @@ -129,4 +129,5 @@ type AcquireTokenByDeviceCodeOptions struct { | |||
| type AcquireTokenByAuthCodeOptions struct { | |||
| Code string | |||
| CodeChallenge string | |||
| RedirectURI string | |||
There was a problem hiding this comment.
After discussion we do need to add this back. However we shouldn't place this in the options struct as it makes it appear optional when it should be user-specified (falling back to a default value is incorrect). Can you please move this out of the options struct and make it a parameter so it's clear the caller must supply a value?
There was a problem hiding this comment.
Overall looks good, thanks for contributing. I've changed the location of AcquireTokenByAuthCodeOptions in #104 so it's clear that its shared in between public clients and confidential clients. Once the change when the RedirectUri == "" is made, it should be ready to be merged.
| authParams.Redirecturi = p.RedirectURI | ||
| } | ||
| if authParams.Redirecturi == "" { // set it to default if it is still not set | ||
| authParams.Redirecturi = "https://login.microsoftonline.com/common/oauth2/nativeclient" |
There was a problem hiding this comment.
If RedirectUri == "", lets return an error stating that this is not a valid value. We should not default to "https://login.microsoftonline.com/common/oauth2/nativeclient" as embedded browsers are not yet supported in MSAL Go.
There was a problem hiding this comment.
I propose making this a parameter so it's clear the caller must supply a value. What do you think?
There was a problem hiding this comment.
Oh yes good point. So instead of going in the options struct, it should be in the parameters for both public client auth code and confidential client auth code
There was a problem hiding this comment.
If I make RedirectURI required/argument, I need to figure out what to do in tests like these https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/dev/msal/public_client_application_test.go#L80
I will work on it, but it will take a bit of time. I don't yet know how those mocks work. Thanks
|
Superseded by #188 |
Changes
See also this comment elsewhere for context Azure/azure-sdk-for-go#13565 (comment)
The change retains the ability to have a default RedirectURI while still allowing the caller to set one if available.
@jhendrixMSFT