-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add AuthController
#1368
Add AuthController
#1368
Conversation
Pull Request Test Coverage Report for Build 8614191593Details
💛 - Coveralls |
src/config/entities/configuration.ts
Outdated
nonceTtlSeconds: parseInt( | ||
process.env.AUTH_NONCE_TTL_SECONDS ?? `${60 * 60}`, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference or recommended minimum value from the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is an arbitrary value. It is effectively the period when a user should sign the message. Do you think we should decrease it?
I considered setting it to the expirationTime
but that value is not always present and the nonce is cleared if the message is expired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per discussion, this was reduced to five minutes in 8c6934c.
src/domain/auth/auth.repository.ts
Outdated
// Don't prevent nonce from being deleted | ||
.catch(() => false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: wdyt about using a try-catch-finally
block here? The nonce
deletion seems to fit in a finally
clause at it needs to be always executed, and we can also have specific errors for each condition (expired, invalid nonce, invalid signature) that could be managed in a catch
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in 0ce0207.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 👏
Summary
This adds the
AuthController
as part of a test implementation for SiWe authentication, breaking down #1321 for review.Changes
AuthModule
AuthController
, enabled behind a feature flagGET
/v1/auth/nonce
- returns nonce to be signed in SiWe messagePOST
/v1/auth/verify
- verify signature and return access tokenVerifyAuthMessageDtoSchema
AuthService
generateNonce
- get nonce to include in SiWe messageverify
- verify signature according to expiration, nonce and validity of signature