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
from

Conversation

@rhwilr
Copy link
Contributor

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.

rhwilr added 3 commits Nov 28, 2018
@coveralls

This comment has been minimized.

Copy link

commented Dec 8, 2018

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

@RomainLanz RomainLanz requested a review from thetutlage Dec 17, 2018

@RomainLanz RomainLanz requested review from thetutlage and removed request for thetutlage Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.