fix(authorization): authorization url encoding #3062
Conversation
hassan254-prog
changed the title
| let authorizationUri = simpleOAuthClient.authorizeURL({ | ||
| redirect_uri: callbackUrl, | ||
| scope: providerConfig.oauth_scopes ? providerConfig.oauth_scopes.split(',').join(provider.scope_separator || ' ') : '', | ||
| scope: scopes, | ||
| state: session.id, | ||
| ...allAuthParams | ||
| }); |
There was a problem hiding this comment.
Might be easier at some point to not use authorizeURL and just iterate over the query params
There was a problem hiding this comment.
Directly manipulating query parameters provides for a more controlled approach.
| newQuery = newQuery ? `${newQuery}&scope=${scopes}` : `scope=${scopes}`; | ||
| } | ||
| url.search = newQuery; | ||
| authorizationUri = url.toString(); |
There was a problem hiding this comment.
can this be simplified like this?
const url = new URL(authorizationUri);
url.searchParams.delete('scope');
if (scopes) {
url.searchParams.set('scope', scopes);
}
authorizationUri = url.toString();There was a problem hiding this comment.
Yes, the simplification works functionally, but it will still URL-encode the scopes value.
| @@ -4469,6 +4470,8 @@ pennylane: | |||
| grant_type: authorization_code | |||
| refresh_params: | |||
| grant_type: refresh_token | |||
| authorization_url_skip_encode: | |||
| - scopes | |||
There was a problem hiding this comment.
I would personally prefer to bundle it with the authorization_url or token_url attributes. Something like:
authorization_url:
url: https://app.pennylane.com/oauth/authorize
skip_encode: scopes
but it would require changing the type of an existing attributes 😓
There was a problem hiding this comment.
I agree. I'm not sure, but to me, this might be the best time to clean up and bundle all related fields together for both authorization_url and token_url. Currently, we have loosely grouped fields for authorization_url, such as:
authorization_url_replacements
authorization_url_fragment
Bundling them together would make the providers.yaml file cleaner. @TBonnin what do you think?
There was a problem hiding this comment.
grouping them together would make sense. Not in this PR of course. Maybe adding an
authorization:
url: ...
url_replacements:...
...
and same thing with token. We would need to ensure backward compatibilities though. 😓
hassan254-prog
deleted the
kelvinwari/ext-258-investigate-on-pennylane
branch
Describe the problem and your solution
Turns out some of the providers don't like it when we encode some of the url parts, i.e
scopes,base_urlwhen we are interpolating withconnectionConfigs. To help with this, the PR introduces a way we can handle this within the various parts. Currently we can skip authorization_url encoding within thescopes, or when interpolating thebase_urlwithconnectionConfig.Testing instructions
When creating a new connection for
Pennylane, the scopes aren't encodedcustomer_invoices+supplier_invoices+ledger+accounting