-
Notifications
You must be signed in to change notification settings - Fork 99
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
Updates for Beta: Adding scopes and removing unsupported methods #99
Conversation
src/api/oauth.ts
Outdated
client: AxiosInstance, | ||
) { | ||
requireArgs({ client_id }); | ||
|
||
const params = { response_type, client_id }; | ||
if (redirect_uri) params["redirect_uri"] = redirect_uri; | ||
if (state) params["state"] = state; | ||
if (scope) params["scope"] = scope; | ||
if (scopes && scopes.length > 0) params["scope"] = scopes.join(" "); |
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.
Should we join these with the "+" or "%20" delimiter?
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 + is added automatically since spaces in URLs get encoded as the "+" character. The results of the current implementation gives them a URL of: "https://test.com/oauth/authorize?response_type=code&client_id=6287ec36a841b25637c663de&state=1234567890&scope=assets:read+cms:write"
Which I think is what we want. I've added a test that validates this here.
src/api/oauth.ts
Outdated
@@ -52,15 +52,15 @@ export class OAuth { | |||
* @returns The URL to authorize a user | |||
*/ | |||
static authorizeUrl( | |||
{ response_type = "code", redirect_uri, client_id, state, scope }: IAuthorizeUrlParams, | |||
{ response_type = "code", redirect_uri, client_id, state, scopes }: IAuthorizeUrlParams, |
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.
One breaking change to callout here @dpim is the removal of the scope
param in favor of supporting a scopes
param. With scopes
users can pass an array of the supported scopes into 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.
I would be supportive of offering an optional helper function to pass an array of scope strings that resolves to ?scope=scope1 scope2 scope3
, if that's what you're proposing.
We may also want to support passing the raw scope string with scope
| "deleteItems" | ||
| "publishItems"; | ||
|
||
export const SCOPES_ARRAY = [ |
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.
We may also want to add scopes for users:read
, users:write
, ecommerce:read
and ecommerce:write
No description provided.