Skip to content

Pass params submitted to launch-uri to authorization-uri#42

Open
sbauch wants to merge 6 commits intoweavejester:masterfrom
enterprise-oss:authorization-url-dynamic-params
Open

Pass params submitted to launch-uri to authorization-uri#42
sbauch wants to merge 6 commits intoweavejester:masterfrom
enterprise-oss:authorization-url-dynamic-params

Conversation

@sbauch
Copy link
Copy Markdown

@sbauch sbauch commented Feb 10, 2021

Some OAuth providers support extra query params that aren't appropriate for hardcoding. One example is Google OAuth, which accepts an hd param to the authorization URL that can restrict logins to users with the provided email domain.

This allows a developer to submit a request to the launch-uri with parameters. These parameters will now be passed along to the authorization URI as the user is sent to the OAuth provider to authenticate.

Also adds a test for this functionality.

#41

I'm definitely out of my element here! I ran a linter and it didnt catch anything but I'm not sure I handled indentation correctly.

Some OAuth providers support extra query params that aren't appropriate for hardcoding. One example is Google OAuth, which accepts an `hd` param to the authorization URL that can restrict logins to users with the provided email domain.

This commit allows a developer to submit a request to the launch-uri with parameters. These parameters will now be passed along to the authorization URI as the user is sent to the OAuth provider to authenticate.

Also adds a test for this functionality.

weavejester#41
Comment thread test/ring/middleware/oauth2_test.clj Outdated
@sbauch
Copy link
Copy Markdown
Author

sbauch commented Feb 17, 2021

would love to get some 👀 on this if you have some bandwidth! we have some folks starting to use our fork and I'm not terribly confident this is up to snuff 🙈

@weavejester
Copy link
Copy Markdown
Owner

Rather than using wrap-params, instead we should pull out the query-string and append it to the URL being generated.

Comment thread src/ring/middleware/oauth2.clj Outdated
(defn- authorize-uri [profile request state]
(str (:authorize-uri profile)
(if (.contains ^String (:authorize-uri profile) "?") "&" "?")
(request :query-string)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

a little unsure how to handle cases where there is no query string, though the tests are green so perhaps this is enough?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd suggest changing this to: (:query-string request). In general that's more robust.

Comment thread src/ring/middleware/oauth2.clj Outdated
@sbauch
Copy link
Copy Markdown
Author

sbauch commented Feb 24, 2021

I realized that my previous approach wasn't including an & between the params from the querystring and the other params we're adding.

I improved the test and patched the issue, but I'm less and less confident here and would appreciate some help getting it over the finish line from someone with a bit more clojure experience 🙏

@sbauch
Copy link
Copy Markdown
Author

sbauch commented Apr 7, 2021

@weavejester hate to bother but could you take another look here? 🙏

@weavejester
Copy link
Copy Markdown
Owner

This looks fine. Can you squash down your commits and make sure the commit message is okay?

@weavejester
Copy link
Copy Markdown
Owner

Just chasing this up: do you have time to squash the commits and make the commit message:

Relay params from launch URI to authorization URI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants