-
Notifications
You must be signed in to change notification settings - Fork 7
Allow all oauth2 authorize params to be passed while using login. #171
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
Conversation
There appears to be a request for the internal fix as well. |
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.
Linting check failed 😖. It looks like it doesn't like that there isn't a new line at the very end of packages/sdk-react/package.json
. It could be fixed with yarn format:fix
Other than that, this looks like it'll do as expected and pass along arbitrary arguments from authParams
to FA 👍
@@ -7,3 +7,12 @@ export const TEST_CONFIG: FusionAuthProviderConfig = { | |||
scope: 'openid email profile offline_access', | |||
postLogoutRedirectUri: 'http://localhost', | |||
}; | |||
|
|||
export const TEST_AUTHPARAM_CONFIG: FusionAuthProviderConfig = { |
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.
If possible, I would recommend consolidating this object with the one above because they are almost the same--maybe someone will be confused by this in the future. Perhaps we could strip the authParams
property off or ignore it in tests where appropriate. Definitely not a big deal though, and it's great that that property is tested for!
@@ -24,6 +24,11 @@ export interface SDKConfig { | |||
*/ | |||
scope?: string; | |||
|
|||
/** | |||
* Additional params passed to loginPath typically `/app/login/`, which redirects to `/oauth2/authorize`. Example of this might be loginParams = [{idp_hint:'44449786-3dff-42a6-aac6-1f1ceecb6c46'}] or any params found at https://fusionauth.io/docs/lifecycle/authenticate-users/oauth/endpoints |
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.
Good call adding a link to FA docs in this comment. I find that kind of thing helpful as a user of someone else's libraries.
What is this PR and why do we need it?
An issue with similar request is from #164. In diving deeper this might rely on an internal update to the endpoint as well, as it appears not all params are being passed. Will confirm with internal team before we push this fix.
Pre-Merge Checklist (if applicable)