aws-samples / amazon-cognito-passwordless-auth

Passwordless authentication with Amazon Cognito: FIDO2 (WebAuthn, support for Passkeys), Magic Link, SMS OTP Step Up
Apache License 2.0
385 stars 70 forks source link

WIP: Enforce MIN_SECONDS_BETWEEN for successful authentications #176

Closed tinti closed 5 months ago

tinti commented 5 months ago

Issue #175

Description of changes:

V1

Enforces MIN_SECONDS_BETWEEN in both cases by creating an used flag for each magic link.

Reorder magic link validation. Checks expiration before anything.

Validate all fields stored in DynamoDB.

V2

Enforces MIN_SECONDS_BETWEEN in both cases by creating an uat epoch attribute for each magic link.

Reorder magic link validation. Checks expiration before anything.

Validate all fields stored in DynamoDB. Note that uat is optional.

Fix: change DeleteCommand to UpdateCommand. Make sure that UpdateCommand do not perform an insert.

tinti commented 5 months ago

Hey mate, i had a look and we're nearly there.

  • Let's use uat (used at) instead of used (because we also do iat for issued at)
  • Let's not make it a boolean but make it a number, in which we store the unix timestamp of when it was used (like iat and exp)
  • In the condition expression we can just say that this attribute MUST NOT yet exist. It's presence signifies that the magic link was already used (and at which time exactly, which may be helpful to know for audit purposes)

The full condition expression then becomes:

`attribute_exists(#signatureHash) AND #signatureHash = :signatureHash AND attribute_exists(#userNameHash) AND #userNameHash = :userNameHash AND attribute_not_exists(#uat)`

How does that sound?

Sounds great. Working on it.

Cheers

tinti commented 5 months ago

@ottokruse Done. Please take a look.

Fix: ensure UpdateCommand do not perform an insert Feat: change used to uat Feat: make uat an optional field when validating

Cheers.

ottokruse commented 5 months ago

Had a look and a play around, pushed a tweak. Think this looks pretty good now! Will merge and publish to NPM in a bit.

(Part of the tweak is a little pedantry, you don't need to check the value of userNameHash in the condition expression as it is the table primary key and so DynamoDB will make sure it is the same)

tinti commented 5 months ago

Had a look and a play around, pushed a tweak. Think this looks pretty good now! Will merge and publish to NPM in a bit.

(Part of the tweak is a little pedantry, you don't need to check the value of userNameHash in the condition expression as it is the table primary key and so DynamoDB will make sure it is the same)

Don't we need it to ensure that DynamoDB will perform an update and not an insert? I mean without the condition the an insert could happen.

ottokruse commented 5 months ago

Because it is the primary key, just specifying attribute_exists(userNameHash) is enough. Because if it would need to create that item (if the key is not present in the table yet) then surely attribute userNameHash does not yet exist for that item, because it's a new item.

Try it :) The following only works if the item with userNameHash test exists in the table and raises a ConditionalCheckFailedException otherwise:

aws dynamodb update-item \
    --table-name passwordless-end-to-end-example-SecretsTablePasswordlessXYZ \
    --key '{"userNameHash": {"B": "test"}}' \
    --update-expression "SET #uat = :uat" \
    --expression-attribute-names '{"#uat": "uat"}' \
    --expression-attribute-values '{":uat": {"N": "12345" }}' \
    --condition-expression "attribute_exists(userNameHash)"
tinti commented 5 months ago

aws dynamodb update-item \ --table-name passwordless-end-to-end-example-SecretsTablePasswordlessXYZ \ --key '{"userNameHash": {"B": "test"}}' \ --update-expression "SET #uat = :uat" \ --expression-attribute-names '{"#uat": "uat"}' \ --expression-attribute-values '{":uat": {"N": "12345" }}' \ --condition-expression "attribute_exists(userNameHash)"

A little bit late. But I tested and agree. Thanks for accepting the patch.

ottokruse commented 5 months ago

Published to npm in v0.14.0