Improve bit Boilerplate identity (#12792) - #12793
Merged
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe boilerplate identity flows now validate return URLs, handle missing or replayed tokens, support two-factor confirmation safely, localize phone parsing errors, adjust external authentication configuration, and add integration and UI tests. ChangesIdentity hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConfirmPage
participant IdentityController
participant IdentityStore
ConfirmPage->>IdentityController: Confirm email or phone token
IdentityController->>IdentityStore: Confirm contact
IdentityStore-->>IdentityController: two-factor-required or success
IdentityController-->>ConfirmPage: confirmation result
ConfirmPage->>IdentityStore: store authentication tokens only on success
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs (1)
25-41: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSolid fix: converts an unhandled 500 into a controlled 400 for anonymous callers.
Wrapping
Parse/Formatand returning a localizedBadRequestExceptionis correct given libphonenumber's stricter validation than the[Phone]attribute.One optional note: the exception is not logged before conversion. Since the comment acknowledges that header-derived regions (
XX,T1) can cause false failures for legitimate users, consider a low-level log entry to keep visibility into how often this path triggers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs` around lines 25 - 41, Optionally add low-level logging in the NumberParseException catch block before throwing BadRequestException, recording the parse failure and caller-influenced region while preserving the existing localized 400 conversion and exception data.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate security-validation logic across three files.
GetSafeReturnUrl()is defined identically inNotAuthorizedPage.razor.cs,SignInPanel.razor.cs, andConfirmPage.razor.cs. Extract this into a single shared helper, for example a static extension method next toUri.IsAppRelativeUrlinUriExtensions.cs. A single implementation reduces the risk that a future change to the return-URL rule updates only some call sites.♻️ Proposed extraction
// UriExtensions.cs public static string GetSafeReturnUrl(this string? returnUrl) => IsAppRelativeUrl(returnUrl, requireLeadingSlash: false) ? returnUrl! : PageUrls.Home;- private string GetSafeReturnUrl() => Uri.IsAppRelativeUrl(ReturnUrl, requireLeadingSlash: false) ? ReturnUrl : PageUrls.Home; + private string GetSafeReturnUrl() => ReturnUrl.GetSafeReturnUrl();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs` at line 12, Extract the shared return-URL validation from GetSafeReturnUrl in NotAuthorizedPage, SignInPanel, and ConfirmPage into one helper alongside Uri.IsAppRelativeUrl in UriExtensions.cs. Update all three call sites to use the shared helper and remove their duplicate local implementations, preserving the existing fallback to PageUrls.Home.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs`:
- Around line 21-25: Update the return URL validation at the external sign-in
callback’s Uri.IsAppRelativeUrl call to pass requireLeadingSlash: false,
matching GetSafeReturnUrl and preserving valid rootless return URLs instead of
redirecting them home.
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs`:
- Around line 95-102: Update the external sign-in callback around user creation
and AddLoginAsync to track whether the callback created the user; when login
association fails, delete only that newly created user before throwing the
existing ResourceValidationException. Preserve existing users found by email or
phone, and retain the current successful-association flow.
---
Nitpick comments:
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs`:
- Line 12: Extract the shared return-URL validation from GetSafeReturnUrl in
NotAuthorizedPage, SignInPanel, and ConfirmPage into one helper alongside
Uri.IsAppRelativeUrl in UriExtensions.cs. Update all three call sites to use the
shared helper and remove their duplicate local implementations, preserving the
existing fallback to PageUrls.Home.
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs`:
- Around line 25-41: Optionally add low-level logging in the
NumberParseException catch block before throwing BadRequestException, recording
the parse failure and caller-influenced region while preserving the existing
localized 400 conversion and exception data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a5e5ba3-bb8f-4358-aa18-42066888e8fc
📒 Files selected for processing (17)
src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.jsonsrc/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/ConfirmPage.razor.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razorsrc/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.EmailConfirmation.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.PhoneConfirmation.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ResetPassword.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/Services/RemoteAuthenticationOptionsConfigurator.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Infrastructure/Extensions/UriExtensions.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/BunitUITests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ConfirmPageTwoFactorTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/OneTimeTokenTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/PhoneNumberNormalizationUITests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ReturnUrlHardeningTests.cs
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
1 participant
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.
closes #12792
Summary by CodeRabbit