Skip to content

Support for setting redirect-url in token-fetch portion of confidential-app authentication code flow #96

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.

Sign up for GitHub

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

Closed
harikb wants to merge 6 commits into from
Closed

Support for setting redirect-url in token-fetch portion of confidential-app authentication code flow #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
175c883
Fix link to samples directory
harikb Dec 6, 2020
dc17d21
go get needs an import path. otherwise the command fails
harikb Dec 6, 2020
6f39f0f
Add RedirectURI as another option in AuthCodeOptions
harikb Dec 6, 2020
ff332bb
AuthCodeParameters, created above with some defaults will now be fill…
harikb Dec 6, 2020
e0c75bb
Add a RedirectURI parameter to acquireTokenAuthCodeParameters. Use pr…
harikb Dec 6, 2020
7d446d8
Removing -u argument
harikb Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ The MSAL library for Go is part of the [Microsoft identity platform for develope

Quick links:

| [Getting Started](https://docs.microsoft.com/azure/active-directory/develop/#quickstarts) | [GoDoc](https://godoc.org/github.com/AzureAD/microsoft-authentication-library-for-go/msal) | [Wiki](https://github.com/AzureAD/microsoft-authentication-library-for-go/wiki) | [Samples](https://github.com/AzureAD/microsoft-authentication-library-for-go/tree/dev/examples) | [Support](README.md#community-help-and-support) |
| [Getting Started](https://docs.microsoft.com/azure/active-directory/develop/#quickstarts) | [GoDoc](https://godoc.org/github.com/AzureAD/microsoft-authentication-library-for-go/msal) | [Wiki](https://github.com/AzureAD/microsoft-authentication-library-for-go/wiki) | [Samples](https://github.com/AzureAD/microsoft-authentication-library-for-go/tree/test/devapps) | [Support](README.md#community-help-and-support) |
| ------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------ |

## Build Status
Expand All @@ -19,7 +19,7 @@ Quick links:
To install Go, visit [this link](https://golang.org/dl/).

### Installing MSAL Go
`go get -u github.com/AzureAD/microsoft-authentication-library-for-go/`
`go get github.com/AzureAD/microsoft-authentication-library-for-go/msal`

## Usage
Before using MSAL Go, you will need to [register your application with the Microsoft identity platform](https://docs.microsoft.com/azure/active-directory/develop/quickstart-v2-register-an-app).
Expand Down
8 changes: 7 additions & 1 deletion msal/acquire_token_auth_code_parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type acquireTokenAuthCodeParameters struct {
CodeChallenge string
clientCredential msalbase.ClientCredential
requestType requests.AuthCodeRequestType
RedirectURI string
}

// createAcquireTokenAuthCodeParameters creates an AcquireTokenAuthCodeParameters instance.
Expand All @@ -30,6 +31,11 @@ func createAcquireTokenAuthCodeParameters(scopes []string) *acquireTokenAuthCode

func (p *acquireTokenAuthCodeParameters) augmentAuthenticationParameters(authParams *msalbase.AuthParametersInternal) {
p.commonParameters.augmentAuthenticationParameters(authParams)
authParams.Redirecturi = "https://login.microsoftonline.com/common/oauth2/nativeclient"
if p.RedirectURI != "" {
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"
Copy link
Contributor

@sangonzal sangonzal Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jhendrixMSFT jhendrixMSFT Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose making this a parameter so it's clear the caller must supply a value. What do you think?

Copy link
Contributor

@sangonzal sangonzal Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@harikb harikb Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
authParams.AuthorizationType = msalbase.AuthorizationTypeAuthCode
}
1 change: 1 addition & 0 deletions msal/confidential_client_application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func (cca *ConfidentialClientApplication) AcquireTokenByAuthCode(ctx context.Con
if options != nil {
authCodeParams.Code = options.Code
authCodeParams.CodeChallenge = options.CodeChallenge
authCodeParams.RedirectURI = options.RedirectURI
}
return cca.clientApplication.acquireTokenByAuthCode(ctx, authCodeParams)

Expand Down
1 change: 1 addition & 0 deletions msal/public_client_application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,5 @@ type AcquireTokenByDeviceCodeOptions struct {
type AcquireTokenByAuthCodeOptions struct {
Code string
CodeChallenge string
RedirectURI string
Copy link
Contributor

@jhendrixMSFT jhendrixMSFT Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmprieur can you please comment on this? I explicitly removed this in #68 per your request.

Copy link
Author

@harikb harikb Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jhendrixMSFT jhendrixMSFT Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@harikb harikb Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@jhendrixMSFT jhendrixMSFT Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}