Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

added 'created' to all tokens and 'generation' to accounts #485

Merged
merged 2 commits into from Jan 13, 2014

Conversation

dannycoates
Copy link
Contributor

This doesn't actually do anything with the timestamps, it just brings them into existence. The 'generation' on accounts isn't necessarily for this proposal but it could be used for that; I was specifically thinking about #388 (comment)

While I was at it I did a minor refactor to pull all of the sql statements up a notch. Also, it turns out that db_tests.js was totally busted and would silently always use heap.js. 🍌

Lots more work to do in this area, but this is mergeable, so r?

P.S. bigint for timestamps, yea or nay?

@ckarlof
Copy link
Contributor

ckarlof commented Jan 10, 2014

See #486 as well.

@ckarlof
Copy link
Contributor

ckarlof commented Jan 10, 2014

Bike shedding a bit, but generation can also be viewed as a createdAt annotation of the password verifier, correct? That makes it a bit more explicit when it should change and what it means vs generation.

@dannycoates
Copy link
Contributor Author

I agree, generation is a nebulous term in this context. It should be considered a placeholder until we get more clarity on it's use cases.

var CREATE_ACCOUNT = 'INSERT INTO accounts' +
' (uid, normalizedEmail, email, emailCode, verified,' +
' kA, wrapWrapKb, authSalt, verifyHash, generation)' +
' VALUES (?, LOWER(?), ?, ?, ?, ?, ?, ?, ?, ?)'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 much clearer this way

@rfk
Copy link
Contributor

rfk commented Jan 13, 2014

LGTM, r+

bigint for timestamps, yea or nay?

Yea.

While there's almost zero change of a race here (e.g. two closely-spaced password-reset operations producing the same generation numbers) I think we can afford to store the millisecond timestamps for maximal precision. We were going to use bigint timestamps in sync2.0, which had a lot more timestamps than we do here.

Also we can refactor this internally later if we need to, e.g. truncate the high bits of the timestamp and store it in a smaller column.

dannycoates added a commit that referenced this pull request Jan 13, 2014
added 'created' to all tokens and 'generation' to accounts
@dannycoates dannycoates merged commit 4b0bb50 into mozilla:master Jan 13, 2014
@dannycoates dannycoates deleted the created-time branch May 13, 2015 00:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants