Skip to content
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

Jump to bottom

feat: validate account.tokens.type #36

Merged
merged 2 commits into from Nov 20, 2016
Merged

feat: validate account.tokens.type #36

merged 2 commits into from Nov 20, 2016

Conversation

Taekyoon
Copy link
Contributor

@Taekyoon Taekyoon commented Nov 11, 2016 •
edited

@vkai also contributed this issue.
Added validating account.tokens.type in account.js that might cause invalidate form of tokens

@Taekyoon
Copy link
Contributor Author

@gr2m Greg, I think it is weird. I had a pull request about 'test: sessions.remove..', but it still remains. I'm afraid this pull request goes wrong.

@gr2m
Copy link
Member

gr2m commented Nov 11, 2016

I’ve rebased your pull request on latest changes in master and forced-pushed the changes to your master branch. Make sure you git pull them before you continue working on it

gr2m
gr2m requested changes Nov 11, 2016
View reviewed changes
lib/account.js
if (typeof tokenOptions.type !== 'string' ||
!validPattern.test(tokenOptions.type)) {
return Promise.reject(errors.TOKEN_TYPE_INVALID)
}
Copy link
Member

Choose a reason for hiding this comment

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

can you move this code into a separate file like /lib/utils/is-valid-token-type.js and then do if (!isValidTokenType(tokenOptions.type)) {?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fixing it!

lib/utils/errors.js

module.exports.TOKEN_TYPE_INVALID = hoodieError({
name: 'Bad Request',
message: 'Type must be a string of lowercase characters, numbers, -, or _, and must begin with a character.',
Copy link
Member

Choose a reason for hiding this comment

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

Can we change the error message to: Token type must consist of lowercase letters (a-z), digits (0-9), _ and - only. Must begin with a letter. No . at the end

test/unit/account/tokens-test.js
t.is(error.status, 400)
t.is(error.message, 'Type must be a string of lowercase characters, numbers, -, or _, and must begin with a character.')
})
})
Copy link
Member

Choose a reason for hiding this comment

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

Please add another test for when type is not set at all

gr2m
gr2m requested changes Nov 15, 2016
View reviewed changes
lib/account.js
var findUserDoc = require('./utils/find-user-doc-by-username-or-id-or-token')
>>>>>>> 0f0e8b3462cec11dfc4661ba1d06283f35098533
Copy link
Member

Choose a reason for hiding this comment

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

you have to resolve the merge conflicts :) @Taekyoon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I found it! But this is the first time that I experienced. I think it takes some time to solve it!. Just give me some time.

Copy link
Member

Choose a reason for hiding this comment

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

no rush at all :)

@Taekyoon
Copy link
Contributor Author

Taekyoon commented Nov 15, 2016 •
edited

ran the test, and I found out some test didn't work well. The test result shows this.

test/integration/email-verification-test.js ........... 1/3 1s
  email verification
  not ok api.account(...).tokens.find is not a function
    found:
      name: TypeError
      stack: |-
        TypeError: api.account(...).tokens.find is not a function
            at /Users/Taekyoon/GitHub/hoodie-account-server-api/test/integration/email-verification-test.js:30:48
            at process._tickCallback (node.js:369:9)
      message: api.account(...).tokens.find is not a function
    at:
      line: 369
      column: 9
      file: node.js
      type: process
      function: _tickCallback
    stack: ''

  email verification
  not ok test unfinished: email verification
    at:
      line: 8
      column: 1
      file: test/integration/email-verification-test.js
    source: |
      test('email verification', function (t) {

@vkai
Copy link
Contributor

vkai commented Nov 15, 2016

@Taekyoon it looks like you deleted the tokens.find function in your last commit. Take a look at lib/account.js before you resolved the merge conflicts. The test is failing because the tokens.find function is missing.

@Taekyoon
Copy link
Contributor Author

Oh.. I didn't notice it because I thought I didn't deal with that code. I'll find out!

gr2m
gr2m approved these changes Nov 20, 2016
View reviewed changes
Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

great work 👍

@gr2m gr2m merged commit 68c1752 into hoodiehq:master Nov 20, 2016
@gr2m gr2m mentioned this pull request Nov 20, 2016
Closed
@vkai vkai mentioned this pull request Dec 1, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Taekyoon @gr2m @vkai