fix(sdk-auth): to encode request body #1581

tdeekens · 2020-07-04T17:52:36Z

Summary

This pull request fixes the sdk-auth package to encode the request's body at all times.

Description

The sdk-auth is used to perform request authorization in our Node.js SDK. For that it often uses parameters such as scope, refresh_token or grant_type among others.

The parameters mentioned above can often contain strings which need encoding. This holds true for e.g. the <scope>:<projectKey> or also the token itself.

So far the sdk-auth just concatinated strings to build up the request. For example as body += &scope=${scope}. Then sometimes it encodes a parameter as request.body += &refresh_token=${encodeURIComponent(token)}. Our APIs can deal with this inconsistency but it can also cause problems when a string is not rightfully detected as not encoded. This is what I think @cneijenhuis noticed when having problems with token encoding and investigating it.

As a result of this this pull request suggests to always encode the body's paramters as it's send as 'Content-Type': 'application/x-www-form-urlencoded'.

There are usually two options going about it:

Use a library
Use URLSearchParams (web standard)

I propose to use qss which is a tiny library which works both in Node.js and the browser. We have good experience with it in the Merchant Center.
Using URLSearchParams would need a polyfill as it's not supported in all browsers. Usually leading to larger bundles while we do not need most of it's functionality/API surface.

The pull request also updates the tests affected by this which outlines the change.

This overall instead of for example:

grant_type=client_credentials&scope=view_products:sample-project manage_types:sample-project

gives us

grant_type=client_credentials&scope=view_products%3Asample-project%20manage_types%3Asample-project

tdeekens · 2020-07-04T17:54:35Z

packages/sdk-auth/src/auth.js

+    const isNotRefreshTokenGrantType = grantType !== 'refresh_token'
+    const initialBody = encode({
+      grant_type: grantType,
+      ...(disableRefreshToken && { refresh_token: false }),
+      ...(isNotRefreshTokenGrantType && { scope }),
+    })


Building up the initial request encoding all params.

tdeekens · 2020-07-04T17:54:55Z

packages/sdk-auth/src/auth.js

+  static _appendToRequestBody(
+    request: AuthRequest,
+    toAppend: Object
+  ): AuthRequest {
+    const previousDecodedRequestBody = request.body ? decode(request.body) : {}
+    const nextEncdedRequestBody = encode({
+      ...previousDecodedRequestBody,
+      ...toAppend,
+    })
+    request.body = nextEncdedRequestBody
+
+    return request


A small helper reused within the module just to append to the request cause that's what we do most of the time here.

tdeekens · 2020-07-04T17:55:20Z

packages/sdk-auth/src/auth.js

+  static _appendToRequestBody(
+    request: AuthRequest,
+    toAppend: Object
+  ): AuthRequest {


We return the request not just the body here. That makes this easier to compose from my perspective.

I like this change a lot 👍

tdeekens · 2020-07-04T17:55:44Z

packages/sdk-auth/src/auth.js

  anonymousFlow(anonymousId: string = '', config: CustomAuthOptions = {}) {
    const _config = this._getRequestConfig(config)
-    const request = SdkAuth._buildRequest(
+    let request = SdkAuth._buildRequest(


We build the request, then append to its body and get a new request. That's what I mean by composing.

codecov · 2020-07-04T17:56:15Z

Codecov Report

Merging #1581 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1581   +/-   ##
=======================================
  Coverage   98.63%   98.63%           
=======================================
  Files         128      128           
  Lines        3305     3307    +2     
  Branches      761      760    -1     
=======================================
+ Hits         3260     3262    +2     
  Misses         41       41           
  Partials        4        4

Impacted Files	Coverage Δ
packages/sdk-auth/src/auth.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7371b71...2913cfe. Read the comment docs.

tdeekens · 2020-07-04T17:57:37Z

@daern91 please assign anybody else if needed. You seem to be most familiar with this however so maybe a good reviewer 😄.

daern91

Thanks for this one. Much cleaner now with a proper lib and not the hacky template strings 👍

Would also be nice to have the ability to use the, supposedly faster, querystring.stringify native solution in node. As described here: https://github.com/lukeed/qss#qss-

But OTOH it's probably not necessary and can be implemented at a later stage if needed.

daern91 · 2020-07-05T19:02:09Z

packages/sdk-auth/src/auth.js

+    toAppend: Object
+  ): AuthRequest {
+    const previousDecodedRequestBody = request.body ? decode(request.body) : {}
+    const nextEncdedRequestBody = encode({


nitpick typo:
~~nextEncdedRequestBody~~
nextEncodedRequestBody

Thanks. Feel free to use GitHub's review suggestions next time. Helps to get better diffs of what you carefully markdowned yerself :)

daern91 · 2020-07-05T19:03:25Z

packages/sdk-auth/src/auth.js

+  static _appendToRequestBody(
+    request: AuthRequest,
+    toAppend: Object
+  ): AuthRequest {


I like this change a lot 👍

daern91 · 2020-07-05T19:03:36Z

packages/sdk-auth/src/auth.js

    username: string,
    password: string
-  ): string {
+  ): AuthRequest {


Here too 👍

daern91 · 2020-07-05T19:06:20Z

packages/sdk-auth/src/auth.js


-    if (anonymousId) request.body += `&anonymous_id=${anonymousId}`
+    if (anonymousId)
+      request = SdkAuth._appendToRequestBody(request, {


I guess we could technically leave request as a constant and return here instead of re-assigning and returning below. Just a matter of flavor tho.

Yes, didn't want to change such things much in this PR. Generally the composition in this code can be improve. Ideally, the AuthRequeust could have a map and value function. Then you could just request.map(SdkAuth._appendToRequestBody(...)) which gives you a mapped request and then value() when you pass it somewhere. That's another story though.

daern91 · 2020-07-05T19:07:59Z

packages/sdk-auth/src/auth.js

-      username,
-      password
-    )
+    request = SdkAuth._appendUserCredentialsToBody(request, username, password)


same with this one, but might just make things too nested and unreadable.

daern91 · 2020-07-05T19:08:44Z

packages/sdk-auth/src/auth.js

-      request.body = SdkAuth._appendUserCredentialsToBody(
-        request.body,
+      request = SdkAuth._appendUserCredentialsToBody(
+        request,


I see, we keep this pattern throughout 👍

The conditionals are a bit hard to improve without better patterns of compositions. I will adjust those which don't involve them.

daern91 · 2020-07-05T19:15:23Z

Adding @commercetools/clients-team as FYI

cneijenhuis · 2020-07-06T07:23:26Z

Encoding LGTM 👍

tdeekens · 2020-07-06T07:27:33Z

Would also be nice to have the ability to use the, supposedly faster, querystring.stringify native solution in node. As described here: lukeed/qss#qss-

That's a Node.js utility. We also run in the browser. Given we encode three things - as you mentioned - I don't see this, comparing to other impacts, relevant for performance of this lib. With 100k ops yes, not with 3.

Thanks for this one. Much cleaner now with a proper lib and not the hacky template strings 👍

I am still not super happy with this. Encapsulation is sub-optimal:

The AuthRequest should have a map and value and sort of act as a Functor
The encoding chould be "inside of" it. Not part of the surrouding code

Still this is better than before and step-wise is how to improve things 🐱.

daern91 · 2020-07-07T08:06:26Z

@jenschude any comments on this one before we merge?

jenschude

Just a comment about the testing part

jenschude · 2020-07-07T08:20:06Z

packages/sdk-auth/test/client-password-flow.spec.js

-        `grant_type=password&scope=manage_project:${config.projectKey}` +
-        `&username=user%204%5El*aJ%40ETso%2B%2F%5CHdE1!x0u4q5` + // encoded
-          `&password=pass%204%5El*aJ%40ETso%2B%2F%5CHdE1!x0u4q5` // encoded
+        encode({


Also it may make the test more readable you are testing the encoding with the function to encode itself. I'm pretty sure that this works correct but a non working the encode function would result in a still working test.

But I also can see that the encode function may not produce a canonical string to compare it to a static string.

Good point. ~~Let me suggest something later :)~~

I played around a bit. Somehow having the encode feels alright to me. The nock library will throw if a mock is not defined. When expecating on a string I would use Inline Snapshots.

To be on the safe side you could use https://nodejs.org/api/querystring.html#querystring_querystring_encode

Yet another good point 💋. Thanks for being persistent. Here we go: 2913cfe.

I take the thumbs up as an approval.

fix(sdk-auth): to encode request body

e08687f

tdeekens commented Jul 4, 2020

View reviewed changes

tdeekens requested review from cneijenhuis and daern91 July 4, 2020 17:56

tdeekens self-assigned this Jul 4, 2020

tdeekens added Status: Review Type: Bug labels Jul 4, 2020

tdeekens marked this pull request as ready for review July 4, 2020 17:57

daern91 approved these changes Jul 5, 2020

View reviewed changes

daern91 requested a review from a team July 5, 2020 19:15

refactor(sdk-auth): improve code readability

d15aca5

jenschude reviewed Jul 7, 2020

View reviewed changes

refactor(sdk-auth): to use querystring under test

2913cfe

tdeekens merged commit 0e9ca25 into master Jul 7, 2020

tdeekens deleted the fix/sdk-auth-encoding branch July 7, 2020 11:39

harm-meijer mentioned this pull request Jul 13, 2020

How to use node sdk-auth to get token after error #1528

Closed

fix(sdk-auth): to encode request body #1581

fix(sdk-auth): to encode request body #1581

Uh oh!

Conversation

tdeekens commented Jul 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tdeekens commented Jul 4, 2020

Uh oh!

daern91 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daern91 commented Jul 5, 2020

Uh oh!

cneijenhuis commented Jul 6, 2020

Uh oh!

tdeekens commented Jul 6, 2020

Uh oh!

daern91 commented Jul 7, 2020

Uh oh!

jenschude left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdeekens Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tdeekens commented Jul 4, 2020 •

edited

Loading

codecov bot commented Jul 4, 2020 •

edited

Loading

tdeekens Jul 7, 2020 •

edited

Loading