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

Pass resource parameter in login url #753

Merged
merged 9 commits into from
Sep 29, 2020

Conversation

codablock
Copy link
Contributor

@codablock codablock commented Sep 3, 2020

Description

This passes the value given by --resource=<my-resource> to the login url.

Motivation and Context

Without this, Azure will always return access tokens for graph.microsoft.com, which is not what one would expect when --resource is used. The modified method is not Azure specific, but I would assume that the resource parameter should also be present in other providers.

How Has This Been Tested?

We have this code (based on a master version shortly after 6.0.0 was released) running in production since a few weeks.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@codablock codablock requested a review from a team as a code owner September 3, 2020 12:32
@codablock codablock mentioned this pull request Sep 7, 2020
3 tasks
@@ -90,6 +90,9 @@ func (p *ProviderData) GetLoginURL(redirectURI, state string) string {
params.Set("client_id", p.ClientID)
params.Set("response_type", "code")
params.Add("state", state)
if p.ProtectedResource != nil && p.ProtectedResource.String() != "" {
Copy link
Member

Choose a reason for hiding this comment

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

This is already a string type and defaults to "". You can simplify this logic and mirror this line's format: if p.AcrValues != "" {

Can you add a unit test for this addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of current master, this is of type *url.URL, so I need to verify that it is not nil before checking for emptyness. I'll try to add a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

Ack - you are right! I was looking at the options ProtectedResource that made it - thanks!

Copy link
Member

Choose a reason for hiding this comment

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

OK - I had a chance to research this more:

I think since this is a flag that is documented as Azure only, it shouldn't be added on the default GetLoginURL method, it should be under azure.go (which means making a GetLoginURL under AzureProvider)

...but I see a commit from 5 years ago where it was added to the default Redeem even though this is only set by the AzureProvider and the AzureProvider implements its own Redeem method. 🤷‍♂️

Can you take a try at getting this completely Azure specific?

Otherwise we can do it as you've listed here (having it part of defaults for both Redeem & GetLoginURL) -- I'd just like to update the documentation for the --resource flag that it applies to all providers and remove the note that it is an Azure only flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I was not aware that the --resource flag was meant to be Azure specific. I moved the code into azure.go

Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

I don't know if any of the maintainers are Azure users -- can you point me at some Azure documentation of what this does?

And share some sample config values you have for your environment for this flag so I can better visualize?

@codablock
Copy link
Contributor Author

codablock commented Sep 10, 2020

@NickMeves I've added a unit test for the resource=xxx parameter.

Unfortunately it is a huge mess to navigate through Azure AD documentation and keep track of all the details and how stuff is related to other stuff...so I have no idea anymore where I got this info from. Googling for it I found this Stackoverflow entry which describes the resource parameter in the selected answer, but also fails to mention the documentation that actually describes it.

@codablock codablock force-pushed the azure-resource branch 3 times, most recently from b42940d to 06ea1f6 Compare September 14, 2020 11:57
@codablock
Copy link
Contributor Author

@NickMeves I've refactored the default_provider.go GetLoginURL in a way so that it can be re-used by other providers. Azure now uses the new DefaultGetLoginURL and then adds azure specific stuff to the params. And just for fun, I used the same method to de-duplicate the logingov provider.

@@ -73,8 +73,7 @@ func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (s
return
}

// GetLoginURL with typical oauth parameters
func (p *ProviderData) GetLoginURL(redirectURI, state string) string {
func DefaultGetLoginURL(p *ProviderData, redirectURI, state string) (url.URL, url.Values) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a step in the right direction 👍

Let's refactor it a bit. Let's move this over to providers/util.go with the other provider helpers and call it makeLoginURL or something.

It might be over-optimization if it is only used in a couple places, but I'm not a fan of the helper returning the raw url.URL, url.Values and requiring the consumers to know to do this:

	a, params := DefaultGetLoginURL(p.ProviderData, redirectURI, state)
	// CUSTOMIZE PARAMS HERE
	a.RawQuery = params.Encode()
	return a.String()

Maybe make a light struct around this with a few helper methods to do what we need as far as param add/set + String()

Copy link
Contributor Author

@codablock codablock Sep 15, 2020

Choose a reason for hiding this comment

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

I came up with an alternative form of the method that simply requires to pass extraParams, which should make it very clear for the caller what is expected. Pushed 2 commits with the related changes.

@NickMeves
Copy link
Member

@codablock Just an FYI - I need to think about the repercussions more, but I think this and your other PR might barely hover on the breaking change threshold between requiring a minor vs major release to get this functionality out. (for instance --cookie-refresh & the session validation never did anything in the past for Azure - but it wasn't documented that it wasn't Azure supported, so users might've set flags and not realized they were no-ops and then starting to care about them might break setups).

But the point might be moot - I'm chatting with @JoelSpeed about our next release potentially being v7 since we are starting to get to a critical mass of PRs that need a major release.

We'll keep you posted.

@codablock
Copy link
Contributor Author

@NickMeves I'm fine with this being a major release and thus needing more time. We're already running oauth2-proxy from a forked branch with the necessary changes merged.

@NickMeves NickMeves added breaking A change that will cause a major version bump enhancement labels Sep 15, 2020
NickMeves
NickMeves previously approved these changes Sep 19, 2020
Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

Thanks for the changes & refactoring. Looks good!

Please go ahead and add a CHANGELOG entry.

Per our discussion, I'm going to wait to merge this until we are positive our next release is a major (its looking like it, though).

codablock added a commit to codablock/oauth2-proxy that referenced this pull request Sep 21, 2020
@codablock
Copy link
Contributor Author

@NickMeves please see #787 for CHANGELOG.

codablock added a commit to codablock/oauth2-proxy that referenced this pull request Sep 21, 2020
@NickMeves
Copy link
Member

I'm ready to merge this one -- can you get the CHANGELOG entry over in this PR and not split out and rebase?

@JoelSpeed wanted to take a peek at #754 still

@JoelSpeed
Copy link
Member

I agree, let's get this PR rebased, add the changelog into this PR and then we can merge, it's much easier to look back at changes if the changelog entry is in the same PR

Do we need to update the docs at all?

…ltGetLoginURL

This allows us to reuse code from different providers in case slight
modifications to the URL are needed.
Also add unit test to ensure logingov specific logic is applied.
And don't require the caller to know how to use the returned params.
@codablock
Copy link
Contributor Author

Rebased on master and added changelog entry for this PR.

Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

1 last NIT on the CHANGELOG then we're good to go!

CHANGELOG.md Outdated
@@ -12,9 +12,14 @@
## Breaking Changes

- [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) When a Redis session store is configured, OAuth2-Proxy will fail to start up unless connection and health checks to Redis pass
- A bug in the Azure provider prevented it from properly passing the configured protected `--resource`
Copy link
Member

Choose a reason for hiding this comment

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

Can you add in the PR link to associate with this breaking change note to tie it to the CHANGELOG entry below?

Once that's in, I'll merge 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickMeves done

Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that will cause a major version bump enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants