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.

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

WIP: Generate tokens with variable expiry date #25

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

rhwilr
Copy link
Contributor

@rhwilr rhwilr commented Dec 5, 2018

Hi @thetutlage

I added the expires_at field to tokens as we discussed in #24. So far it works, but I have two questions I'd like to get your input on:

1:
In order to allow you to set a different duration for each token, I had two options: Add a parameter to all methods (generateToken, register, updateEmail, updateProfile) that generate a token, or add a chainable method. I went with the latter since it would have made it a bit awkward to pass the parameter down. And I especially didn't like the signature for register:

async register (payload, callback, validFor = this.config.validFor) {

So currently I use the isValidFor method to set the parameter and return this to allow chaining. But this means I have to reset the value after generating a token.

I'm not happy with either of those options. So I want to ask you what you prefer or if you have a better idea?

2:
What should happen with existing tokens when users upgrade?
Currently, I treat tokens without an expiry date as invalid. To make the migration easy we could add a fallback: If expires_at is null, we fall back to the previous behavior and check the updated_at field. However, this fallback would only ever be used for 24 hours after the upgrade.

I would prefer the fallback option. So I tried to construct a query, but it turns out to be quite difficult since the query parameter is sometimes a user.tokens() relationship which would lead to a wrong or clause.

This was my attempt:

  _addTokenConstraints (query, type) {
    const dateFormat = this.config.dateFormat

    query
      .where('type', type)
      .where('is_revoked', false)
      .where('expires_at', '>=', moment().format(dateFormat))
      .orWhere(function () {
        this.whereNull('expires_at')
          .where('type', type)
          .where('is_revoked', false)
          .where('updated_at', '>=', moment().subtract(24, 'hours').format(dateFormat))
      })
  }

Looking forward to your feedback.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 43

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 93.846%

Totals Coverage Status
Change from base Build 42: 0.3%
Covered Lines: 144
Relevant Lines: 149

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants