feat(identity): OAuth flow refactor with account merge strategy#288
Merged
Conversation
…odels in BaseSeeder TenantUserObserver already auto-creates profiles when users are attached to tenants.
Rewrite the OAuth system to be tenant-based only with clear separation of concerns. Controller is now a thin HTTP adapter, business logic lives in dedicated Actions (HandleOAuthCallback, FindOrCreateUserByProvider, AttachProviderToUser). State DTO carries intent (login/link), provider, panel, tenant, and return URL.
Add OAuth social login buttons to the app panel login page with Discord and GitHub providers. Add GitHub service config entry.
Replace hardcoded redirect_uri from config with dynamic URL built from APP_URL. Ensures correct callback domain regardless of reverse proxy.
Admin panel connections now attach to the Tenant (model_type=tenant) instead of the User. Update getTenantProviders query to filter by model_type and use connectedByUser relationship.
Migrate tenants.id from auto-increment bigint to UUID. Update all 31 child tables with tenant_id FK. Add HasUuids trait to Tenant model. Convert all int $tenantId type hints to string across the codebase.
Implement GitHub OAuth integration following the same pattern as Twitch: GitHubOAuthConnector, GitHubApiConnector, ExchangeCodeForToken request, GetCurrentUser request, GitHubOAuthAccessDTO, GitHubOAuthUserDTO, and GitHubOAuthClient. Register in IdentityProvider::getClient().
Redesign connection-hub user view for profile sidebar context: - Smaller icons, reduced padding, removed accent strip and descriptions - Disconnect button as centered pill with hover-red styling - Move timestamp next to provider title, username-only second row - Action buttons as direct flex children for vertical centering
…uth user creation - Remove tenant_id filter from ExternalIdentity lookup (cross-tenant resolution) - Filter by model_type=user to ignore tenant-owned identities - Check username existence before INSERT to avoid PostgreSQL transaction abort - Generate sequential suffix (-2, -3, ...) when username collides - Use DB::transaction savepoint for race condition safety - Skip email lookup when OAuth email is null Closes #284
- Add `first_login_at` nullable timestamp to users table - New `EnrichUserOnFirstLogin` action: updates email, name, attempts username on first real login (when first_login_at is null) - Integrate enrichment into `FindOrCreateUserByProvider` via DI - Username update skipped silently when it would collide - Subsequent logins (first_login_at set) leave user untouched Closes #285
…indOrCreateUserByProvider
- New `DetectMergeConflict` action: checks if external_account_id already belongs to another User (cross-tenant, model_type=user only) - New `MergeConflictDTO`: carries conflicting user ID, provider, credentials, and OAuth user data for session serialization - `HandleOAuthCallbackAction` now checks for conflict before attaching provider on Link intent — returns early with merge conflict in result - `OAuthResultDTO` extended with optional `mergeConflict` field - `OAuthController` saves conflict to session key `oauth_merge_pending` and redirects back to panel without creating identity Closes #286
…AccountsAction - New `MergeAccountsAction`: moves ExternalIdentities and tenant memberships from current user to old user, enriches old user if first_login_at is null, deletes current user — all in DB::transaction - ConnectionHub detects `oauth_merge_pending` session on mount - Shows confirmation modal with old user's username, created_at, and message count - confirmMerge: executes merge, re-logs as old user, redirects - cancelMerge: clears session, dismisses modal Closes #287
…edirect Livewire's request()->url() returns the internal update endpoint, causing a MethodNotAllowedHttpException on redirect after merge.
…file updates - Redesigned login page with branded split layout - Added GitHub and Discord OAuth buttons - Profile page and lang file updates - Composer lock and vite config sync
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (7)
📝 WalkthroughWalkthroughThis PR converts tenant identifiers to string/UUID across models and casts, adds a users.first_login_at column and cast, refactors OAuth state/intent DTOs, implements callback orchestration (FindOrCreateUserByProvider, EnrichUserOnFirstLogin, DetectMergeConflict, AttachProviderToUser, MergeAccountsAction, HandleOAuthCallbackAction), adds GitHub OAuth client/connectors/DTOs, updates Discord/Twitch callback URL handling, propagates tenantId string types through ETL and DTOs, adds UI (custom login, connection-hub merge modal), and includes migrations and tests. Possibly related issues
Possibly related PRs
Suggested reviewers
|
- Timeline model: remove integer cast on tenant_id (was converting UUIDs to PHP_INT_MAX) - Moderation classifier tests: use Tenant::factory() instead of hardcoded integer IDs - FindExternalIdentity tests: use real tenant UUID instead of '1' - Discord OAuth test: assert APP_URL-based callback instead of configurable redirect_uri - AppPanelProvider: use domain tenancy only in production (fixes UrlGenerationException in tests)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app-modules/activity/src/Timeline/Timeline.php (1)
84-94:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove or update the integer cast for
tenant_id.The
casts()method at line 88 declares'tenant_id' => 'integer', but the PHPDoc at line 23 correctly reflects the new UUID-basedstringtype. This inconsistency will cause runtime errors when the database returns a UUID string but Eloquent attempts to cast it to an integer.🔧 Proposed fix
protected function casts(): array { return [ 'user_id' => 'string', - 'tenant_id' => 'integer', + 'tenant_id' => 'string', 'root_id' => 'string', 'parent_id' => 'string', 'is_ignored' => 'boolean', 'pinned' => 'boolean', ]; }🤖 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 `@app-modules/activity/src/Timeline/Timeline.php` around lines 84 - 94, The casts() method still maps 'tenant_id' => 'integer' causing Eloquent to cast UUID tenant IDs incorrectly; change the cast in the casts() array so 'tenant_id' uses a string cast (or remove the integer entry) to match the PHPDoc and UUID type. Locate the casts() method in the Timeline class and update the 'tenant_id' entry to 'string' (or delete the cast if implicit string behavior is preferred) and run tests to ensure no further type-related errors occur.app-modules/panel-app/resources/views/pages/profile.blade.php (1)
21-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConnections management is inaccessible on mobile layouts.
The new connections section is hidden below
xl, so mobile users can’t connect/disconnect providers or resolve merge prompts from this page.Proposed fix
-<div class="hidden space-y-6 xl:sticky xl:top-4 xl:block"> +<div class="space-y-6 xl:sticky xl:top-4">🤖 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 `@app-modules/panel-app/resources/views/pages/profile.blade.php` around lines 21 - 37, The connections panel is currently hidden on mobile due to the container div's responsive classes ("hidden ... xl:block"); update the wrapper around the profile-preview-card and the connections block (the div containing <livewire:connection-hub />) to be visible on smaller screens by removing the "hidden" and "xl:block" combination (preserve xl:sticky and xl:top-4 if you want desktop sticky behavior) so the connections UI (livewire:connection-hub) is rendered and usable on mobile. Ensure the class list still contains spacing like "space-y-6" and any desired xl-only positioning classes.
🧹 Nitpick comments (1)
app-modules/identity/src/Auth/Actions/AttachProviderToUser.php (1)
35-35: Fix theconnected_bynullability concern—DB won’t fail, but Login flows will store null
external_identities.connected_byis a nullable UUID ($table->uuid('connected_by')->nullable()), and it’s only constrained by an FK—so theauth()->id()returningnullduring unauthenticated OAuth Login won’t cause a DB constraint violation.In
OAuthController::getRedirect(), unauthenticated users are put intoOAuthIntent::Login, andOAuthController::getAuthenticate()callsAuth::login($result->user)afterHandleOAuthCallbackActionruns—soAttachProviderToUser::execute()(whereconnected_byis set) will seeauth()->id() === nullfor Login flows by design. Ifconnected_byis intended to record the user who initiated the connect, set it from the resolved$userinstead ofauth()->id().🤖 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 `@app-modules/identity/src/Auth/Actions/AttachProviderToUser.php` at line 35, The AttachProviderToUser action is using auth()->id() to populate the external_identities.connected_by column which is null during OAuth Login flows; change AttachProviderToUser::execute() to set connected_by from the resolved user instance (the $user or $result->user passed/returned by HandleOAuthCallbackAction / OAuthController::getAuthenticate) instead of auth()->id() so Login flows record the connected_by correctly; update the parameter usage in AttachProviderToUser::execute() and any callers to pass or use the resolved $user when creating the external identity.
🤖 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
`@app-modules/identity/database/migrations/2026_05_26_001934_add_first_login_at_to_users_table.php`:
- Around line 9-17: Add a down() rollback that removes the first_login_at column
so rollbacks restore schema parity: implement a public function down() in the
anonymous Migration class that uses Schema::table('users', function (Blueprint
$table) { drop the first_login_at column }); optionally guard the drop with a
column-exists check if your Schema helper supports it; reference the existing
up(), the users table and the first_login_at column when locating where to add
the method.
In `@app-modules/identity/src/Auth/Actions/AttachProviderToUser.php`:
- Around line 19-37: The current use of owner->providers()->updateOrCreate
includes connected_at and connected_by in the values array, which causes those
audit fields to be overwritten on updates; remove connected_at/connected_by from
the values passed to updateOrCreate and instead call updateOrCreate with only
updatable fields (type, credentials_type, credentials, metadata), then check the
returned model's wasRecentlyCreated property and, if true, set connected_at =
now() and connected_by = auth()->id() and save the model so those audit fields
are only set on initial create.
In `@app-modules/identity/src/Auth/Actions/EnrichUserOnFirstLogin.php`:
- Around line 22-24: The retry path in EnrichUserOnFirstLogin assumes only
username uniqueness can fail; if email causes a
UniqueConstraintViolationException we must remove or alter the conflicting field
before retrying. Update the exception handling around the database save/retry
logic (EnrichUserOnFirstLogin) so that when a UniqueConstraintViolationException
occurs you detect which field conflicts (check exception message or look up an
existing user by email/username) and then strip/modify that specific key from
the $updates array (e.g., unset($updates['email']) or
unset($updates['username'])) before attempting the retry; ensure the same logic
is applied in both places where $updates is built/used (the blocks that set
$updates['email'] and $updates['username'] around the shown lines).
In `@app-modules/identity/src/Auth/Actions/FindOrCreateUserByProvider.php`:
- Around line 29-33: The exists()+attach() pair in
FindOrCreateUserByProvider.php is racy; instead of checking then attaching, call
the atomic pivot attach helper to avoid duplicate pivot inserts—replace the
$user->tenants()->where(...)->exists() and $user->tenants()->attach($tenant)
logic with a single atomic operation such as
$user->tenants()->syncWithoutDetaching($tenant->getKey()) (or an equivalent
pivot upsert) so concurrent requests cannot create duplicate membership rows.
- Around line 65-80: The current one-shot fallback in FindOrCreateUserByProvider
can still race and throw UniqueConstraintViolationException again; change the
logic to perform a bounded retry loop (e.g., maxAttempts = 3-5) around the
create call: inside a retry loop call DB::transaction (or use transaction +
savepoint) and attempt User::query()->create(...) with a username generated by
generateSuffixedUsername($oauthUser->username); on
UniqueConstraintViolationException catch, increment attempt count, re-generate a
new suffix and retry until attempts exhausted, and only then rethrow the
exception; ensure you update both the initial create path and the fallback path
to use the same bounded retry behavior.
In `@app-modules/identity/src/Auth/Actions/MergeAccountsAction.php`:
- Around line 60-65: The retry fallback only unsets 'username' on
UniqueConstraintViolationException which fails if the violation is for 'email'
(or another unique key); update the catch to inspect the exception/underlying
SQL error to determine which unique constraint triggered the failure (or
conservatively remove all known unique fields from $updates), then retry
$target->update($updates) inside the transaction; target symbols:
DB::transaction, $target->update(), $updates, and
UniqueConstraintViolationException — map constraint names to fields (e.g. email,
username) and unset the offending keys before the retry so the merge succeeds
for email collisions as well.
In `@app-modules/identity/src/Auth/DTOs/MergeConflictDTO.php`:
- Around line 26-33: The DTO is serializing raw OAuth tokens via
$this->credentials->toDatabase(), which must not be persisted in session; change
MergeConflictDTO to exclude sensitive token fields and only include non-secret
metadata (e.g., provider, expiry, scopes, token_type) by either calling a new
safe serializer like credentials->toSessionSafe() or by constructing the
'credentials' array manually without access_token/refresh_token, and persist
full tokens in a secure token store (or save a reference/id) instead; update the
credentials class to add toSessionSafe() (omitting token values) and replace
uses of toDatabase() in MergeConflictDTO (and any consumers) to use the safe
serializer.
In `@app-modules/identity/src/Auth/Http/Controllers/OAuthController.php`:
- Around line 34-35: The code currently stores url()->previous() into the OAuth
state (returnUrl) and later redirects to it, which allows open redirects; change
this to only accept and persist internal relative paths or same-host URLs.
Validate/sanitize the value returned by url()->previous() before putting it into
the OAuth state in OAuthController (the returnUrl assignment) and likewise
verify state->returnUrl before performing the redirects at the later redirect
points: allow only values that start with '/' (relative paths) or whose host
matches the app's configured host (e.g., compare parse_url(..., PHP_URL_HOST) to
config('app.url')/parse host), otherwise set returnUrl to null or a safe default
(e.g., '/') so external domains cannot be redirected to.
- Around line 46-49: The OAuth callback handler (OAuthController) currently
calls OAuthStateDTO::fromEncryptedString(request()->input('state')) and
$action->execute($state, $identityProvider, request()->input('code')) without
validating inputs, which can cause runtime failures; before calling
fromEncryptedString or execute, explicitly check that request()->input('state')
and request()->input('code') are present and non-empty, return a controlled HTTP
error (e.g., 400 Bad Request) if missing, and wrap the call to
OAuthStateDTO::fromEncryptedString in a try/catch to handle malformed or
tampered state (returning a 400/422 error) so $action->execute only runs with
validated $state and $code.
In `@app-modules/integration-github/src/IntegrationGithubServiceProvider.php`:
- Around line 15-18: IntegrationGithubServiceProvider is using untyped
config('services.github.client_id')/config('services.github.client_secret') when
binding GitHubOAuthConnector; change these to the typed
config()->string('services.github.client_id') and
config()->string('services.github.client_secret') so the container gets
non-nullable strings consistent with other providers and to avoid runtime type
issues when env values are missing.
In `@app-modules/integration-github/src/OAuth/GitHubOAuthClient.php`:
- Around line 55-57: The callbackUrl() method in GitHubOAuthClient.php uses
mb_rtrim which requires the ext-mbstring extension; update the implementation to
avoid that dependency by replacing mb_rtrim(config('app.url'), '/') with PHP's
built-in rtrim (i.e., use rtrim(...) ) in the callbackUrl() method of the
GitHubOAuthClient class (and similarly remove/replace other uses of mb_rtrim
across modules) OR alternatively add ext-mbstring to composer.json require if
you intentionally depend on mbstring; choose one approach and apply
consistently.
In `@app-modules/panel-app/resources/views/auth/login.blade.php`:
- Around line 85-93: The OAuth links are passing request()->getHost() as the
'tenant' param which is wrong for slug-based panel routing; update both href
lines that call route('oauth.redirect', ['tenant' => request()->getHost(), ...])
to pass the route tenant context instead (e.g. use request()->route('tenant') or
the existing tenant route variable) so the 'tenant' parameter uses the
slug-based tenant from the current route rather than the host.
In `@app/Livewire/ConnectionHub.php`:
- Around line 58-64: Tenant::query()->find($this->tenantId) can return null, so
before building the route in redirect(route('oauth.redirect', ...)) guard
against a missing tenant: check the result of
Tenant::query()->find($this->tenantId) (referenced as $tenant) and handle the
null case (for example abort(404), emit an error, or return early) instead of
dereferencing $tenant->domain or $tenant->slug; only build the route using
$tenant->domain ?? $tenant->slug when $tenant is not null.
In
`@database/migrations/2026_05_25_225721_convert_tenant_id_from_bigint_to_uuid.php`:
- Around line 78-81: The down() method is a no-op and should explicitly fail for
this irreversible migration; modify the down() method in the migration class to
throw a runtime exception (e.g., throw new \RuntimeException('Irreversible
migration: tenant ID conversion cannot be reverted')) so rollbacks fail fast and
are clearly signaled.
- Around line 63-67: The migration's up() drops tenant_id foreign keys via
dropForeignKeyIfExists() and swaps columns (DROP/RENAME) for each table in
$childTables but never re-creates the foreign key/index to tenants(id); update
the up() loop after the RENAME to add back the referential integrity for each
table in $childTables by creating an index (if needed) and a foreign key
constraint on tenant_id referencing tenants(id) (use the same naming convention
as dropForeignKeyIfExists()/existing constraints), and ensure the down() mirrors
this by dropping that new FK before reverting columns so foreign keys are
consistently recreated and removed when migrating up/down.
---
Outside diff comments:
In `@app-modules/activity/src/Timeline/Timeline.php`:
- Around line 84-94: The casts() method still maps 'tenant_id' => 'integer'
causing Eloquent to cast UUID tenant IDs incorrectly; change the cast in the
casts() array so 'tenant_id' uses a string cast (or remove the integer entry) to
match the PHPDoc and UUID type. Locate the casts() method in the Timeline class
and update the 'tenant_id' entry to 'string' (or delete the cast if implicit
string behavior is preferred) and run tests to ensure no further type-related
errors occur.
In `@app-modules/panel-app/resources/views/pages/profile.blade.php`:
- Around line 21-37: The connections panel is currently hidden on mobile due to
the container div's responsive classes ("hidden ... xl:block"); update the
wrapper around the profile-preview-card and the connections block (the div
containing <livewire:connection-hub />) to be visible on smaller screens by
removing the "hidden" and "xl:block" combination (preserve xl:sticky and
xl:top-4 if you want desktop sticky behavior) so the connections UI
(livewire:connection-hub) is rendered and usable on mobile. Ensure the class
list still contains spacing like "space-y-6" and any desired xl-only positioning
classes.
---
Nitpick comments:
In `@app-modules/identity/src/Auth/Actions/AttachProviderToUser.php`:
- Line 35: The AttachProviderToUser action is using auth()->id() to populate the
external_identities.connected_by column which is null during OAuth Login flows;
change AttachProviderToUser::execute() to set connected_by from the resolved
user instance (the $user or $result->user passed/returned by
HandleOAuthCallbackAction / OAuthController::getAuthenticate) instead of
auth()->id() so Login flows record the connected_by correctly; update the
parameter usage in AttachProviderToUser::execute() and any callers to pass or
use the resolved $user when creating the external identity.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 784181be-4459-419d-ad80-795991bb06c7
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (100)
app-modules/activity/src/Message/DTOs/NewMessageDTO.phpapp-modules/activity/src/Message/Models/MembershipEvent.phpapp-modules/activity/src/Message/Models/Message.phpapp-modules/activity/src/Message/Models/MessageAttachment.phpapp-modules/activity/src/Message/Models/MessageEmbed.phpapp-modules/activity/src/Message/Models/MessageMention.phpapp-modules/activity/src/Message/Models/MessageThread.phpapp-modules/activity/src/Moderation/Models/ModerationEvent.phpapp-modules/activity/src/Reaction/Models/Reaction.phpapp-modules/activity/src/Timeline/DTOs/CreatePostDTO.phpapp-modules/activity/src/Timeline/DTOs/CreateReplyDTO.phpapp-modules/activity/src/Timeline/Listeners/PublishModerationToTimeline.phpapp-modules/activity/src/Timeline/Queries/TimelineFeed.phpapp-modules/activity/src/Timeline/Timeline.phpapp-modules/activity/src/Tracking/DTOs/TrackActivityDTO.phpapp-modules/activity/src/Tracking/Models/Interaction.phpapp-modules/activity/src/Voice/Models/Voice.phpapp-modules/community/src/Feedback/Models/Feedback.phpapp-modules/community/src/Feedback/Models/Review.phpapp-modules/community/src/Meeting/Models/Meeting.phpapp-modules/gamification/src/Badge/DTOs/NewBadgeDTO.phpapp-modules/gamification/src/Badge/Models/Badge.phpapp-modules/gamification/src/Character/Actions/CharacterInitializerAction.phpapp-modules/gamification/src/Character/Models/PastSeason.phpapp-modules/gamification/src/Season/Models/Season.phpapp-modules/identity/database/migrations/2026_05_26_001934_add_first_login_at_to_users_table.phpapp-modules/identity/docs/adr/0001-oauth-user-resolution-and-merge-strategy.mdapp-modules/identity/routes/authentication-routes.phpapp-modules/identity/src/Auth/Actions/AttachProviderToUser.phpapp-modules/identity/src/Auth/Actions/AuthenticateAction.phpapp-modules/identity/src/Auth/Actions/DetectMergeConflict.phpapp-modules/identity/src/Auth/Actions/EnrichUserOnFirstLogin.phpapp-modules/identity/src/Auth/Actions/FindOrCreateUserByProvider.phpapp-modules/identity/src/Auth/Actions/HandleOAuthCallbackAction.phpapp-modules/identity/src/Auth/Actions/MergeAccountsAction.phpapp-modules/identity/src/Auth/DTOs/MergeConflictDTO.phpapp-modules/identity/src/Auth/DTOs/OAuthResultDTO.phpapp-modules/identity/src/Auth/DTOs/OAuthStateDTO.phpapp-modules/identity/src/Auth/Enums/OAuthIntent.phpapp-modules/identity/src/Auth/Exceptions/OAuthFlowException.phpapp-modules/identity/src/Auth/Http/Controllers/OAuthController.phpapp-modules/identity/src/ExternalIdentity/Actions/CreateAccountByExternalIdentity.phpapp-modules/identity/src/ExternalIdentity/DTOs/NewProviderDTO.phpapp-modules/identity/src/ExternalIdentity/DTOs/ResolveUserProviderDTO.phpapp-modules/identity/src/ExternalIdentity/Enums/IdentityProvider.phpapp-modules/identity/src/ExternalIdentity/Models/ExternalIdentity.phpapp-modules/identity/src/Tenant/Models/Tenant.phpapp-modules/identity/src/Tenant/Models/TenantUser.phpapp-modules/identity/src/User/Models/User.phpapp-modules/identity/tests/Feature/Auth/DetectMergeConflictTest.phpapp-modules/identity/tests/Feature/Auth/EnrichUserOnFirstLoginTest.phpapp-modules/identity/tests/Feature/Auth/FindOrCreateUserByProviderTest.phpapp-modules/identity/tests/Feature/Auth/MergeAccountsActionTest.phpapp-modules/integration-devto/src/Polling/SyncDevToArticles.phpapp-modules/integration-discord/src/ETL/Actions/ImportDiscordMessageAction.phpapp-modules/integration-discord/src/ETL/Actions/ImportDiscordModerationEventAction.phpapp-modules/integration-discord/src/ETL/Actions/ImportDiscordProfileAction.phpapp-modules/integration-discord/src/ETL/Actions/ImportDiscordReactionsAction.phpapp-modules/integration-discord/src/ETL/Actions/ImportDiscordVoiceLogAction.phpapp-modules/integration-discord/src/ETL/Console/ImportDiscordMessagesCommand.phpapp-modules/integration-discord/src/OAuth/DiscordOAuthClient.phpapp-modules/integration-github/composer.jsonapp-modules/integration-github/database/factories/.gitkeepapp-modules/integration-github/database/migrations/.gitkeepapp-modules/integration-github/database/seeders/.gitkeepapp-modules/integration-github/phpstan.ignore.neonapp-modules/integration-github/phpstan.neonapp-modules/integration-github/src/IntegrationGithubServiceProvider.phpapp-modules/integration-github/src/OAuth/DTO/GitHubOAuthAccessDTO.phpapp-modules/integration-github/src/OAuth/DTO/GitHubOAuthUserDTO.phpapp-modules/integration-github/src/OAuth/GitHubOAuthClient.phpapp-modules/integration-github/src/Transport/GitHubApiConnector.phpapp-modules/integration-github/src/Transport/GitHubOAuthConnector.phpapp-modules/integration-github/src/Transport/Requests/OAuth/ExchangeCodeForToken.phpapp-modules/integration-github/src/Transport/Requests/Users/GetCurrentUser.phpapp-modules/integration-github/tests/Feature/.gitkeepapp-modules/integration-github/tests/Unit/.gitkeepapp-modules/integration-twitch/src/IntegrationTwitchServiceProvider.phpapp-modules/integration-twitch/src/Models/TwitchSubscription.phpapp-modules/integration-twitch/src/OAuth/TwitchOAuthClient.phpapp-modules/integration-twitch/src/Transport/TwitchOAuthConnector.phpapp-modules/panel-app/lang/en/profile.phpapp-modules/panel-app/lang/pt_BR/profile.phpapp-modules/panel-app/resources/views/auth/login.blade.phpapp-modules/panel-app/resources/views/pages/profile.blade.phpapp-modules/panel-app/src/Livewire/Timeline/Feed.phpapp-modules/panel-app/src/Pages/LoginPage.phpapp-modules/profile/src/Models/Profile.phpapp/Livewire/ConnectionHub.phpapp/Providers/Filament/AppPanelProvider.phpcomposer.jsonconfig/services.phpdatabase/migrations/2026_05_25_225721_convert_tenant_id_from_bigint_to_uuid.phpdatabase/seeders/BaseSeeder.phpphpunit.xmlresources/css/filament/app/theme.cssresources/css/filament/user/theme.cssresources/views/livewire/connection-hub-admin.blade.phpresources/views/livewire/connection-hub.blade.phpvite.config.js
💤 Files with no reviewable changes (4)
- app-modules/identity/src/Auth/Actions/AuthenticateAction.php
- app-modules/integration-twitch/src/Transport/TwitchOAuthConnector.php
- resources/css/filament/user/theme.css
- phpunit.xml
- AuditLogTest: remove (int) cast on tenant_id comparison (UUID string) - phpunit.xml: set CACHE_STORE=array for cache tagging support in tests - Update PHPDoc @Property tenant_id from int to string in 7 models: ModerationAuditLog, ModerationRule, ModerationAppeal, ModerationAction, Character, DiscordGuild, TwitchEventLog
gvieira18
approved these changes
May 26, 2026
YuriSouzaDev
approved these changes
May 26, 2026
Clintonrocha98
approved these changes
May 26, 2026
gvieira18
pushed a commit
that referenced
this pull request
May 26, 2026
…rovements (#290) ## Summary - **OAuth error handling**: Gracefully handle token exchange failures and missing credentials instead of crashing with unhandled exceptions - **ConnectionHub UX**: Compact layout for profile sidebar, light/dark theme support, refined disconnect button styling - **Portal improvements**: Add "Área do Usuário" navbar link, Twitch as login provider, fix hero avatar metadata key (`name` vs `username` for legacy/new GitHub records) - **Provider reconnection**: Clear `disconnected_at` when reconnecting an existing provider ## Changes ### Identity module - `OAuthController`: catch `OAuthFlowException` on token exchange, redirect with error notification - `AttachProviderToUser`: clear `disconnected_at` on reconnect - OAuth clients (Discord, GitHub, Twitch): throw `OAuthFlowException` on missing credentials ### Panel App - `ConnectionHub`: compact card layout, dark/light theme support, pill-style disconnect button, timestamp next to title - `LoginPage`: add Twitch OAuth button - Profile wrapper: light mode compatible (`bg-white dark:bg-gray-900`) ### Portal - Navbar: outlined "Área do Usuário" button linking to `/app` - `HeroSection`: support both `username` (new) and `name` (legacy) metadata keys for GitHub avatars ## Test plan - [ ] OAuth login with Discord, GitHub, and Twitch - [ ] Disconnect/reconnect a provider from profile ConnectionHub - [ ] Verify ConnectionHub renders correctly in both light and dark mode - [ ] Portal landing page loads without errors (hero avatars) - [ ] "Área do Usuário" navbar link navigates to `/app` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Description This PR enhances OAuth security and user experience across the platform. It adds comprehensive error handling for OAuth token exchanges, validates provider credentials at initialization, and resets disconnected provider states on reconnection. The ConnectionHub UI is updated with improved light/dark theme support and refined styling, while the portal gains Twitch OAuth support and fixes GitHub avatar metadata handling. The identity module prevents unhandled exceptions by catching OAuth failures with explicit error notifications. ## References - PR `#288`: [OAuth flow refactor with account merge strategy](#288) — Related OAuth improvements - PR `#272`: [Twitch integration with admin panel, tenant-scoped EventSub, and subscription management](#272) — Twitch integration foundation - PR `#289`: [Data migration indexes](#289) — Related database changes ## Dependencies & Requirements No new dependencies were added or updated in this PR. All changes utilize existing framework and module capabilities. ## Contributor Summary | Contributor | Lines Added | Lines Removed | Files Changed | |---|---|---|---| | danielhe4rt | 136 | 45 | 12 | ## Changes Summary | File Path | Change Description | |---|---| | app-modules/identity/src/Auth/Exceptions/OAuthFlowException.php | Added `tokenExchangeFailed()` factory method for formatted OAuth token exchange error messages | | app-modules/identity/src/Auth/Http/Controllers/OAuthController.php | Enhanced error handling for missing OAuth configurations, denied callbacks, and token exchange failures with try/catch blocks and redirects | | app-modules/identity/src/Auth/Actions/AttachProviderToUser.php | Reset `disconnected_at` to null on reconnection to mark providers as active again | | app-modules/integration-discord/src/OAuth/DiscordOAuthClient.php | Added validation for `access_token` presence in Discord token response; throws `OAuthFlowException` on missing token | | app-modules/integration-github/src/IntegrationGithubServiceProvider.php | Added validation for GitHub client credentials with explicit RuntimeException on missing configuration | | app-modules/integration-github/src/OAuth/GitHubOAuthClient.php | Added validation for `access_token` in GitHub token response; throws `OAuthFlowException` on missing token | | app-modules/integration-twitch/src/OAuth/TwitchOAuthClient.php | Added validation for `access_token` in Twitch token response; throws `OAuthFlowException` on missing token | | app-modules/panel-app/resources/views/auth/login.blade.php | Added Twitch OAuth login button alongside Discord and GitHub options | | app-modules/panel-app/resources/views/pages/profile.blade.php | Updated profile card styling with rounded backgrounds, rings, and dark mode variants | | app-modules/portal/resources/views/components/navbar.blade.php | Added responsive "Área do Usuário" link and restructured Discord button for mobile/desktop variants | | app-modules/portal/src/Livewire/HeroSection.php | Updated avatar metadata mapping to support both legacy `name` and new `username` keys for GitHub records | | resources/views/livewire/connection-hub.blade.php | Refined ConnectionHub styling for light/dark theme compatibility, updated disconnect button and permissions section appearance | <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/he4rt/heartdevs.com/pull/290?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/he4rt/heartdevs.com/pull/290?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
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
6 participants
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.
Summary
tenant_idfrom bigint to UUID across 31 tablesfirst_login_atfield enriches ETL-created users on their first real loginKey Changes
OAuth Architecture
HandleOAuthCallbackActionorchestrates the full callback flowFindOrCreateUserByProviderresolves users cross-tenant with sequential username suffix on collisionEnrichUserOnFirstLoginupdates email/name on first real loginDetectMergeConflictidentifies when a Link would create duplicatesMergeAccountsActionmerges lightweight new user into heavy old user (DB::transaction)Account Merge (ADR-0001)
GitHub Integration
app-modules/integration-github/Login Page
Test plan
Related Issues
Closes #284, #285, #286, #287
Description
This PR refactors the OAuth flow to support tenant-based authentication with intent-based routing (Login vs Link), converts tenant_id from bigint to UUID across the application, and introduces GitHub OAuth integration. A key feature is the account merge strategy that detects and resolves conflicts when linking providers that belong to different users, preventing duplicate accounts from ETL vs web OAuth. The system now enriches ETL-created users on first real login and includes a redesigned login page with OAuth provider buttons.
References
#284: Handle username collision with sequential suffix on OAuth user creation#285: OAuth flow refactor with account merge strategy#286: GitHub OAuth integration#287: Login page redesignapp-modules/identity/docs/adr/0001-oauth-user-resolution-and-merge-strategy.md)Dependencies & Requirements
New Dependencies:
he4rt/integration-github(>=1)Environment Variables Added:
GITHUB_OAUTH_CLIENT_IDGITHUB_OAUTH_CLIENT_SECRETGITHUB_OAUTH_SCOPES(default:'read:user user:email')GITHUB_OAUTH_ENABLED(default:true)Database Changes:
2026_05_25_225721_convert_tenant_id_from_bigint_to_uuid.php: Convertstenant_idfrom bigint to UUID across 31 tables2026_05_26_001934_add_first_login_at_to_users_table.php: Adds nullablefirst_login_attimestamp column to users tableContributor Summary
Changes Summary
database/migrations/2026_05_25_225721_convert_tenant_id_from_bigint_to_uuid.phpapp-modules/identity/database/migrations/2026_05_26_001934_add_first_login_at_to_users_table.phpapp-modules/identity/src/Auth/Actions/HandleOAuthCallbackAction.phpapp-modules/identity/src/Auth/Actions/FindOrCreateUserByProvider.phpapp-modules/identity/src/Auth/Actions/EnrichUserOnFirstLogin.phpapp-modules/identity/src/Auth/Actions/DetectMergeConflict.phpapp-modules/identity/src/Auth/Actions/MergeAccountsAction.phpapp-modules/identity/src/Auth/Actions/AttachProviderToUser.phpapp-modules/identity/src/Auth/DTOs/OAuthStateDTO.phpapp-modules/identity/src/Auth/DTOs/OAuthResultDTO.phpapp-modules/identity/src/Auth/DTOs/MergeConflictDTO.phpapp-modules/identity/src/Auth/Enums/OAuthIntent.phpapp-modules/integration-github/src/OAuth/GitHubOAuthClient.phpapp-modules/integration-github/src/OAuth/DTO/GitHubOAuthAccessDTO.phpapp-modules/integration-github/src/OAuth/DTO/GitHubOAuthUserDTO.phpapp-modules/integration-github/src/Transport/GitHubOAuthConnector.phpapp-modules/integration-github/src/Transport/GitHubApiConnector.phpapp-modules/integration-github/src/Transport/Requests/OAuth/ExchangeCodeForToken.phpapp-modules/integration-github/src/Transport/Requests/Users/GetCurrentUser.phpapp-modules/integration-github/src/IntegrationGithubServiceProvider.phpinttostring, added email/first_login_at properties to Userapp-modules/identity/src/Tenant/Models/Tenant.phpapp-modules/identity/src/Auth/Http/Controllers/OAuthController.phpapp-modules/identity/routes/authentication-routes.phpapp-modules/panel-app/resources/views/auth/login.blade.phpapp-modules/panel-app/src/Pages/LoginPage.phpresources/views/livewire/connection-hub.blade.phpresources/views/livewire/connection-hub-admin.blade.phpapp/Livewire/ConnectionHub.phpapp-modules/identity/tests/Feature/Auth/DetectMergeConflictTest.phpapp-modules/identity/tests/Feature/Auth/FindOrCreateUserByProviderTest.phpapp-modules/identity/tests/Feature/Auth/EnrichUserOnFirstLoginTest.phpapp-modules/identity/tests/Feature/Auth/MergeAccountsActionTest.phpconfig/services.phpcomposer.jsonvite.config.js