#18 implement properties.timeout for account().tokens.add(properties) #43

vkai · 2016-11-27T00:25:09Z

closes #18

Update

vkai · 2016-11-27T00:38:43Z

@gr2m the hoodie-account-server-api doc lists under api.account().tokens.add() that the token would store the creation date "createdAt": "2016-01-01T00:00:00.000Z". However, when writing my integration test, I created a token that did not automatically include the "createdAt" property. I ended up adding the property manually for the purposes of the test.

Am I missing something about how the tokens should work?

gr2m · 2016-11-27T10:44:44Z

you are right, the token methods have been added in a hurry as it looks, there seem rather incomplete
https://github.com/hoodiehq/hoodie-account-server-api/blob/82bf25c47839818d4eb73467b8ba500074804f67/lib/utils/add-token-to-user-doc.js

We usually start with a README for a new project and then implement it backwards, so it’s possible that we sometimes miss things. I agree that we need the createdAt timestamps in order for timeout to be useful. I would add it to the add-token-to-user-doc.js itself, instead of adding it test

gr2m · 2016-11-28T09:53:29Z

lib/utils/add-token-to-user-doc.js

@@ -15,6 +15,9 @@ function addTokenToUserDoc (db, account, token) {
    var id = token.id || uuid.v4()
    delete token.id

+    var now = new Date(Date.now())


you can leave out the Date.now(), new Date() should do the same thing.
You can also shorten the two lines to token.createdAt = new Date().toISOString() if you don’t use the now variable anywhere else

gr2m · 2016-11-29T19:57:09Z

lib/account.js

@@ -13,6 +13,10 @@ function account (setupPromise, state, findAccountOptions) {
          return Promise.reject(errors.TOKEN_TYPE_INVALID)
        }

+        if (!tokenOptions.hasOwnProperty('timeout')) {
+          tokenOptions.timeout = 7200
+        }


I’m sorry I should have thought about this earlier, but can we add another timestamp expiresAt instead of adding a timeout property? It will be simpler to lookup later as we can directly look for that property without any calculation. Or do you see any downside with that approach?

No I agree that would be a more practical approach, as well as more conventional. I'll change it!

you’re the best 🙌

gr2m · 2016-12-01T00:07:34Z

This looks all good 👍 thanks!

Only thing left is to update the README now, where it still says "TO BE DONE" for properties.timeout in the options table for api.account().tokens.add()

vkai · 2016-12-01T06:17:18Z

I've removed the "TO BE DONE" note for properties.timeout as well as properties.type as that was pushed as part of #36 that @Taekyoon and I worked on last.

gr2m

Hey I’m so sorry, I found another bug right now, you put in the check for sessions.add, but we also use the token authentication in other places. I think we should put in the check into find-user-doc-by-username-or-id-or-token.js as it’s used in all the places. Could you make that change, please?

vkai · 2016-12-02T06:48:05Z

@gr2m, no worries, that makes more sense. Long post ahead...

I think I've found a bug that came up in email-verification-test.js#L35 when I moved the expiration check to find-user-doc-by-username-or-id-or-token.js.

account().tokens.find() returns the token "as is" from the userdoc without an id property. When api.accounts.update() is called in email-verification-test.js, there is no id property on token. Follow the code down to find-user-doc-by-username-or-id-or-token.js#L34 and there's no tokenId for the db query.

The test still works because the query would still respond with the userdoc (not sure how that works). Is this by design? This creates a problem because I've moved the expiration check into that snippet:

  return db.query('byToken', {
    key: tokenId,
    include_docs: true
  }).then(function (response) {
    if (response.rows.length === 0) {
      throw PouchDBErrors.MISSING_DOC
    }

    var doc = response.rows[0].doc
    if (isTokenExpired(doc.tokens[tokenId])) {
      return Promise.reject(errors.TOKEN_EXPIRED)
    }

    return doc
  })

tokenId is undefined by this point, and thus I can't check if the token is expired. Does my check make sense there? If that makes sense, then I would change email-verification-test.js to use a preset token id. What do you think?

gr2m · 2016-12-03T21:01:07Z

can you push your current state please?

account().tokens.find() returns the token "as is" from the userdoc without an id property

That sounds like a bug, the id property should definitely be returned. We have to add logic around

hoodie-account-server-api/lib/utils/find-user-doc-by-username-or-id-or-token.js

Line 41 in 82bf25c

return response.rows[0].doc

where we set the id before returning the doc, I think?

upstream update

email-verification-test.js failing

…i into 18_timeout

…, email-verification-test.js failing

vkai · 2016-12-03T22:08:19Z

I've pushed my current state with email-verification-test.js still failing.

If the id should be returned with the token, that would solve the problem. api.account().tokens.add() works because add-token-to-user-doc.js#L23 adds the id back to the token object before returning. If find() should also return the id, should we be deleting the id property from the token in the first place? add-token-to-user-doc.js#L16

gr2m · 2016-12-03T23:00:44Z

If find() should also return the id, should we be deleting the id property from the token in the first place? add-token-to-user-doc.js#L16

yes, because it would be the same value as the tokens key and there would be a risk for corrupt data where the tokens would be different from the id property of the token

vkai · 2016-12-03T23:38:27Z

I see. Ok then I will go ahead and add the id to the result of find() as well.

gr2m

great work 👍👏

gr2m · 2016-12-04T10:36:05Z

If you like, you could work on code coverage for hoodie-account-server-api, it’s currently at . Let me know if you are interested, I can put together an issue with some more information on how to do that :)

