Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 7, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e08687f
fix(sdk-auth): to encode request body
tdeekens Jul 4, 2020
d15aca5
refactor(sdk-auth): improve code readability
tdeekens Jul 6, 2020
2913cfe
refactor(sdk-auth): to use querystring under test
tdeekens Jul 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/sdk-auth/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
},
"dependencies": {
"@commercetools/sdk-middleware-http": "^6.0.7",
"lodash.defaultsdeep": "^4.6.0"
"lodash.defaultsdeep": "^4.6.0",
"qss": "2.0.3"
},
"devDependencies": {
"nock": "12.0.3",
Expand Down
82 changes: 49 additions & 33 deletions packages/sdk-auth/src/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {
AuthRequest,
UserAuthOptions,
} from 'types/sdk'
import { decode, encode } from 'qss'
import defaultsDeep from 'lodash.defaultsdeep'
import { getErrorByCode } from '@commercetools/sdk-middleware-http'
import * as constants from './constants'
Expand Down Expand Up @@ -106,11 +107,28 @@ export default class SdkAuth {
config.token || SdkAuth._encodeClientCredentials(credentials)
const authType = config.authType || constants.DEFAULT_AUTH_TYPE

let body = `grant_type=${grantType}`
if (grantType !== 'refresh_token') body += `&scope=${scope}`
if (disableRefreshToken === true) body += '&refresh_token=false'
const isNotRefreshTokenGrantType = grantType !== 'refresh_token'
const initialBody = encode({
grant_type: grantType,
...(disableRefreshToken && { refresh_token: false }),
...(isNotRefreshTokenGrantType && { scope }),
})
Comment on lines +110 to +115
Copy link
Contributor Author

@tdeekens tdeekens Jul 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building up the initial request encoding all params.


return { basicAuth, authType, uri, body: initialBody, headers }
}

static _appendToRequestBody(
request: AuthRequest,
toAppend: Object
): AuthRequest {
Copy link
Contributor Author

@tdeekens tdeekens Jul 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@daern91 daern91 Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change a lot 👍

const previousDecodedRequestBody = request.body ? decode(request.body) : {}
const nextEncodedRequestBody = encode({
...previousDecodedRequestBody,
...toAppend,
})
request.body = nextEncodedRequestBody

return { basicAuth, authType, uri, body, headers }
return request
}

_process(request: AuthRequest) {
Expand Down Expand Up @@ -159,29 +177,25 @@ export default class SdkAuth {
return SdkAuth._parseResponseJson(response).then((jsonResponse) => {
if (SdkAuth._isErrorResponse(response))
throw SdkAuth._createResponseError(jsonResponse, uri, response.status)

return SdkAuth._enrichTokenResponse(jsonResponse)
})
}

static _appendUserCredentialsToBody(
body: string,
request: AuthRequest,
username: string,
password: string
): string {
): AuthRequest {
Copy link
Contributor

@daern91 daern91 Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too 👍

if (!(username && password))
throw new Error('Missing required user credentials (username, password)')

return [
body,
`username=${encodeURIComponent(username)}`,
`password=${encodeURIComponent(password)}`,
]
.filter(Boolean)
.join('&')
return SdkAuth._appendToRequestBody(request, { username, password })
}

static _enrichUriWithProjectKey(uri: string, projectKey: ?string): string {
if (!projectKey) throw new Error('Missing required option (projectKey)')

return uri.replace('--projectKey--', projectKey)
}

Expand Down Expand Up @@ -216,21 +230,26 @@ export default class SdkAuth {

anonymousFlow(anonymousId: string = '', config: CustomAuthOptions = {}) {
const _config = this._getRequestConfig(config)
const request = SdkAuth._buildRequest(
let request = SdkAuth._buildRequest(
Copy link
Contributor Author

@tdeekens tdeekens Jul 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_config,
SdkAuth._enrichUriWithProjectKey(
this.ANONYMOUS_FLOW_URI,
_config.projectKey
)
)

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

@daern91 daern91 Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tdeekens tdeekens Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

anonymous_id: anonymousId,
})

return this._process(request)
}

clientCredentialsFlow(config: CustomAuthOptions = {}) {
const _config = this._getRequestConfig(config)
const request = SdkAuth._buildRequest(_config, this.BASE_AUTH_FLOW_URI)

return this._process(request)
}

Expand All @@ -240,13 +259,9 @@ export default class SdkAuth {
url: string
) {
const { username, password } = credentials || {}
const request = SdkAuth._buildRequest(config, url, 'password')
let request = SdkAuth._buildRequest(config, url, 'password')

request.body = SdkAuth._appendUserCredentialsToBody(
request.body,
username,
password
)
request = SdkAuth._appendUserCredentialsToBody(request, username, password)
Copy link
Contributor

@daern91 daern91 Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return this._process(request)
}
Expand All @@ -269,29 +284,30 @@ export default class SdkAuth {
config: CustomAuthOptions = {}
) {
const _config = this._getRequestConfig(config)

return this._passwordFlow(credentials, _config, this.BASE_AUTH_FLOW_URI)
}

refreshTokenFlow(token: string, config: CustomAuthOptions = {}) {
if (!token) throw new Error('Missing required token value')
const _config = this._getRequestConfig(config)

const request = SdkAuth._buildRequest(
_config,
this.BASE_AUTH_FLOW_URI,
'refresh_token'
const _config = this._getRequestConfig(config)
const request = SdkAuth._appendToRequestBody(
SdkAuth._buildRequest(_config, this.BASE_AUTH_FLOW_URI, 'refresh_token'),
{ refresh_token: token }
)
request.body += `&refresh_token=${encodeURIComponent(token)}`

return this._process(request)
}

introspectToken(token: string, config: CustomAuthOptions = {}) {
const _config = this._getRequestConfig(config)
if (!token) throw new Error('Missing required token value')

const request = SdkAuth._buildRequest(_config, this.INTROSPECT_URI)
request.body = `token=${encodeURIComponent(token)}`
const _config = this._getRequestConfig(config)
const request = SdkAuth._appendToRequestBody(
SdkAuth._buildRequest(_config, this.INTROSPECT_URI),
{ token }
)

return this._process(request)
}
Expand All @@ -313,12 +329,12 @@ export default class SdkAuth {
headers,
})

const request = SdkAuth._buildRequest(_config, uri)
let request = SdkAuth._buildRequest(_config, uri)
request.body = body || '' // let user to build their own body

if (credentials)
request.body = SdkAuth._appendUserCredentialsToBody(
request.body,
request = SdkAuth._appendUserCredentialsToBody(
request,
Copy link
Contributor

@daern91 daern91 Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, we keep this pattern throughout 👍

Copy link
Contributor Author

@tdeekens tdeekens Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

credentials.username,
credentials.password
)
Expand Down
35 changes: 23 additions & 12 deletions packages/sdk-auth/test/client-password-flow.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import nock from 'nock'
import { encode } from 'qss'
import Auth from '../src/auth'
import config from './resources/sample-config'
import response from './resources/sample-response.json'
Expand All @@ -11,12 +12,15 @@ describe('Client Password flow', () => {

test('should authenticate with correct user credentials', async () => {
const scope = nock(config.host)
.post(`/oauth/token`, {
grant_type: 'password',
scope: `manage_project:${config.projectKey}`,
username: 'user123',
password: 'pass123',
})
.post(
`/oauth/token`,
encode({
grant_type: 'password',
scope: `manage_project:${config.projectKey}`,
username: 'user123',
password: 'pass123',
})
)
.reply(200, JSON.stringify(response))

expect(scope.isDone()).toBe(false)
Expand All @@ -40,9 +44,12 @@ describe('Client Password flow', () => {
.post(
`/oauth/token`,
// expected body
`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({
Copy link
Contributor

@jenschude jenschude Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tdeekens tdeekens Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jenschude jenschude Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@tdeekens tdeekens Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@tdeekens tdeekens Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take the thumbs up as an approval.

grant_type: 'password',
scope: `manage_project:${config.projectKey}`,
username: userCredentials.username,
password: userCredentials.password,
})
)
.reply(200, JSON.stringify(response))

Expand All @@ -63,9 +70,13 @@ describe('Client Password flow', () => {
const scope = nock(config.host)
.post(
`/oauth/token`,
// expected body
`grant_type=password&scope=manage_project:${config.projectKey}` +
`&refresh_token=false&username=user&password=pass`
encode({
grant_type: 'password',
refresh_token: false,
scope: `manage_project:${config.projectKey}`,
username: 'user',
password: 'pass',
})
)
.reply(200, JSON.stringify(response))

Expand Down
12 changes: 9 additions & 3 deletions packages/sdk-auth/test/common.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import nock from 'nock'
import fetch from 'node-fetch'
import { encode } from 'qss'
import { getErrorByCode } from '@commercetools/sdk-middleware-http'
import Auth from '../src/auth'
import config from './resources/sample-config'
Expand All @@ -14,7 +15,10 @@ describe('Common processes', () => {
const request = {
basicAuth,
uri: 'https://auth.commercetools.com/api-endpoint',
body: 'grant_type=client_credentials&scope=manage_project:project-key',
body: encode({
grant_type: 'client_credentials',
scope: 'manage_project:project-key',
}),
}

beforeEach(() => nock.cleanAll())
Expand Down Expand Up @@ -220,8 +224,10 @@ describe('Common processes', () => {
basicAuth,
authType: 'Basic',
uri: 'https://auth.commercetools.com/api-endpoint',
body:
'grant_type=client_credentials&scope=view_products:sample-project manage_types:sample-project',
body: encode({
grant_type: 'client_credentials',
scope: 'view_products:sample-project manage_types:sample-project',
}),
})
})
})
Expand Down
26 changes: 16 additions & 10 deletions packages/sdk-auth/test/customer-password-flow.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import nock from 'nock'
import { encode } from 'qss'
import Auth from '../src/auth'
import config from './resources/sample-config'
import response from './resources/sample-response.json'
Expand All @@ -11,12 +12,15 @@ describe('Customer Password flow', () => {

test('should authenticate with correct user credentials', async () => {
const scope = nock(config.host)
.post(`/oauth/${config.projectKey}/customers/token`, {
grant_type: 'password',
scope: `manage_project:${config.projectKey}`,
username: 'user123',
password: 'pass123',
})
.post(
`/oauth/${config.projectKey}/customers/token`,
encode({
grant_type: 'password',
scope: `manage_project:${config.projectKey}`,
username: 'user123',
password: 'pass123',
})
)
.reply(200, JSON.stringify(response))

expect(scope.isDone()).toBe(false)
Expand All @@ -39,10 +43,12 @@ describe('Customer Password flow', () => {
const scope = nock(config.host)
.post(
`/oauth/${config.projectKey}/customers/token`,
// expected body
`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({
grant_type: 'password',
scope: `manage_project:${config.projectKey}`,
username: userCredentials.username,
password: userCredentials.password,
})
)
.reply(200, JSON.stringify(response))

Expand Down
12 changes: 9 additions & 3 deletions packages/sdk-auth/test/token-introspection.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import nock from 'nock'
import { encode } from 'qss'
import Auth from '../src/auth'
import config from './resources/sample-config'

Expand All @@ -15,9 +16,14 @@ describe('Token Introspection', () => {

test('should introspect token', async () => {
const scope = nock(config.host)
.post('/oauth/introspect', {
token: 'tokenValue',
})
.post(
'/oauth/introspect',
encode({
grant_type: 'client_credentials',
scope: 'manage_project:sample-project',
token: 'tokenValue',
})
)
.reply(200, JSON.stringify(response))

expect(scope.isDone()).toBe(false)
Expand Down
Loading