chore: implement oauth state for every provider#116
Conversation
…ing in 3pontos homepage.
… renderHook in EventPanelProvider
WalkthroughImplements tenant-aware OAuth state and routing, updates OAuth clients/contracts to accept an OAuthStateDTO, refactors authentication to handle tenant flows, adds tenant logout controller/response and route, updates login UI and Filament theming for tenant context, and adds service toggles for OAuth providers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Provider as OAuthProvider
participant OAuthController
participant AuthenticateAction
participant Session
User->>Frontend: Clicks OAuth login (tenant context)
Frontend->>Provider: GET /authorize?state=OAuthStateDTO(panel,tenant)
Provider->>OAuthController: Callback(code, state)
OAuthController->>AuthenticateAction: withOAuth(state: OAuthStateDTO, code)
AuthenticateAction->>AuthenticateAction: authenticateTenant() [if state.tenant]
AuthenticateAction->>Session: create/authenticate tenant user
OAuthController-->>Frontend: Redirect to panel-aware URL
sequenceDiagram
participant User
participant Browser
participant TenantLogoutController
participant Filament
participant Session
participant TenantLogoutResponse
User->>Browser: Clicks Logout
Browser->>TenantLogoutController: POST /{tenantSlug}/logout
TenantLogoutController->>Session: capture panel & tenant
TenantLogoutController->>Filament: perform logout
TenantLogoutController->>Session: invalidate & regenerate
TenantLogoutController->>Session: restore tenant & panel
TenantLogoutController->>TenantLogoutResponse: return response
TenantLogoutResponse->>Filament: set current panel
TenantLogoutResponse-->>User: redirect to panel login
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 12
🧹 Nitpick comments (15)
app-modules/events/src/Filament/Events/EventLandingPage.php (2)
16-17: Clarify the purpose of the emptymount()method.The
mount()lifecycle hook is present but contains no logic. Comparing toEventLogin.php(lines 59-69), a typical mount implementation would validate the tenant and handle authenticated user redirects. SincegetView()already validates tenant existence (line 33), consider either:
- Implementing authentication checks and redirects in
mount()if needed, or- Removing the empty method if it serves no purpose
21-35: Consider extracting duplicated tenant resolution logic.The tenant slug extraction logic (lines 21-29) is duplicated in
ParticipantDashboard.php(lines 24-32). This pattern appears in multiple places and could be extracted into a shared helper method or trait to improve maintainability.app-modules/he4rt/resources/views/components/partials/oauth-connect.blade.php (1)
16-16: Consider graceful handling when OAuth provider credentials are missing.While
isEnabled()checks if the provider is enabled, it may not validate that all required credentials (client_id, client_secret) are configured. IfgetRedirectUri()is called with incomplete credentials, the OAuth flow could fail silently or produce unclear errors for users.app-modules/events/resources/views/components/themes/3pontos/participant-dashboard.blade.php (1)
1-23: Tenant dashboard layout and footer look consistentThe dashboard composition and footer props mirror the homepage theme and are straightforward. Since the
:columnsstructure is duplicated here and in the homepage, consider extracting this footer config into a shared partial/config if it starts to diverge across pages, but it’s fine as-is.app-modules/events/resources/views/components/themes/3pontos/homepage.blade.php (2)
12-14: Consider guarding the decorative image against accidental overlapThe decorative logo with
alt=""is good for accessibility. Depending on how other sections set their z-index, theabsolute+ transforms atz-0could still sit over interactive content. If you notice click/hover issues, consider addingpointer-events-noneor adjusting z-index to keep it firmly in the background.
16-34: Shared footer config duplicated with dashboardFooter usage and props look correct and consistent with the participant dashboard. As this navigation grows, you may want to centralize the
:columns/copy for 3pontos so homepage and dashboard stay in sync without duplication, but it’s not urgent.app-modules/he4rt/resources/css/support/themes.css (1)
162-165: Helper tokens now map to lighter tints – verify contrast in usagesSwitching
--helper-success|warning|error|infoto*-300makes helpers noticeably lighter. That’s fine, but it would be good to spot-check key components (alerts, badges, form feedback) for sufficient contrast in both light and dark themes; if needed, you can later add.dark-specific overrides for these helper tokens.app/Providers/FilamentServiceProvider.php (1)
40-47: Consider externalizing tenant color configuration.The hardcoded match expression limits flexibility. As more tenants are added, this will become a maintenance burden.
Consider moving color configuration to a config file or database:
- $color = match ($tenantSlug) { - '3pontos' => Color::Gray, - default => Color::Indigo - }; + $color = config("tenants.colors.{$tenantSlug}", Color::Indigo);Then create
config/tenants.php:return [ 'colors' => [ '3pontos' => \Filament\Support\Colors\Color::Gray, // Add more tenants here ], ];app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php (1)
22-24: Consider extracting redirect logic for clarity.The ternary with
Filament::hasLogin()is compact but could be clearer. The redirect targets also assume certain contexts.- return redirect()->to( - Filament::hasLogin() ? Filament::getLoginUrl(['tenantSlug' => $tenantSlug]) : Filament::getUrl($tenant), - ); + $redirectUrl = Filament::hasLogin() + ? Filament::getLoginUrl(['tenantSlug' => $tenantSlug]) + : Filament::getUrl($tenant); + + return redirect()->to($redirectUrl);app-modules/integrations/src/Twitch/OAuth/Client/TwitchOAuthClient.php (1)
18-26: State parameter is optional for Twitch OAuth—current implementation is acceptable.Twitch's OAuth documentation shows the state parameter is optional, though strongly recommended to be non-empty for CSRF protection. The current code passes an empty string when
$stateis null, which Twitch will accept without issue.However, for cleaner URLs and clearer intent, consider omitting the state parameter when null:
public function redirectUrl(?OAuthStateDTO $state = null): string { - return 'https://id.twitch.tv/oauth2/authorize?'.http_build_query([ + $params = [ 'client_id' => config('services.twitch.client_id'), 'redirect_uri' => config('services.twitch.redirect_uri'), 'response_type' => 'code', 'scope' => config('services.twitch.scopes'), - 'state' => (string) $state, - ]); + ]; + if ($state) { + $params['state'] = (string) $state; + } + return 'https://id.twitch.tv/oauth2/authorize?'.http_build_query($params); }app-modules/authentication/src/DTO/OAuthStateDTO.php (1)
18-26: Consider more defensive handling of malformed state payloads
fromHashedString()assumes successful decryption and JSON decoding; a tampered or corrupted$statewill causejson_decode()to returnnullor a non-array and then fail at argument unpacking. Wrapping decryption/decoding in a try/catch and validating the decoded array shape beforenew self(...)would give you a controlled failure path (e.g., 400/403) instead of a generic runtime error.app-modules/events/src/Filament/Events/ParticipantDashboard.php (1)
13-43: Align auth guard and consider reusing tenant view resolution logicTwo small suggestions here:
shouldRegisterNavigation()currently usesauth()->check(). To stay in lockstep with the panel’s configured guard, consider usingFilament::auth()->check()instead, so navigation visibility always matches Filament’s notion of an authenticated user.getView()duplicates the tenant‑slug and view‑path resolution fromEventLandingPage. Extracting this into a shared helper (or trait) would reduce duplication and keep tenant routing rules in one place.app/Providers/Filament/EventPanelProvider.php (1)
41-68: Harden tenant‑dependent login/logout URLs against missing tenant contextThe panel setup looks coherent with the new tenant login/logout flow; a couple of edge cases are worth double‑checking:
- In the TOPBAR_END hook,
Filament::getLoginUrl(['tenantSlug' => session()->get('tenant')])assumessession('tenant')is always set. If a guest ever hits the panel without that session key (e.g., first request beforeGuestTenantIdentifierruns),getLoginUrl()could error due to a missing required route parameter. A defensive fallback (e.g., deriving the slug from the current tenant or host) would make this more robust.- The logout user‑menu Action builds its URL from
filament()->getTenant()?->slug. IfgetTenant()can benullin any scenario,route('tenant.logout', ['tenantSlug' => null])will similarly fail. You might want to guard this or hide the logout item when no tenant is resolved.@guestuses Laravel’s default auth guard; if the event panel ever uses a custom guard, usingFilament::auth()->check()(or the corresponding Blade directives) would keep the button visibility in sync with Filament authentication.app-modules/events/src/Filament/Shared/EventLogin.php (2)
46-57: Document dynamic form properties to satisfy phpstan (and future readers)Phpstan is flagging
$form,$multiFactorChallengeForm, and$registerActionas undefined properties, even though Filament’s page base/traits provide them dynamically. To make this explicit and quiet static analysis, consider adding a class‑level phpdoc block describing these properties as dynamically resolved:+/** + * @property mixed $form + * @property mixed $multiFactorChallengeForm + * @property \Filament\Actions\Action $registerAction + */ class EventLogin extends SimplePage {This doesn’t affect runtime behavior (the properties are still provided by Filament / Livewire), but it gives tools and maintainers a clear contract that these members exist.
Also applies to: 69-70, 82-83, 100-105, 162-167, 194-197
60-70: Confirm tenant slug handling inmount()and avoid exposing non-existent tenants
mount()immediately aborts with 404 when the given$tenantSlugdoesn’t exist and then stores it on the instance:abort_unless(Tenant::query()->where('slug', $tenantSlug)->exists(), 404); $this->tenantSlug = $tenantSlug;That’s a good safety net; just make sure:
- Every route pointing at
EventLoginactually passes a non‑null{tenantSlug}that matches yourTenantmodel.- There’s no scenario where guests can hit the login URL without having a valid tenant (e.g., direct
/event/loginwithout slug), otherwise they’ll see an immediate 404.If you expect a “default” tenant or want a friendlier error page, you might eventually want a dedicated exception handler or redirect instead of a raw 404.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
app-modules/authentication/routes/authentication-routes.php(1 hunks)app-modules/authentication/src/Actions/AuthenticateAction.php(2 hunks)app-modules/authentication/src/Contracts/OAuthClientContract.php(1 hunks)app-modules/authentication/src/DTO/OAuthStateDTO.php(1 hunks)app-modules/authentication/src/Enums/OAuthProviderEnum.php(2 hunks)app-modules/authentication/src/Http/Controllers/OAuthController.php(1 hunks)app-modules/authentication/src/Http/Controllers/TenantLogoutController.php(1 hunks)app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php(1 hunks)app-modules/authentication/src/Providers/AuthenticationServiceProvider.php(0 hunks)app-modules/events/resources/views/components/themes/3pontos/components/sections/event-progress.blade.php(1 hunks)app-modules/events/resources/views/components/themes/3pontos/homepage.blade.php(1 hunks)app-modules/events/resources/views/components/themes/3pontos/participant-dashboard.blade.php(1 hunks)app-modules/events/src/EventPanelPlugin.php(2 hunks)app-modules/events/src/Filament/Events/EventLandingPage.php(1 hunks)app-modules/events/src/Filament/Events/ParticipantDashboard.php(1 hunks)app-modules/events/src/Filament/Shared/EventLogin.php(1 hunks)app-modules/events/src/Filament/Shared/GuestSidebar.php(1 hunks)app-modules/events/src/Filament/Shared/GuestTopbar.php(1 hunks)app-modules/he4rt/resources/css/components/avatar.css(1 hunks)app-modules/he4rt/resources/css/components/button.css(2 hunks)app-modules/he4rt/resources/css/index.css(1 hunks)app-modules/he4rt/resources/css/support/themes.css(1 hunks)app-modules/he4rt/resources/views/components/button.blade.php(1 hunks)app-modules/he4rt/resources/views/components/card.blade.php(1 hunks)app-modules/he4rt/resources/views/components/partials/oauth-connect.blade.php(1 hunks)app-modules/integrations/src/Discord/OAuth/DiscordOAuthClient.php(1 hunks)app-modules/integrations/src/Twitch/OAuth/Client/TwitchOAuthClient.php(1 hunks)app/Providers/Filament/EventPanelProvider.php(3 hunks)app/Providers/FilamentServiceProvider.php(2 hunks)config/services.php(1 hunks)
💤 Files with no reviewable changes (1)
- app-modules/authentication/src/Providers/AuthenticationServiceProvider.php
🧰 Additional context used
🧬 Code graph analysis (12)
app-modules/events/src/Filament/Events/EventLandingPage.php (3)
app-modules/events/src/Filament/Shared/EventLogin.php (1)
mount(60-70)app/Filament/Pages/Login.php (1)
mount(9-19)app-modules/events/src/Filament/Events/ParticipantDashboard.php (1)
getView(25-43)
app-modules/integrations/src/Twitch/OAuth/Client/TwitchOAuthClient.php (3)
app-modules/authentication/src/DTO/OAuthStateDTO.php (2)
OAuthStateDTO(11-36)__construct(13-16)app-modules/authentication/src/Contracts/OAuthClientContract.php (1)
redirectUrl(13-13)app-modules/integrations/src/Discord/OAuth/DiscordOAuthClient.php (1)
redirectUrl(15-24)
app-modules/integrations/src/Discord/OAuth/DiscordOAuthClient.php (4)
app-modules/authentication/src/DTO/OAuthStateDTO.php (1)
OAuthStateDTO(11-36)app-modules/authentication/src/DTO/OAuthUserDTO.php (1)
OAuthUserDTO(9-31)app-modules/authentication/src/Contracts/OAuthClientContract.php (1)
redirectUrl(13-13)app-modules/integrations/src/Twitch/OAuth/Client/TwitchOAuthClient.php (1)
redirectUrl(18-27)
app-modules/authentication/src/Actions/AuthenticateAction.php (1)
app-modules/authentication/src/DTO/OAuthStateDTO.php (1)
OAuthStateDTO(11-36)
app-modules/authentication/src/Http/Controllers/OAuthController.php (5)
app-modules/authentication/src/DTO/OAuthStateDTO.php (2)
OAuthStateDTO(11-36)fromHashedString(23-26)app-modules/authentication/src/Enums/OAuthProviderEnum.php (1)
getClient(22-28)app-modules/integrations/src/Discord/OAuth/DiscordOAuthClient.php (1)
redirectUrl(15-24)app-modules/integrations/src/Twitch/OAuth/Client/TwitchOAuthClient.php (1)
redirectUrl(18-27)app-modules/authentication/src/Actions/AuthenticateAction.php (2)
AuthenticateAction(18-87)withOAuth(20-29)
app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php (2)
app-modules/tenant/src/Models/Tenant.php (1)
Tenant(22-94)app/Providers/Filament/EventPanelProvider.php (1)
panel(35-82)
app-modules/authentication/src/Enums/OAuthProviderEnum.php (4)
app-modules/authentication/src/DTO/OAuthStateDTO.php (1)
OAuthStateDTO(11-36)app-modules/authentication/src/Contracts/OAuthClientContract.php (1)
redirectUrl(13-13)app-modules/integrations/src/Discord/OAuth/DiscordOAuthClient.php (1)
redirectUrl(15-24)app-modules/integrations/src/Twitch/OAuth/Client/TwitchOAuthClient.php (1)
redirectUrl(18-27)
app-modules/authentication/src/Http/Controllers/TenantLogoutController.php (2)
app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php (1)
TenantLogoutResponse(11-26)app/Providers/Filament/EventPanelProvider.php (1)
panel(35-82)
app-modules/authentication/src/DTO/OAuthStateDTO.php (4)
app-modules/authentication/src/DTO/OAuthAccessDTO.php (4)
OAuthAccessDTO(7-25)__construct(9-13)toDatabase(17-24)make(15-15)app-modules/authentication/src/DTO/OAuthUserDTO.php (3)
OAuthUserDTO(7-29)__construct(9-17)make(19-19)app-modules/integrations/src/Twitch/OAuth/DTO/TwitchOAuthDTO.php (1)
TwitchOAuthDTO(10-26)app-modules/integrations/src/Twitch/OAuth/DTO/TwitchOAuthAccessDTO.php (1)
TwitchOAuthAccessDTO(9-19)
app-modules/events/src/Filament/Events/ParticipantDashboard.php (2)
app-modules/events/src/Filament/Shared/EventLogin.php (1)
getTitle(207-210)app-modules/events/src/Filament/Events/EventLandingPage.php (1)
getView(18-36)
app-modules/authentication/routes/authentication-routes.php (3)
app-modules/authentication/src/Http/Controllers/TenantLogoutController.php (1)
TenantLogoutController(11-25)app-modules/authentication/src/Http/Controllers/OAuthController.php (1)
OAuthController(14-33)app-modules/authentication/src/Providers/AuthenticationServiceProvider.php (1)
AuthenticationServiceProvider(13-25)
app-modules/events/src/EventPanelPlugin.php (1)
app-modules/events/src/Filament/Events/ParticipantDashboard.php (1)
ParticipantDashboard(11-44)
🪛 GitHub Check: Perform Phpstan Check / Run
app/Providers/FilamentServiceProvider.php
[failure] 49-49:
Call to an undefined method App\Providers\FilamentServiceProvider::getId().
[failure] 45-45:
Call to an undefined method App\Providers\FilamentServiceProvider::colors().
app-modules/events/src/Filament/Shared/EventLogin.php
[failure] 434-434:
Attribute class He4rt\Events\Filament\Shared\SensitiveParameter does not exist.
[failure] 294-294:
Attribute class He4rt\Events\Filament\Shared\SensitiveParameter does not exist.
[failure] 231-231:
Access to an undefined property He4rt\Events\Filament\Shared\EventLogin::$registerAction.
[failure] 121-121:
Access to an undefined property He4rt\Events\Filament\Shared\EventLogin::$multiFactorChallengeForm.
[failure] 104-104:
Access to an undefined property He4rt\Events\Filament\Shared\EventLogin::$multiFactorChallengeForm.
[failure] 82-82:
Access to an undefined property He4rt\Events\Filament\Shared\EventLogin::$form.
[failure] 69-69:
Access to an undefined property He4rt\Events\Filament\Shared\EventLogin::$form.
🪛 PHPMD (2.15.0)
app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php
13-13: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (19)
app-modules/he4rt/resources/css/components/avatar.css (1)
16-18: LGTM!The new
hp-size-xlmodifier fits logically in the avatar sizing scale and follows the established pattern consistently. The change integrates well with existing size modifiers.app-modules/he4rt/resources/views/components/card.blade.php (1)
62-76: Good optimization – no issues found, safe to merge.CSS styling for
.hp-card-bodyis semantically tied to content existence (flex layout for spacing between title and description). All current card usages include either title or description, making the conditional optimization always true in practice. No JavaScript selectors depend on this element, and the change prevents unnecessary DOM rendering without affecting layout or behavior.app-modules/events/resources/views/components/themes/3pontos/components/sections/event-progress.blade.php (2)
38-60: The suggested implementation is architecturally incorrect for this codebase.The buttons are indeed non-functional and the data is hardcoded, but the proposed solution using
wire:click="logout"andwire:click="disconnectAccount"is incompatible with the actual architecture.Why the review is incorrect:
Wrong component type: ParticipantDashboard is a Filament Page (not a Livewire component), and event-progress is a plain Blade component. Livewire directives like
wire:clickrequire a Livewire component class, which doesn't exist in this codebase.Correct approach: The logout should use a form submission with POST method to the existing route
auth/{tenantSlug}/logout, not Livewire method calls.Missing feature: A "disconnect account" (
disconnectAccount) method was suggested but doesn't exist anywhere in the codebase. Only theTenantLogoutControllerlogout functionality is implemented.The buttons need to be connected to the logout route via form submission or a POST link, and the data should be dynamically bound if props are passed to the component. However, this requires understanding how the ParticipantDashboard page should pass user data to the component and implementing the form submission pattern appropriately for a Filament Page context.
Likely an incorrect or invalid review comment.
63-92: Rewrite review comment: Clarify that OAuth management belongs in ConnectionHub, not event-progress.The event-progress component is part of the event participant dashboard and does not appear to be intended for managing user OAuth connections. The codebase already has a dedicated ConnectionHub Livewire component (
app/Livewire/ConnectionHub.php) that properly handles OAuth provider connections with functional disconnect buttons. The hardcoded platform cards in event-progress may be intentional for displaying event-specific metrics rather than user connection status.If this review is targeting user OAuth connection management, the work should focus on ConnectionHub (which already has disconnect functionality), not on modifying the event-progress component.
Likely an incorrect or invalid review comment.
app-modules/events/src/Filament/Shared/GuestSidebar.php (1)
5-5: LGTM! Namespace refactor aligns with shared component organization.The namespace migration from
EventstoSharedproperly reflects the component's role as a shared guest UI element, consistent with the parallel change inGuestTopbar.php.app-modules/he4rt/resources/css/index.css (1)
137-163: LGTM! Comprehensive gray palette enhances theming.The addition of light and dark mode gray color tokens using OKLCH color space provides better color consistency and accessibility. The
!importantflags ensure these tokens take precedence in the theming system.app-modules/events/src/Filament/Shared/GuestTopbar.php (1)
5-5: LGTM! Namespace refactor completes shared component migration.Consistent with
GuestSidebar.php, this namespace change properly organizes shared guest UI components.config/services.php (1)
41-41: LGTM! OAuth provider enable flags provide flexible runtime control.The addition of
enabledconfiguration keys for Discord and Twitch OAuth services allows runtime control over provider availability without code changes. The default value oftruemaintains backward compatibility.Also applies to: 48-48
app-modules/he4rt/resources/views/components/button.blade.php (1)
7-7: LGTM! Default rounding change aligns with updated design system.Changing the default
roundedprop from"sm"to"md"increases border radius for buttons across the application. This aligns with the corresponding CSS updates inbutton.csswhere the rounding token mappings have been reorganized.app-modules/he4rt/resources/css/components/button.css (1)
25-25: LGTM! Button styling updates improve consistency.The changes realign CSS class names with their actual Tailwind rounding utilities:
hp-button-rounded-smnow correctly appliesrounded-sm(previouslyrounded-md)hp-button-rounded-mdnow correctly appliesrounded-md(previouslyrounded-lg)- New
hp-button-rounded-xlclass adds an additional rounding optionThe
py-2padding forxsbuttons (line 25) provides slightly more breathing room. These changes work cohesively with the default rounding update inbutton.blade.php.Also applies to: 77-87
app-modules/events/src/EventPanelPlugin.php (1)
11-25: ParticipantDashboard registration in the panel looks correctImporting and registering
ParticipantDashboard::classalongsideEventLandingPage::classis the right way to expose the new page on the event panel; this matches Filament’s expectations and the dynamic tenant view resolution implemented inParticipantDashboard.app-modules/authentication/src/Contracts/OAuthClientContract.php (1)
8-13: LGTM! Clean interface update for state-driven OAuth flow.The change from
?string $stateto?OAuthStateDTO $statealigns with the PR's objective to implement structured OAuth state handling. All visible implementations (Discord, Twitch) have been updated accordingly.app-modules/authentication/src/Http/Controllers/OAuthController.php (1)
27-29: Verify intended behavior for missing tenant.The
firstOrFail()will throw aModelNotFoundExceptionif the tenant doesn't exist. Confirm this is the desired behavior rather than gracefully handling the error.If graceful handling is preferred, consider:
- $redirectUri = filament() - ->getPanel($state->panel) - ->getUrl(Tenant::query()->where('slug', $state->tenant)->firstOrFail()); + $tenant = Tenant::query()->where('slug', $state->tenant)->first(); + if (! $tenant) { + abort(404, 'Tenant not found'); + } + + $redirectUri = filament() + ->getPanel($state->panel) + ->getUrl($tenant);app/Providers/FilamentServiceProvider.php (2)
45-47: Static analysis hints are false positives.The PHPStan errors about undefined
colors()andgetId()methods are false positives. These methods exist on thePanelclass, and within the macro closure,$thisrefers to the Panel instance, not the ServiceProvider.Also applies to: 49-50
49-50: Reconsider the performance concern—session writes occur during bootstrap, not per-request.The macro is called only once during panel registration in
EventPanelProvider.php:50, not on every request. However, the session writes in lines 49-50 still warrant scrutiny: session values are derived from request data (tenant slug from path/host), yet this initialization happens during app bootstrap before typical request handling. Writing request-dependent values to session during bootstrap is problematic because:
- Request context may not be properly initialized at bootstrap time
- These values are re-derived on each macro invocation anyway (lines 25-33), making session caching ineffective
- Session storage during bootstrap creates unnecessary coupling between initialization and request handling
Consider either removing the session writes entirely (if these values aren't needed elsewhere) or deferring them to middleware where proper request context exists.
app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php (1)
13-13: Static analysis hint about unused parameter is expected.The
$requestparameter is unused but required by theLogoutResponseinterface contract. This is a false positive.app-modules/authentication/src/Actions/AuthenticateAction.php (1)
50-54: Clarify authentication flow logic.The auth flow on Lines 50-54 checks
! auth()->check(), then callsAuth::logout(), thenAuth::login(). This seems contradictory:
- If not authenticated (
! auth()->check()), why callAuth::logout()?- If already authenticated, the user is not logged out as the intended user.
Consider simplifying:
- if (! auth()->check()) { - Auth::logout(); - Auth::login($provider->user); - filament()->auth()->setUser($provider->user); - } + if (! auth()->check() || auth()->id() !== $provider->user->id) { + Auth::logout(); + Auth::login($provider->user); + filament()->auth()->setUser($provider->user); + }Or clarify the intended logic with comments.
app-modules/authentication/routes/authentication-routes.php (1)
12-18: Verify OAuth redirect routes are correctly encoding panel and tenant into stateThe new redirect routes (
oauth.single.redirectandoauth.tenant.redirect) add{panel}and{tenant}segments but still point atOAuthController::getRedirect(OAuthProviderEnum $provider), which only takes$provideras an argument. That’s fine in Laravel (panel/tenant remain available viarequest()->route()), but it does mean the logic that buildsOAuthStateDTOmust now read those route params explicitly (likely inside the OAuth client or controller).Please double‑check:
- The code that calls
route('oauth.single.redirect', ...)/route('oauth.tenant.redirect', ...)passes keys matching{tenant}/{panel}/{provider}.- The redirect handler ultimately constructs
OAuthStateDTOwith the correctpanelandtenantfrom those route params before sending the user to the provider.app-modules/events/src/Filament/Shared/EventLogin.php (1)
72-141: Overall authenticate flow looks solid; just ensure MFA providers are wired as expectedThe
authenticate()method:
- Applies rate limiting via
WithRateLimiting.- Validates credentials against the auth provider.
- Orchestrates multi‑factor authentication using
Filament::getMultiFactorAuthenticationProviders(), includingHasBeforeChallengeHooksupport and challenge form validation.- Uses
attemptWhen()to enforceFilamentUser::canAccessPanel()checks before logging in.- Regenerates the session and returns the panel’s
LoginResponseon success.This aligns well with Filament’s recommended patterns. The main thing to verify in integration tests is that:
- When multiple MFA providers are enabled for a user, the provider radio and conditional challenge groups behave correctly.
- The encrypted
userUndertakingMultiFactorAuthenticationtoken cannot be tampered with from the client (the#[Locked]attribute should cover this) and always decrypts to the same user id being authenticated.Functionally, the structure looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php (2)
app-modules/authentication/src/DTO/OAuthStateDTO.php (1)
OAuthStateDTO(11-36)app-modules/authentication/src/Actions/AuthenticateAction.php (1)
withOAuth(20-29)
🔇 Additional comments (2)
app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php (2)
6-6: LGTM!The import of
OAuthStateDTOis correct and necessary for the refactored OAuth state handling.
60-60: LGTM!The OAuth state is correctly instantiated with the panel ('admin') and tenant slug ('he4rt'), and named parameters improve readability.
5 participants
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.