Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4139 +/- ##
==========================================
+ Coverage 75.94% 76.03% +0.09%
==========================================
Files 303 307 +4
Lines 11352 11448 +96
Branches 1407 1418 +11
==========================================
+ Hits 8621 8705 +84
- Misses 2330 2337 +7
- Partials 401 406 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
imnasnainaec
changed the title
imnasnainaec
force-pushed
the
lexbox-login
branch
3 times, most recently
from
d49af8a to
d54a65d
Compare
imnasnainaec
force-pushed
the
lexbox-login
branch
from
d54a65d to
525194b
Compare
github-actions
bot
added
the
dependencies
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
@myieye reviewed 17 files and all commit messages, and made 6 comments.
Reviewable status: 17 of 24 files reviewed, all discussions resolved (waiting on imnasnainaec).
Backend/Startup.cs line 137 at r3 (raw file):
var key = ASCII.GetBytes(secretKey); var lexboxAuthority = Configuration["LexboxAuth:Authority"]?.Trim().TrimEnd('/');
It's largely a syntax thing, but you could get rid of almost all of this by putting the values (fallback as well as any overrides you're wanting) in appsettings.json files and doing this:
configuration.Bind("LexboxAuth:Oidc", options)
See: AuthKernel.cs
Backend/Startup.cs line 149 at r3 (raw file):
?? "https://lexbox.org/.well-known/openid-configuration"; var lexboxPrompt = Configuration["LexboxAuth:Prompt"] ?? "select_account"; var lexboxScope = Configuration["LexboxAuth:Scope"] ?? "profile openid offline_access sendandreceive";
offline_access is primarily for refresh tokens. So, I'm pretty sure you can leave that one out.
Backend/Controllers/AuthController.cs line 34 at r3 (raw file):
using var activity = OtelService.StartActivityWithTag(otelTagName, "getting auth status"); if (!_permissionService.IsCurrentUserAuthenticated(HttpContext))
I expect there's a class attribute available to handle this check.
Backend/Controllers/AuthController.cs line 45 at r3 (raw file):
} var user = GetUserFromClaims(result.Principal);
Since GetUserFromClaims can return null, you could consider letting it also take null and handling the if case above.
Backend/Controllers/AuthController.cs line 105 at r3 (raw file):
private static LexboxAuthUser? GetUserFromClaims(System.Security.Claims.ClaimsPrincipal principal) { var userId = principal.FindFirst("sub")?.Value?.Trim();
This method would probably be more readable if you try to get an ID first. If that doesn't work, return null.
Then try to get a displayName and return the object.
There's also only a single use of System.Security.Claims.ClaimTypes. I don't know what's in there, but presumably more constants that would make other FindFirsts more consistent and readable.
It's also maybe more confusing than helpful to have so many display name fallbacks. I'm not entirely sure which claims we should expect here, but it might be worth checking and simplifying.
I'd also be tempted to throw exceptions for things we don't expect.
E.g. Is it possible for the sub claim to be missing? Should NameIdentifier really be a fallback for userId? I'd expect the ID claim to be vey stable whichever it is. Presumably sub. That's what we use.
Backend.Tests/Controllers/AuthControllerTests.cs line 89 at r3 (raw file):
var result = await _controller.GetLexboxLogin(); Assert.That(result, Is.InstanceOf<EmptyResult>());
EmptyResult doesn't sound like the same thing as "ReturnsExpectedLoginPath"
imnasnainaec
force-pushed
the
lexbox-login
branch
from
b8b2bfc to
8956f13
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: 16 of 24 files reviewed, all discussions resolved (waiting on myieye).
Backend/Controllers/AuthController.cs line 34 at r3 (raw file):
Previously, myieye (Tim Haasdyk) wrote…
I expect there's a class attribute available to handle this check.
There is, but this is the pattern we have in all our controllers. It should be updated everywhere... some other time.
Backend/Controllers/AuthController.cs line 105 at r3 (raw file):
Previously, myieye (Tim Haasdyk) wrote…
This method would probably be more readable if you try to get an ID first. If that doesn't work, return null.
Then try to get a displayName and return the object.There's also only a single use of
System.Security.Claims.ClaimTypes. I don't know what's in there, but presumably more constants that would make otherFindFirsts more consistent and readable.It's also maybe more confusing than helpful to have so many display name fallbacks. I'm not entirely sure which claims we should expect here, but it might be worth checking and simplifying.
I'd also be tempted to throw exceptions for things we don't expect.
E.g. Is it possible for the sub claim to be missing? Should NameIdentifier really be a fallback for userId? I'd expect the ID claim to be vey stable whichever it is. Presumably sub. That's what we use.
Updated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
Backend/Models/LexboxAuthUser.cs (1)
5-6: Tighten auth-user invariants with non-null required properties.Line 5-Line 6 are nullable, but this model represents authenticated identity data. Making these
requirednon-null avoids accidental invalid instances.Proposed refactor
- public string? UserId { get; init; } - public string? DisplayName { get; init; } + public required string UserId { get; init; } + public required string DisplayName { get; init; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/Models/LexboxAuthUser.cs` around lines 5 - 6, The LexboxAuthUser model currently allows nullable UserId and DisplayName which undermines authenticated identity invariants; update the LexboxAuthUser type so UserId and DisplayName are non-nullable and required (e.g., change from string? to string and mark as required or add a constructor that enforces them) so instances cannot be created without valid values; target the UserId and DisplayName properties on the LexboxAuthUser class when making this change.src/components/Lexbox/LexboxLogin.tsx (2)
53-63: Consider callingonStatusChange("logged-in")when status changes to logged-in.The
onStatusChangecallback is only invoked for logout. For consistency and to allow parent components to react to login events, consider also calling it when the user becomes logged in (afterloadStatusdetects a logged-in state).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Lexbox/LexboxLogin.tsx` around lines 53 - 63, The component currently only calls props.onStatusChange("logged-out") inside handleLogout; add a matching call for login events by invoking props.onStatusChange("logged-in") when loadStatus detects the user is logged in (e.g., after successful login flow or inside the code path that processes loadStatus results). Locate the login/status-refresh logic (functions loadStatus and any login handler) and call props.onStatusChange("logged-in") when the status returned by loadStatus indicates a logged-in state so parent components are notified consistently; keep existing handleLogout behavior intact.
49-51: Remove unnecessaryasyncor add loading state for login.The
handleLoginfunction is markedasyncbut doesn'tawaitanything. Either removeasyncfor clarity, or consider adding loading state feedback if the login flow should show a spinner.Additionally, consider handling the case where
window.open()returnsnull(popup blocked).♻️ Suggested fix
- const handleLogin = async (): Promise<void> => { - window.open(getLexboxLoginUrl()); + const handleLogin = (): void => { + const popup = window.open(getLexboxLoginUrl()); + if (!popup) { + console.warn("Popup was blocked"); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Lexbox/LexboxLogin.tsx` around lines 49 - 51, The handleLogin function is marked async but doesn't await anything and doesn't handle popup blocking; either remove the async keyword from handleLogin (in LexboxLogin component) to reflect it’s synchronous, or implement a simple loading state (e.g., setLoading true/false) around the login flow and await any async operations, and after calling window.open(getLexboxLoginUrl()) check the returned window handle for null and handle that case (e.g., show an error/toast informing the user their popup was blocked).Backend/appsettings.json (1)
2-16: Consider making ClientId configurable via environment variable.The
ClientIdis hardcoded to"the-combine". For flexibility across different deployment environments (development, staging, production), consider allowing this to be overridden via environment variable, similar to how other sensitive configuration is handled inStartup.cs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/appsettings.json` around lines 2 - 16, The LexboxAuth.ClientId is hardcoded; update the auth configuration to accept an environment override: read an env var (e.g. LEXBOX_CLIENT_ID) and use it if present, otherwise fall back to the existing "LexboxAuth:ClientId" value from appsettings.json. Implement this change where the LexboxAuth settings are bound/used (e.g., in Startup.ConfigureServices or the code that constructs the authentication options), referencing the "LexboxAuth" section and "ClientId" key so deployments can override client ID without changing appsettings.json.src/components/Lexbox/tests/LexboxLogin.test.tsx (1)
26-41: Consider adding a test for error handling.The test suite covers the happy paths well. However, the component has error handling in
loadStatus()(lines 37-39 in LexboxLogin.tsx) that sets status toundefinedon failure. Consider adding a test case to verify the component behavior whengetLexboxAuthStatusrejects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Lexbox/tests/LexboxLogin.test.tsx` around lines 26 - 41, Add a new test on LexboxLogin that simulates getLexboxAuthStatus rejecting to exercise the loadStatus error path: have mockGetLexboxAuthStatus.mockRejectedValueOnce(new Error("fail")) (or similar), render <LexboxLogin /> inside act, wait for loadStatus to run, then assert the component set status to undefined by checking the UI state (e.g., login button is disabled or an error message/placeholder is shown) and ensure no unexpected calls to mockGetLexboxLoginUrl or window.open occur; reference the loadStatus function, mockGetLexboxAuthStatus, LexboxLogin component, and mockGetLexboxLoginUrl when locating where to add the test.Backend/Controllers/AuthController.cs (2)
22-22: Consider using PascalCase for the constant name.C# naming conventions recommend PascalCase for
constfields. Consider renaming toOtelTagName.- private const string otelTagName = "otel.AuthController"; + private const string OtelTagName = "otel.AuthController";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/Controllers/AuthController.cs` at line 22, Rename the const field otelTagName to PascalCase OtelTagName and update all references to it (e.g., in AuthController methods and any callers) to use the new identifier; ensure the const's value remains unchanged and run/adjust any unit tests or usages that reference the old otelTagName symbol so the code compiles.
65-76: Consider using POST instead of GET for the logout endpoint.Using GET for logout creates a minor CSRF risk — an attacker could embed the logout URL in an
<img>tag to force users to log out. While the impact is low (user inconvenience only), using POST with anti-forgery protection is the safer pattern for state-changing operations.- [HttpGet("lexbox-logout", Name = "LogOutLexbox")] + [HttpPost("lexbox-logout", Name = "LogOutLexbox")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/Controllers/AuthController.cs` around lines 65 - 76, Change the logout endpoint from GET to a POST to avoid CSRF risk: update the attribute on the LogOutLexbox action from [HttpGet("lexbox-logout", Name = "LogOutLexbox")] to an HTTP POST attribute (e.g., [HttpPost("lexbox-logout", Name = "LogOutLexbox")]) and add anti-forgery validation (e.g., [ValidateAntiForgeryToken]) to the method; keep the existing method body that calls OtelService.StartActivityWithTag and SignOutAsync with LexboxCookieScheme and LexboxOidcScheme, and update any clients or tests that call LogOutLexbox to send a POST with the antiforgery token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Backend/BackendFramework.csproj`:
- Around line 21-24: The project currently mixes
Microsoft.AspNetCore.Authentication.JwtBearer (8.0.24) with
Microsoft.IdentityModel.Tokens and System.IdentityModel.Tokens.Jwt (8.16.0),
causing a runtime API mismatch; fix by aligning versions: either downgrade
Microsoft.IdentityModel.Tokens and System.IdentityModel.Tokens.Jwt to the 7.x
line (e.g., 7.1.2) to match JwtBearer 8.0.24, or upgrade
Microsoft.AspNetCore.Authentication.JwtBearer to a release that officially
supports IdentityModel 8.x; update the PackageReference entries for
Microsoft.AspNetCore.Authentication.JwtBearer, Microsoft.IdentityModel.Tokens,
and System.IdentityModel.Tokens.Jwt accordingly so all three packages are on a
mutually compatible major version.
In `@src/components/Lexbox/LexboxLogin.tsx`:
- Around line 45-47: The component LexboxLogin currently calls loadStatus() only
on mount inside the useEffect, so add a mechanism to refresh status after OAuth
completes: attach a window "focus" event listener in the same useEffect (or a
new one) that calls loadStatus when the window regains focus, and remove the
listener on cleanup; alternatively (optional) implement a BroadcastChannel or
localStorage postMessage in your OAuth popup flow to notify the main window to
call loadStatus, or start a short polling interval while login is in
progress—update the useEffect around loadStatus (and any openOAuthWindow
handler) to include the focus listener/cleanup or the messaging/polling
subscribed/unsubscribed logic.
In `@src/components/ProjectScreen/CreateProject.tsx`:
- Around line 276-279: The LexboxLogin component is currently rendered inside
the project form in CreateProject, which can cause its internal buttons to
trigger the form's createProject submit; move the <LexboxLogin /> JSX out of the
form element (place it before or after the <form> that wraps the project inputs)
so its buttons are not nested inside the form, and verify createProject remains
attached only to the form's onSubmit handler.
---
Nitpick comments:
In `@Backend/appsettings.json`:
- Around line 2-16: The LexboxAuth.ClientId is hardcoded; update the auth
configuration to accept an environment override: read an env var (e.g.
LEXBOX_CLIENT_ID) and use it if present, otherwise fall back to the existing
"LexboxAuth:ClientId" value from appsettings.json. Implement this change where
the LexboxAuth settings are bound/used (e.g., in Startup.ConfigureServices or
the code that constructs the authentication options), referencing the
"LexboxAuth" section and "ClientId" key so deployments can override client ID
without changing appsettings.json.
In `@Backend/Controllers/AuthController.cs`:
- Line 22: Rename the const field otelTagName to PascalCase OtelTagName and
update all references to it (e.g., in AuthController methods and any callers) to
use the new identifier; ensure the const's value remains unchanged and
run/adjust any unit tests or usages that reference the old otelTagName symbol so
the code compiles.
- Around line 65-76: Change the logout endpoint from GET to a POST to avoid CSRF
risk: update the attribute on the LogOutLexbox action from
[HttpGet("lexbox-logout", Name = "LogOutLexbox")] to an HTTP POST attribute
(e.g., [HttpPost("lexbox-logout", Name = "LogOutLexbox")]) and add anti-forgery
validation (e.g., [ValidateAntiForgeryToken]) to the method; keep the existing
method body that calls OtelService.StartActivityWithTag and SignOutAsync with
LexboxCookieScheme and LexboxOidcScheme, and update any clients or tests that
call LogOutLexbox to send a POST with the antiforgery token.
In `@Backend/Models/LexboxAuthUser.cs`:
- Around line 5-6: The LexboxAuthUser model currently allows nullable UserId and
DisplayName which undermines authenticated identity invariants; update the
LexboxAuthUser type so UserId and DisplayName are non-nullable and required
(e.g., change from string? to string and mark as required or add a constructor
that enforces them) so instances cannot be created without valid values; target
the UserId and DisplayName properties on the LexboxAuthUser class when making
this change.
In `@src/components/Lexbox/LexboxLogin.tsx`:
- Around line 53-63: The component currently only calls
props.onStatusChange("logged-out") inside handleLogout; add a matching call for
login events by invoking props.onStatusChange("logged-in") when loadStatus
detects the user is logged in (e.g., after successful login flow or inside the
code path that processes loadStatus results). Locate the login/status-refresh
logic (functions loadStatus and any login handler) and call
props.onStatusChange("logged-in") when the status returned by loadStatus
indicates a logged-in state so parent components are notified consistently; keep
existing handleLogout behavior intact.
- Around line 49-51: The handleLogin function is marked async but doesn't await
anything and doesn't handle popup blocking; either remove the async keyword from
handleLogin (in LexboxLogin component) to reflect it’s synchronous, or implement
a simple loading state (e.g., setLoading true/false) around the login flow and
await any async operations, and after calling window.open(getLexboxLoginUrl())
check the returned window handle for null and handle that case (e.g., show an
error/toast informing the user their popup was blocked).
In `@src/components/Lexbox/tests/LexboxLogin.test.tsx`:
- Around line 26-41: Add a new test on LexboxLogin that simulates
getLexboxAuthStatus rejecting to exercise the loadStatus error path: have
mockGetLexboxAuthStatus.mockRejectedValueOnce(new Error("fail")) (or similar),
render <LexboxLogin /> inside act, wait for loadStatus to run, then assert the
component set status to undefined by checking the UI state (e.g., login button
is disabled or an error message/placeholder is shown) and ensure no unexpected
calls to mockGetLexboxLoginUrl or window.open occur; reference the loadStatus
function, mockGetLexboxAuthStatus, LexboxLogin component, and
mockGetLexboxLoginUrl when locating where to add the test.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.vscode/settings.jsonBackend.Tests/Controllers/AuthControllerTests.csBackend.Tests/Mocks/AuthenticationServiceMock.csBackend/BackendFramework.csprojBackend/Controllers/AuthController.csBackend/Models/AuthStatus.csBackend/Models/LexboxAuthUser.csBackend/Startup.csBackend/appsettings.jsonsrc/api/.openapi-generator/FILESsrc/api/api.tssrc/api/api/auth-api.tssrc/api/models/auth-status.tssrc/api/models/index.tssrc/backend/index.tssrc/components/Lexbox/LexboxLogin.tsxsrc/components/Lexbox/tests/LexboxLogin.test.tsxsrc/components/ProjectScreen/CreateProject.tsxsrc/components/ProjectScreen/tests/CreateProject.test.tsx
|
Progress... |
2 participants
Working toward some form of #3719
Vibe coding locally with GitHub Copilot.
https://app.devin.ai/review/sillsdev/TheCombine/pull/4139
This change is
Summary by CodeRabbit
Release Notes
New Features
Tests