vkai · 2016-12-04T17:13:29Z

@gr2m sure that sounds great!

gr2m · 2016-12-04T17:29:45Z

@vkai awesome! I made this for you: #46

vkai added 2 commits November 17, 2016 23:28

fixed dates in to include seconds place

c5c3756

Merge pull request #1 from hoodiehq/master

64acf27

Update

gr2m reviewed Nov 28, 2016

View reviewed changes

gr2m requested changes Nov 29, 2016

View reviewed changes

gr2m approved these changes Dec 1, 2016

View reviewed changes

vkai added 8 commits December 1, 2016 10:03

added default timeout and timeout expiration check

38db404

fixed token expiration check and added unit tests

39a7753

added integration test

34e4382

ran standard fix

17fdbc6

added createdAt property to tokens

703a150

fixed token.createdAt initialization

c9c9630

changed token object to store expiresAt instead of timeout property

23ff166

updated README to remove TO BE DONE for issues 17 and 18

7e60b31

gr2m force-pushed the 18_timeout branch from 4c130e7 to 7e60b31 Compare December 1, 2016 09:03

gr2m requested changes Dec 1, 2016

View reviewed changes

vkai added 9 commits December 3, 2016 13:37

Merge pull request #2 from hoodiehq/master

fc87a50

upstream update

added default timeout and timeout expiration check

cccb7e0

fixed token expiration check and added unit tests

f191692

added integration test

bfa1842

ran standard fix

d6ebd44

added createdAt property to tokens

8b1fbd6

fixed token.createdAt initialization

4698c6d

changed token object to store expiresAt instead of timeout property

0f9b712

updated README to remove TO BE DONE for issues 17 and 18

5036c21

vkai added 4 commits December 3, 2016 13:37

moved expiration check to find-user-doc-by-username-or-id-or-token.js

22e8ddd

email-verification-test.js failing

Merge branch 'master' into 18_timeout

6b57ba6

Merge branch '18_timeout' of github.com:vkai/hoodie-account-server-ap…

1fe86b8

…i into 18_timeout

moved expiration check to find-user-doc-by-username-or-id-or-token.js…

852fb1e

…, email-verification-test.js failing

added id to resulting token in account().tokens.find()

45d93cd

gr2m approved these changes Dec 4, 2016

View reviewed changes

gr2m merged commit c941c1d into hoodiehq:master Dec 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#18 implement properties.timeout for account().tokens.add(properties) #43

#18 implement properties.timeout for account().tokens.add(properties) #43

vkai commented Nov 27, 2016 •

edited by gr2m

vkai commented Nov 27, 2016

gr2m commented Nov 27, 2016

gr2m Nov 28, 2016

gr2m Nov 29, 2016

vkai Nov 29, 2016

gr2m Nov 29, 2016

gr2m commented Dec 1, 2016 •

edited

vkai commented Dec 1, 2016

gr2m left a comment

vkai commented Dec 2, 2016

gr2m commented Dec 3, 2016 •

edited

vkai commented Dec 3, 2016

gr2m commented Dec 3, 2016

vkai commented Dec 3, 2016

gr2m left a comment

gr2m commented Dec 4, 2016 •

edited

vkai commented Dec 4, 2016

gr2m commented Dec 4, 2016

#18 implement properties.timeout for account().tokens.add(properties) #43

#18 implement properties.timeout for account().tokens.add(properties) #43

Conversation

vkai commented Nov 27, 2016 • edited by gr2m

vkai commented Nov 27, 2016

gr2m commented Nov 27, 2016

gr2m Nov 28, 2016

Choose a reason for hiding this comment

gr2m Nov 29, 2016

Choose a reason for hiding this comment

vkai Nov 29, 2016

Choose a reason for hiding this comment

gr2m Nov 29, 2016

Choose a reason for hiding this comment

gr2m commented Dec 1, 2016 • edited

vkai commented Dec 1, 2016

gr2m left a comment

Choose a reason for hiding this comment

vkai commented Dec 2, 2016

gr2m commented Dec 3, 2016 • edited

vkai commented Dec 3, 2016

gr2m commented Dec 3, 2016

vkai commented Dec 3, 2016

gr2m left a comment

Choose a reason for hiding this comment

gr2m commented Dec 4, 2016 • edited

vkai commented Dec 4, 2016

gr2m commented Dec 4, 2016

vkai commented Nov 27, 2016 •

edited by gr2m

gr2m commented Dec 1, 2016 •

edited

gr2m commented Dec 3, 2016 •

edited

gr2m commented Dec 4, 2016 •

edited