-
Notifications
You must be signed in to change notification settings - Fork 44
add support for provider_query_params option on getAuthorizationUrl
#1308
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 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.
Greptile Summary
This PR adds support for a new provider_query_params option in the getAuthorizationUrl method, allowing developers to pass additional query parameters to identity providers during the authorization flow. The implementation replaces the existing URL parameter handling with the qs library to better support complex object parameters while maintaining backward compatibility.
Key changes:
- Added
providerQueryParamsoption toUserManagementAuthorizationURLOptionsinterface - Switched to
qslibrary (v6.14.0) for query string generation - Refactored
toQueryStringfunction to useqs.stringifywith specific options to maintain backward compatibility - Added comprehensive test coverage for the new feature
The change is well-tested through existing snapshot tests that verify URL generation remains consistent with previous behavior.
Confidence score: 4/5
- This PR is safe to merge with normal review attention
- Score of 4 given due to solid test coverage and backward compatibility, with minor concern about potential edge cases in query parameter handling
- Files needing attention:
- src/user-management/user-management.ts - Verify
qs.stringifyoptions are correctly configured - src/user-management/interfaces/authorization-url-options.interface.ts - Ensure type constraints are appropriate for provider parameters
- src/user-management/user-management.ts - Verify
4 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
| domainHint?: string; | ||
| loginHint?: string; | ||
| provider?: string; | ||
| providerQueryParams?: Record<string, string | boolean | number>; |
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.
style: Consider adding JSDoc comment explaining the purpose and expected format of providerQueryParams, similar to the context param above
| "leb": "^1.0.0", | ||
| "pluralize": "8.0.0" | ||
| "pluralize": "8.0.0", | ||
| "qs": "6.14.0" |
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 avoid adding the new package qs here if it's possible to use the native URLSearchParams (to avoid extra packages installed in the SDK).
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.
the native URLSearchParams doesn't have good support for array or object parameters, like provider_scopes and now provider_query_params. writing our own logic for array params was ok, but I was worried about missing edge cases if we tried to write our own logic to handle objects so I thought this was worth the added dependency.
## Description version bump that includes: - #1307 - #1308 ## Documentation Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
|
Hey @stacurry Not sure if this is a bug or not, but I noticed that after updating to this version, the authorization urls generated with a query parameter with So generating a url previously like this: Would result in a url like this: (where the Whereas now, the same call would return this URL: (note the empty Which results in an error page. I fixed it temporarily by conditionally passing the But if this new behaviour is intended, it should have been marked as a breaking change I think. |
|
Yeah, my bad. The value that was getting passed was I fixed it in my code to ensure that only Thanks for checking this out. |
Description
qsto create query strings forgetAuthorizationUrlto more easily support a new object type parameter and stringify it into the format our API expectsgetAuthorizationUrl(sso and user_management) that compare full url strings to snapshots, so the fact that all of these tests still pass (withqsoptions set to maintain backwards compatibility with our previous implementation) makes this feel pretty safe to me, but if anyone disagrees, let me knowprovider_query_paramsquery parameter / option added in https://github.com/workos/workos/pull/41556Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.