Add disabled at #101
Add disabled at #101
Conversation
Structure the code slightly differently in order to facilitate testing, and then test with a database and a full Rails stack for better assurance we have things working correctly.
When old keys are used, and assuming the disabled_at field is set, this allows those old keys to be successful, but warns that the key is disabled and will be removed soon.
micahhainlinestitchfix
requested review from
brettfishman,
bwebster and
a team
as code owners
lib/stitches/allowlist_middleware.rb
Outdated
| def allowlist_regex | ||
| regex = @except || configuration.allowlist_regexp | ||
|
|
||
| unless regex.nil? || regex.is_a?(Regexp) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Since you've taken the time to refactor this middleware a bit, this check would read better to me as:
if !regex.nil? && !regex.is_a?(Regexp)
README.md
Outdated
| @@ -35,7 +35,7 @@ Then, set it up: | |||
|
|
|||
| ### Upgrading from an older version | |||
|
|
|||
| - When upgrading to version 4.0.0 you may now take advantage of an in-memory cache | |||
| - When upgrading to version 4.1.0 you may now take advantage of an in-memory cache | |||
There was a problem hiding this comment.
The cache was introduced in 4.0.0 and that is still true, right?
There was a problem hiding this comment.
It is, I'll rephrase
README.md
Outdated
| > bin/rails generate stitches:add_disabled_at_to_api_clients | ||
| ``` | ||
|
|
||
| - If you have a version lower than 4.1.0, you need to run one generator: |
There was a problem hiding this comment.
Should this say if you have version 4.1.0 or higher?
There was a problem hiding this comment.
No, but I'll rephrase this and the description I copied this from.
|
|
||
| ```ruby | ||
| Stitches.configure do |config| | ||
| config.disabled_key_leniency_in_seconds = 3 * 24 * 60 * 60 # Time in seconds, defaults to three days |
There was a problem hiding this comment.
Should there be a ceiling / maximum leniency enforced?
There was a problem hiding this comment.
I don't see a huge reason for that. I don't really want anyone at Stitch Fix to modify it, but I don't see any real problem with any particular length of time.
|
Looking good - left a few questions. |
|
One other thought (you're likely already planning to do), but I'd definitely cut an RC and test this out in an app before merging and releasing the new version. |
| ``` | ||
|
|
||
| - If you have a version lower than 3.6.0, you need to run one generator: | ||
| - If you are upgrading from a version between 3.3.0 and 3.5.0 you need to run two generators: |
There was a problem hiding this comment.
I just looked up the actual versions out there to give a better inclusive range
micahhainlinestitchfix
force-pushed
the
add-disabled-at
branch
from
abe5bdb
to
a78dd18
Compare
Blank lines were generated by the generators in a few places, which cause some style format validators to fail, such as rubocop.
Note that we are moving from 4.0.2 to 4.2.0. This is because there were a set of RC builds and tags for 4.1.0 which never made it to a 4.1.0 release. Nevertheless, it seems prudent to move to a new minor revision.
Problem
There were situations in which we want to allow some leniency for old API keys to be used, with escalating warnings before actually hard stopping those calls from going through.
Solution
Add a disabled_at field to the database and use it if present along with the configuration to allow some leniency before rejecting an API key.