212 password flow #281

hisabimbola · 2017-08-18T09:48:56Z

Summary

Fixes #212

Description

Todo

Tests
- Unit
- Integration
- Acceptance
Documentation

affects: @commercetools/sdk-middleware-auth

affects: @commercetools/sdk-middleware-auth ISSUES CLOSED: #212

affects: @commercetools/sdk-middleware-auth

codecov · 2017-08-18T09:48:58Z

Codecov Report

Merging #281 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   97.74%   97.92%   +0.17%     
==========================================
  Files          70       72       +2     
  Lines        1555     1590      +35     
  Branches      341      350       +9     
==========================================
+ Hits         1520     1557      +37     
+ Misses         29       27       -2     
  Partials        6        6

Impacted Files	Coverage Δ
...sdk-middleware-auth/src/client-credentials-flow.js	`100% <100%> (+2.77%)`	⬆️
packages/sdk-middleware-auth/src/utils.js	`100% <100%> (ø)`
packages/sdk-middleware-auth/src/build-requests.js	`100% <100%> (ø)`	⬆️
packages/sdk-middleware-auth/src/base-auth-flow.js	`100% <100%> (ø)`
packages/sdk-middleware-auth/src/password-flow.js	`100% <100%> (+100%)`	⬆️

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 1020430...3a42900. Read the comment docs.

affects: @commercetools/sdk-middleware-auth

adnasa · 2017-08-18T11:30:48Z

packages/sdk-middleware-auth/test/build-requests.spec.js

    },
    scopes: allScopes,
    ...options,
  }
 }

+describe('buildRequestForPasswordFlow', () => {
+  const body = `grant_type=password&scope=${allScopes.join(' ')}\


oneLineTrim?

adnasa

you should switch the values around in your assertions

adnasa · 2017-08-18T11:31:26Z

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

+        expect(request).toEqual(actualParams.request)
+        expect(response).toEqual(actualParams.response)
+        expect(response).toEqual(actualParams.response)
+        expect([]).toEqual(actualParams.pendingTasks)


turn this around.

expect(pendingTasks).toEqual([])

Yeah, my bad...wondering what I was thinking... 😃

adnasa · 2017-08-18T11:31:44Z

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

+        expect(response).toEqual(actualParams.response)
+        expect([]).toEqual(actualParams.pendingTasks)
+        expect(
+          'https://auth.commercetools.co/oauth/foo/token/customers/token',


Can use oneLineTrim

No need for oneLineTrim here. Could not see how it will make this better.

adnasa · 2017-08-18T11:32:20Z

packages/sdk-middleware-auth/test/client-crendentials-flow.spec.js

+        expect(request).toEqual(actualParams.request)
+        expect(response).toEqual(actualParams.response)
+        expect(response).toEqual(actualParams.response)
+        expect([]).toEqual(actualParams.pendingTasks)


same here... you should turn it around

adnasa · 2017-08-18T11:32:32Z

packages/sdk-middleware-auth/test/client-crendentials-flow.spec.js

+        expect(
+          'https://auth.commercetools.co/oauth/token',
+        ).toBe(actualParams.url)
+        expect('MTIzOnNlY3JldA==').toBe(actualParams.basicAuth)


adnasa · 2017-08-18T11:33:04Z

packages/sdk-middleware-auth/test/base-auth-flow.spec.js

+      const middlewareOptions = createTestMiddlewareOptions()
+      let requestCount = 0
+      nock(middlewareOptions.host)
+        .persist() // <-- use the same interceptor for all requests


no need to comment this

affects: @commercetools/sdk-middleware-auth

wizzy25 · 2017-08-19T08:01:54Z

packages/sdk-middleware-auth/src/base-auth-flow.js

+  token: string;
+  expirationTime: number;
+}
+type AuthMiddlewareBaseOptions = {


Would it not be better to add the type-declarations in the types folder and import?

It's only used here, hence the reason but yeah can be refactored to the types folder

wizzy25 · 2017-08-19T08:02:01Z

packages/sdk-middleware-auth/src/build-requests.js

+    user,
+  } = options.credentials
+  const pKey = options.projectKey
+  if (!(clientId && clientSecret && user))


For better handling, this could be checked one after the other

Can you explain what you mean by "better handling"?

"Better usability"
If I have the clientIdand clientSecret but no user, it still throws with the same error:
Missing required credentials (clientId, clientSecret, user)

Yeah, all is at the same level else we can as well begin to validate each field, and that won't make too much sense.

That's why I am validating clientId and clientSecret and user on a level, and user.username and user.password on another level.

That's my thinking

adnasa · 2017-08-19T11:59:28Z

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

+      }
+      const next = (actualParams) => {
+        expect(actualParams.request).toEqual(actualParams.request)
+        expect(actualParams.response).toEqual(actualParams.response)


?! 👆🏽

adnasa · 2017-08-19T12:00:55Z

packages/sdk-middleware-auth/test/client-crendentials-flow.spec.js

        )
+        expect('MTIzOnNlY3JldA==').toBe(actualParams.basicAuth)


switch this please...

adnasa · 2017-08-19T12:01:23Z

packages/sdk-middleware-auth/test/client-crendentials-flow.spec.js

-          'This function should never be called, the response was rejected',
+      const next = (actualParams) => {
+        expect(actualParams.request).toEqual(actualParams.request)
+        expect(actualParams.response).toEqual(actualParams.response)


adnasa · 2017-08-19T12:02:39Z

packages/sdk-middleware-auth/src/build-requests.js

+    clientSecret,
+    user,
+  } = options.credentials
+  const pKey = options.projectKey


pKey -> projectKey

shorter var names to avoid eslint error and unnecessary multiline

I think it's a good trade off 🤷‍♂️

affects: @commercetools/sdk-middleware-auth

hisabimbola · 2017-08-21T11:22:07Z

Updated PR @adnasa @wizzy25.

Please check it again

adnasa · 2017-08-21T13:00:36Z

packages/sdk-middleware-auth/src/base-auth-flow.js

+  // If so, then go directly to the next middleware.
+  if (
+    (request.headers && request.headers['authorization']) ||
+    (request.headers && request.headers['Authorization'])


why are you covering both?
isn't it better to go for one of them and allow the other to fail?

Because it's can be an issue that occurred from the http module used when fetching OAuth token from the API

adnasa · 2017-08-21T13:06:06Z

packages/sdk-middleware-auth/test/base-auth-flow.spec.js

+      const request = createTestRequest()
+      const next = (req /* , res */) => {
+        expect(req).toEqual({
+          ...request,


Why are you setting this up via createTestRequest?

const request = createTestRequest({ headers: { Authorizaton: 'Bearer xxx' } })

to make the code more DRY

use toMatchObject

adnasa · 2017-08-21T13:07:13Z

packages/sdk-middleware-auth/src/build-requests.js

+  const oauthUri = options.oauthUri || `/oauth/${pKey}/customers/token`
+  const url = options.host.replace(/\/$/, '') + oauthUri
+  // eslint-disable-next-line max-len
+  const body = `grant_type=password&username=${username}&password=${password}${scopeStr}`


I thought we were moving towards using oneLineTrim,
one good thing about it is that it increases readability which this line doesn't offer. please change that

Since this is in the main code (that is being used in the browser also), I don't think it make sense to add a new module to the build just to trim new line.

Not a valid trade off from my point of view.

adnasa · 2017-08-21T15:55:36Z

packages/sdk-middleware-auth/test/base-auth-flow.spec.js

+      const request = createTestRequest()
+      const next = (req /* , res */) => {
+        expect(req).toEqual({
+          ...request,


use toMatchObject

affects: @commercetools/sdk-middleware-auth

hisabimbola · 2017-08-21T16:09:24Z

Refactored here 3a42900, thanks @adnasa for the tip, the assertion looks better now....

affects: @commercetools/sdk-middleware-auth ISSUES CLOSED: #212

Abimbola Idowu added 5 commits August 14, 2017 18:02

feat(password-flow): add method to build valid password auth uri

062080e

affects: @commercetools/sdk-middleware-auth

feat(sdk-middleware-auth): implement password flow

6e6106d

affects: @commercetools/sdk-middleware-auth ISSUES CLOSED: #212

refactor(base-auth-flow): refactor fetching token logic

4b2c0cd

affects: @commercetools/sdk-middleware-auth

test(base-auth-flow): add tests

92bb68f

affects: @commercetools/sdk-middleware-auth

chore(base-auth-flow): add comments to tests

f6a9636

affects: @commercetools/sdk-middleware-auth

hisabimbola added the Status: Review label Aug 18, 2017

hisabimbola self-assigned this Aug 18, 2017

hisabimbola requested review from adnasa and junajan August 18, 2017 09:48

Abimbola Idowu added 3 commits August 18, 2017 12:05

docs(password-flow): add docs about new auth flow

ba0358b

docs(password-flow): update docs

0249b2d

refactor(middleware-auth): remove duplicate tests

a8312ed

affects: @commercetools/sdk-middleware-auth

adnasa reviewed Aug 18, 2017

View reviewed changes

adnasa suggested changes Aug 18, 2017

View reviewed changes

Abimbola Idowu added 2 commits August 18, 2017 14:02

refactor(middleware-auth): update base on PR review

45e0118

affects: @commercetools/sdk-middleware-auth

refactor(middleware-auth): improve code

8fbdc12

affects: @commercetools/sdk-middleware-auth

wizzy25 reviewed Aug 19, 2017

View reviewed changes

adnasa suggested changes Aug 19, 2017

View reviewed changes

Abimbola Idowu added 2 commits August 21, 2017 13:04

test(integration): add integration tests for customer login

6b781d7

affects: @commercetools/sdk-middleware-auth

refactor(auth): refactor code base on PR review

76eb40f

affects: @commercetools/sdk-middleware-auth

fix(integration-tests): correct projectKey

84a3057

hisabimbola mentioned this pull request Aug 21, 2017

276 anonymous session #283

Merged

4 tasks

adnasa reviewed Aug 21, 2017

View reviewed changes

adnasa approved these changes Aug 21, 2017

View reviewed changes

refactor(tests): use more elegant assertions

3a42900

affects: @commercetools/sdk-middleware-auth

hisabimbola merged commit dc991d2 into master Aug 21, 2017

hisabimbola deleted the 212_password_flow branch August 21, 2017 16:13

hisabimbola removed the Status: Review label Aug 21, 2017

hisabimbola pushed a commit that referenced this pull request Aug 22, 2017

feat(sdk-middleware-auth): implement password flow (#281)

eeef199

affects: @commercetools/sdk-middleware-auth ISSUES CLOSED: #212

renovate bot mentioned this pull request Feb 5, 2018

chore(deps): update dependency eslint-plugin-react to v7.6.1 #427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

212 password flow #281

212 password flow #281

hisabimbola commented Aug 18, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading

adnasa Aug 18, 2017

adnasa left a comment

adnasa Aug 18, 2017

hisabimbola Aug 18, 2017

adnasa Aug 18, 2017

hisabimbola Aug 18, 2017

adnasa Aug 18, 2017

adnasa Aug 18, 2017

adnasa Aug 18, 2017

wizzy25 Aug 19, 2017

hisabimbola Aug 19, 2017

wizzy25 Aug 19, 2017

hisabimbola Aug 19, 2017

wizzy25 Aug 21, 2017

hisabimbola Aug 21, 2017

adnasa Aug 19, 2017

hisabimbola Aug 19, 2017

adnasa Aug 19, 2017

adnasa Aug 19, 2017

hisabimbola Aug 19, 2017

adnasa Aug 19, 2017

hisabimbola Aug 19, 2017

hisabimbola commented Aug 21, 2017

adnasa Aug 21, 2017

hisabimbola Aug 21, 2017

adnasa Aug 21, 2017

hisabimbola Aug 21, 2017

adnasa Aug 21, 2017

adnasa Aug 21, 2017

hisabimbola Aug 21, 2017

adnasa Aug 21, 2017

hisabimbola commented Aug 21, 2017

212 password flow #281

212 password flow #281

Conversation

hisabimbola commented Aug 18, 2017 • edited Loading

Summary

Description

Todo

codecov bot commented Aug 18, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

adnasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hisabimbola commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hisabimbola commented Aug 21, 2017

hisabimbola commented Aug 18, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading