refactor(token): remove ability to pass createdAt to Token.create #1764

seanmonstar · 2017-03-23T23:50:53Z

This exists to support passing _createdAt as a query parameter, but
only for our tests. Those tests were to exercise our response
validation in a way that the lastAccessTime allows a value of 0. That
validation was fixed long ago, and supporting this hacky parameter has
done nothing but plague us since.

Burn the heretic.

/cc @philbooth @rfk

philbooth · 2017-03-24T07:06:10Z

test/local/tokens/token.js

      var now = Date.now() - 1
      return Token.createNewToken(Token, { createdAt: now }).then(token => {
-        assert.equal(token.createdAt, now, 'token.createdAt is correct')
+        assert.notEqual(token.createdAt, now, 'token.createdAt is new')


The anally-retentive pedant inside me thinks this should assert that token.createdAt > now rather than just asserting inequality.

Mocha pretty prints equal and notEqual assertions better than it can ok assertions. 🤷‍♂️

philbooth · 2017-03-24T07:09:12Z

lib/tokens/token.js

@@ -41,41 +41,13 @@ module.exports = function (log, config, random, P, hkdf, Bundle, error) {
    this.createdAt = details.createdAt || 0


Every time I see this line, it confuses me. Can someone remind me why we need || 0 here again please?

(I mean, as opposed to || Date.now())

Most recently, because the API test client in ./tests/client/ calls fromHex without specifying the createdAt time, so we gotta default it to something, and zero is much safer than e.g. the current time.

Of course the right answer might be to fix those damn tests to use the API correctly, or to use a simpler wrapper around it.

Defaulting to Date.now might give us back that bug with tokens that never expire.

Ah yeah, thanks.

There's this perhaps unintuitive contract in the Token API, which is:

Token.create is for generating brand new tokens, and so createdAt = Date.now()

Token.from (fromHex, fromId) is for building a pre-existing Token, and so createdAt should definitely be provided

new Token doesn't know which context it is being used in, so just defaults to believing the token is from 1969 until told otherwise.

philbooth · 2017-03-24T07:17:07Z

test/remote/device_tests.js

-      )
-    }
-  )
-


Something about deleting all three of these tests sits uneasily with me. I feel like, to some extent, they were legitimately asserting edge-case behaviour of the devices API.

Now that the tests are run in-process, is it possible to retain some of this coverage by mocking the result of Date.now()? Do we want to do that or am I being over-cautious?

It all depends on what exactly we hoped these were testing. My Pull Request CSI suggested that this was added as a regression test that our joi validation of lastAccessTime allowed a value of 0. This was the main reason for it being a "remote" test, so that it went through response validation.

If we want to still test that, a simple unit test in local should be pretty easy:

const schema = getRoute('/accounts/devices').config.response.schema.lastAccessTime assert(!Joi.validate({ lastAccessTime: 0 }, schema).error)

If we were testing specific behavior of handlers, and not response validation, that can probably also become local tests, with the edges cases specifically mocked in.

It all depends on what exactly we hoped these were testing. My Pull Request CSI suggested that this was added as a regression test that our joi validation of lastAccessTime allowed a value of 0.

That's definitely case for the first one, above, but the two underneath look different to me. It seems like they're testing how the guts of the devices endpoint behaves when it encounters odd session tokens. Is that something we're definitely happy to lose coverage of?

I can definitely adapt these tests to not pass _createdAt, but I don't fully understand what they are testing. Is it the behavior of updateDevice? The devices get route? I'll be sure to update the test description too once we know XD

I might be wrong about all this of course, but for what it's worth I read them as testing the behaviour of GET /account/devices. Definitely don't keep them if you don't think they add value though. I just wanted to make sure we'd considered them is all.

Ok, I've audited what these tests were doing:

sessionToken.lastAccessTime === 0: Testing that the route allows returning a last access time of 0, because of the default value in the DB of 0. I've added a local unit test making asserting that lastAccessTime: 0 passes the Joi validation.

sessionToken.lastAccessTime === -1: Simply is testing that /account/create fails with _createdAt=-1. Since that parameter is not even accepted anymore, test is no longer needed.

sessionToken.lastAccessTime === FUTURE: The access time is not adjusted anywhere in devices.upsert or devices.get routes, so this was only testing the initial creation of a device with a sessionToken from the future. The only code that adjusted the timestamp if it was in the future was in optionallyOverrideCreatedAt. So, the test was exercising our optional override, which has also been removed in this PR.

I believe the tests now accurately reflect the world.

philbooth · 2017-03-24T07:27:45Z

I support this change and will be glad to see the back of _createdAt. There's one question inline about whether we should keep some of the test coverage but, either way, I am r+.

This exists to support passing `_createdAt` as a query parameter, but only for our tests. Those tests were to exercise our response validation in a way that the `lastAccessTime` allows a value of 0. That validation was fixed long ago, and supporting this hacky parameter has done nothing but plague us since. Burn the heretic.

seanmonstar added the waffle:active label Mar 23, 2017

philbooth reviewed Mar 24, 2017

View reviewed changes

philbooth added waffle:review and removed waffle:active labels Mar 27, 2017

seanmonstar force-pushed the burn-created-at branch from bf2cf0c to c4255d2 Compare March 27, 2017 18:42

seanmonstar force-pushed the burn-created-at branch from c4255d2 to dac8f64 Compare March 27, 2017 18:51

seanmonstar merged commit 886aa82 into master Mar 27, 2017

seanmonstar deleted the burn-created-at branch March 27, 2017 22:36

seanmonstar removed the waffle:review label Mar 27, 2017

rfk added this to the FxA-0: quality milestone Apr 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(token): remove ability to pass createdAt to Token.create #1764

refactor(token): remove ability to pass createdAt to Token.create #1764

seanmonstar commented Mar 23, 2017

philbooth Mar 24, 2017

seanmonstar Mar 24, 2017

philbooth Mar 24, 2017 •

edited

philbooth Mar 24, 2017

rfk Mar 24, 2017

rfk Mar 24, 2017

philbooth Mar 24, 2017

seanmonstar Mar 24, 2017

philbooth Mar 24, 2017

seanmonstar Mar 24, 2017

philbooth Mar 25, 2017

seanmonstar Mar 25, 2017

philbooth Mar 25, 2017 •

edited

seanmonstar Mar 27, 2017

philbooth commented Mar 24, 2017

		@@ -41,41 +41,13 @@ module.exports = function (log, config, random, P, hkdf, Bundle, error) {
		this.createdAt = details.createdAt \|\| 0

refactor(token): remove ability to pass createdAt to Token.create #1764

refactor(token): remove ability to pass createdAt to Token.create #1764

Conversation

seanmonstar commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth Mar 24, 2017 • edited

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

philbooth Mar 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Mar 24, 2017

philbooth Mar 24, 2017 •

edited

philbooth Mar 25, 2017 •

edited