-
Notifications
You must be signed in to change notification settings - Fork 39
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 params submitted to launch-uri to authorization-uri #42
base: master
Are you sure you want to change the base?
Pass params submitted to launch-uri to authorization-uri #42
Conversation
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
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 🙈 |
Rather than using |
src/ring/middleware/oauth2.clj
Outdated
@@ -19,6 +19,7 @@ | |||
(defn- authorize-uri [profile request state] | |||
(str (:authorize-uri profile) | |||
(if (.contains ^String (:authorize-uri profile) "?") "&" "?") | |||
(request :query-string) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I realized that my previous approach wasn't including an 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 🙏 |
@weavejester hate to bother but could you take another look here? 🙏 |
This looks fine. Can you squash down your commits and make sure the commit message is okay? |
Just chasing this up: do you have time to squash the commits and make the commit message:
|
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.