initial DynamoDB implementation #36
Conversation
|
|
||
| // ..... | ||
| // to expose public keys: | ||
| publicKeyRouter.get('/auth/.well-known/jwks', /* @this publicKeyRouter */ function() { |
There was a problem hiding this comment.
I don't think we need to repeat all of this, just do dynamo specific wireup
| AuthProvisionerDynamo = require('brightspace-auth-keys-dynamodb-store'), | ||
| publicKeyRouter = require('koa-router')(); | ||
|
|
||
| const provisioner = new AuthProvisionerDynamo({ |
There was a problem hiding this comment.
Right, so I don't love this wrappery pattern.
| AbstractProvisioningCache = require('brightspace-auth-provisioning').AbstractProvisioningCache, | ||
| clock = require('./clock'); | ||
|
|
||
| class DynamoProvisioningCache extends AbstractProvisioningCache { |
There was a problem hiding this comment.
Mm, sorry if I misinterpreted.
Would rather keep this separate, maybe giving the node-auth-provisioning repo the same treatment
There was a problem hiding this comment.
(or maaaybe bundling all together more, but, babier steps)
| "main": "src/public-key-store.js", | ||
| "scripts": { | ||
| "test": "nyc --all mocha --timeout 15000 --recursive ./test && npm run check-style", | ||
| "check-style": "eslint ." |
There was a problem hiding this comment.
eslint is taken care of at the top level
There was a problem hiding this comment.
working on cleaning this up
| }, | ||
| "author": "D2L Corporation", | ||
| "license": "Apache-2.0", | ||
| "devDependencies": { |
There was a problem hiding this comment.
Most of these should already installed at the top level.
Trying to keep most things there, since they'll be mostly re-used, and greenkeeper will manage them that way.
I did the the peerDep for "redis" at this level, but i dunno if that was the right move. The scripts don't handle that yet though, so definitely keep that here.
| same "printed page" as the copyright notice for easier | ||
| identification within third-party archives. | ||
|
|
||
| Copyright 2017 D2L Corporation |
There was a problem hiding this comment.
I guess it's techincally 2018 now
There was a problem hiding this comment.
Updating all the licenses
| "type": "git", | ||
| "url": "https://github.com/Brightspace/node-auth-keys.git" | ||
| }, | ||
| "files": [ |
There was a problem hiding this comment.
👍 files is strictly better than npmignore
There was a problem hiding this comment.
just copied it from your PR
| function setTtl(db, tablename) { | ||
| var params = { | ||
| TableName: tablename, | ||
| TimeToLiveSpecification: { |
There was a problem hiding this comment.
I can't find the dynamo documentation that actually describes what the value of the TTL column is/interpreted as...?
There was a problem hiding this comment.
It shows up in the popup in the console "TTL is a mechanism to set a specific timestamp for expiring items from your table. The timestamp should be expressed as an attribute on the items in the table. The attribute should be a Number data type containing time in epoch format. Once the timestamp expires, the corresponding item is deleted from the table in the background."
| return db.createTable(params).promise().then(() => { | ||
| return setTtl(db, tablename); | ||
| }).catch((e) => { | ||
| // ResourceInUseException indicates the table already exists |
There was a problem hiding this comment.
Assuming that the ttl is also set if the table already exists?
There was a problem hiding this comment.
Or that if they manually created that they decided they didn't want it for whatever reason
| } | ||
| }; | ||
| return db.updateTimeToLive(params).promise().catch(e => { | ||
| // UnknownOperationException will be thrown for Dynamo Local |
There was a problem hiding this comment.
Is there a way to "detect" dynamo local, so we can only ignore this for dynamo local?
There was a problem hiding this comment.
The UnknownOperationException exception won't happen unless it doesn't support TTL so even if it was production that threw it I would want to ignore it.
| @@ -0,0 +1,138 @@ | |||
| 'use strict'; | |||
|
|
|||
| const | |||
There was a problem hiding this comment.
nit: I avoid mixing up "local" requires with package requires
| before(() => { | ||
| return DynamoDbLocal.launch(dynamoLocalPort, null, ['-sharedDb']).then(() => { | ||
| db = new AWS.DynamoDB({ | ||
| endpoint: new AWS.Endpoint('http://localhost:8000'), |
There was a problem hiding this comment.
http://localhost:${dynamoLocalPort}?
| }) | ||
| .then(retrievedKeys => { | ||
| // Rather than checking for equality, assert that expectedPublicKey is in the returned array. | ||
| // The contents of the array are not guaranteed, and may contain actual keys. The same applies for the test below. |
There was a problem hiding this comment.
Probably better to just have the comment again rather than positionally referring to another test :)
There was a problem hiding this comment.
Just realized I did it first ;)
| assert.deepInclude(retrievedKeys, expectedPublicKey); | ||
| }) | ||
| .then(() => { | ||
| return provisioner._storePublicKey(expectedPublicKey, oldExpiryTime); |
There was a problem hiding this comment.
Don't get why this is happening?
There was a problem hiding this comment.
To "clean up" after itself. Storing an expired key is functionally the same as deleting the key, but there's no API for deleting. Otherwise there's potential for tests to affect each other.
There was a problem hiding this comment.
I guess re: my other comment then, is there a method to wipe the table, and just do that in an afterEach?
|
|
||
| before(() => { | ||
| return DynamoDbLocal.launch(dynamoLocalPort, null, ['-sharedDb']).then(() => { | ||
| db = new AWS.DynamoDB({ |
There was a problem hiding this comment.
Any way to clear the table between tests?
There was a problem hiding this comment.
Not that I could find. Tests should generally clean up after themselves.
There was a problem hiding this comment.
just .stop() and then start a new one?
There was a problem hiding this comment.
Possible, but heavy-handed and may slow down test execution noticeably. Similarly it's possible to call deleteTable followed by createTable, but other than writing fewer lines of code there doesn't seem to be a good reason to take a heavy handed approach between tests instead of expecting the tests to remove the data they add.
There was a problem hiding this comment.
I decided to just do it since it's what we're doing elsewhere
.eslintrc.json
Outdated
| "extends": "brightspace/node-config" | ||
| "extends": "brightspace/node-config", | ||
| "env": { | ||
| "mocha":true |
There was a problem hiding this comment.
This should be added in an eslintrc in your test folder
| @@ -0,0 +1,47 @@ | |||
| # brightspace-auth-keys-dynamodb-store | |||
|
|
|||
| [](https://travis-ci.org/Brightspace/node-auth-keys) | |||
| "README.md", | ||
| "LICENSE" | ||
| ], | ||
| "devDependencies": { |
There was a problem hiding this comment.
Can you define these top-level? The peer dep is fine to stay here
There was a problem hiding this comment.
(The tests failed because I removed the script that runs install within the packages)
| @@ -14,7 +14,7 @@ | |||
| "LICENSE" | |||
| ], | |||
| "scripts": { | |||
| "test": "istanbul cover --root src mocha -- --recursive ./test" | |||
| "test": "istanbul cover --root src ../../../node_modules/mocha/bin/mocha -- --recursive ./test" | |||
There was a problem hiding this comment.
To work in Windows it seems to be necessary. gotwarlost/istanbul#90
There was a problem hiding this comment.
Alright cool. nyc may not have the same problem, so will look at fixing it afterward
|
Add to the package list in README.md? |
|
Would be nice to have it rebased / squashed down before merge |
| _lookupPublicKeys() { | ||
| const parameters = { | ||
| TableName: this._tableName, | ||
| FilterExpression: '#e > :expires_at', |
There was a problem hiding this comment.
This is a predicate on inclusion, not eviction, correct?
There was a problem hiding this comment.
Correct. Keys are evicted by the table TTL
There was a problem hiding this comment.
I just meant more like, doublechecking it should be a > and not a <
There was a problem hiding this comment.
yes, but the name is bad, I'm going to change that
alamarre
force-pushed
the
implement-dynamodb
branch
from
f58a0c9
to
fd889c6
Compare
omsmith
force-pushed
the
implement-dynamodb
branch
from
fd889c6
to
b74579a
Compare
| return this._scanUntilComplete(null, parameters, []); | ||
| } | ||
|
|
||
| _scanUntilComplete(response, parameters, items) { |
There was a problem hiding this comment.
Thoughts on:?
_scanUntilComplete(response, parameters, items) {
if (response) {
if (response.Items) {
items = items.concat(response.Items.map(row => row.PublicKey.S));
}
if (response.LastEvaluatedKey) {
parameters.ExclusiveStartKey = response.LastEvaluatedKey;
} else {
return Promise.resolve(items);
}
}
return this
._client
.scan(parameters)
.promise()
.then(response => this._scanUntilComplete(response, parameters, items));
}There was a problem hiding this comment.
that's cleaner, I like it
alamarre
force-pushed
the
implement-dynamodb
branch
from
94d5d53
to
122e4c6
Compare
No description provided.