feat(Authenticator): add ITotpService project#417
Conversation
Reviewer's Guide by SourceryThis pull request adds a new project No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the default/fallback hardcoded secret key ('OMM2LVLFX6QJHMYI') to ensure explicit configuration and avoid potential security risks.
- The
VerifyandGetRemainingSecondsmethods inDefaultTotpServicerely on state (Instance) set byCompute; consider making the service stateless or clarifying the intended usage pattern, especially given its Singleton registration.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var mode = options.Algorithm.ToMode(); | ||
| var uri = new OtpUri(type, options.SecretKey, options.UserName, options.IssuerName, mode, options.Digits, options.Period, options.Counter); | ||
| return uri.ToString(); | ||
| } |
There was a problem hiding this comment.
question (bug_risk): Consider thread-safety concerns with the mutable Instance property.
The Instance property is updated in the Compute method and then used in GetRemainingSeconds and Verify. In concurrent scenarios, this mutable field could lead to race conditions or unexpected behavior. It might be worthwhile to review thread-safety or consider a stateless approach.
| return Base32Encoding.ToBytes(input); | ||
| } | ||
|
|
||
| public bool Verify(string code, DateTime? timestamp = null) |
There was a problem hiding this comment.
suggestion (bug_risk): Consistent use of configured secret key in Verify method.
Similar to GetRemainingSeconds, if Instance is null, Verify creates a Totp using a hard-coded secret. It might be beneficial to ensure that verification uses the same configuration from OtpOptions to avoid unexpected results.
Suggested implementation:
var instance = new Totp(GetSecretKeyBytes(OtpOptions.Secret));
return timestamp == null ? instance.RemainingSeconds() : instance.RemainingSeconds(timestamp.Value);Ensure that the DefaultTotpService class has the OtpOptions (or similarly named options object) properly injected and that the property containing the secret key is named "Secret". If your property name is different, please update the field accordingly.
1 participant
Link issues
fixes #416
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a Time-based One-Time Password (TOTP) service.
New Features:
ITotpServiceand its default implementationDefaultTotpService.AddBootstrapBlazorTotpServicefor service registration.