Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements passwordless authentication via OTP, enhancing security by enabling users to log in without traditional passwords. Key changes include:
- Implementation of an OTP provider and its integration into the authentication flow.
- Enhancements in secret management (addition of secret deletion) and adjustments to JWT configurations.
- Updates to API definitions and documentation to support the new OTP endpoints.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/src/test/scala/com/theproductcollectiveco/play4s/auth/DefaultOtpProviderSpec.scala | Added tests to validate OTP round-trip functionality. |
| app/src/main/scala/com/theproductcollectiveco/play4s/auth/DefaultOtpProvider.scala | Implements OTP initialization and validation logic. |
| app/src/main/scala/com/theproductcollectiveco/play4s/auth/DefaultKeyStoreBackend.scala | Introduces deletion functionality for stored secrets in the keystore. |
| app/src/main/scala/com/theproductcollectiveco/play4s/auth/DefaultJwtProvider.scala | Adjusts JWT leeway and refines error handling using AuthProcessingError. |
| app/src/main/scala/com/theproductcollectiveco/play4s/auth/DefaultAuthProvider.scala | Modifies secret initialization to accept an optional provided secret and adds secret removal. |
| app/src/main/scala/com/theproductcollectiveco/play4s/api/AuthService.scala | Integrates OTP provider into the auth service with endpoints for OTP initiation and redemption. |
| app/src/main/scala/com/theproductcollectiveco/play4s/MainApp.scala | Registers the new OTP provider within the main application. |
| api/src/main/smithy/com/theproductcollectiveco/play4s/internal/auth/service-auth-api.smithy | Adds OTP-related operations and updates error definitions. |
| api/src/main/smithy/com/theproductcollectiveco/play4s/internal/auth/model.smithy | Introduces the OTP and contact models required for OTP flows. |
| api/src/main/smithy/com/theproductcollectiveco/play4s/game/sudoku/errors.smithy | Adds definitions for AuthValidationError and AuthProcessingError. |
| README.md | Documents new endpoints for OTP initiation and authorization. |
Comments suppressed due to low confidence (1)
app/src/main/scala/com/theproductcollectiveco/play4s/api/AuthService.scala:43
- The ifM branches appear inverted: when OTP validation returns true (indicating a valid OTP), the current code raises an error. Consider swapping the branches so that a valid OTP results in calling requestToken(requester) and an invalid OTP raises an error.
otpProvider.validateOtp(requester, otp).ifM(InvalidInputError("Invalid OTP supplied").raiseError, requestToken(requester))
| SecureRandom.javaSecuritySecureRandom[F].flatMap { security => | ||
| given SecureRandom[F] = security | ||
| UUIDGen.fromSecureRandom.randomUUID.map(_.toString).flatMap { uuid => | ||
| loaded.retrieve(alias).flatMap { case Some(_) | None => loaded.store(alias, providedSecret.getOrElse(uuid)) } |
There was a problem hiding this comment.
[nitpick] The pattern matching 'case Some(_) | None' covers all cases, which makes the match redundant. Consider removing the match and directly calling loaded.store, if overwriting the secret is the intended behavior.
Suggested change
| loaded.retrieve(alias).flatMap { case Some(_) | None => loaded.store(alias, providedSecret.getOrElse(uuid)) } | |
| loaded.store(alias, providedSecret.getOrElse(uuid)) |
0dbc6a1 to
f685638
Compare
f685638 to
30c5634
Compare
30c5634 to
c41d91c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issues worked on: