fix: resolve session overlap when following email profile links#140
fix: resolve session overlap when following email profile links#140romanetar wants to merge 1 commit into
Conversation
Signed-off-by: romanetar <roman_ag@hotmail.com>
📝 WalkthroughWalkthroughThis PR introduces a signed profile link feature that allows users to access their profile through cryptographically verified URLs sent in emails. A new ChangesSigned Profile Link Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-140/ This page is automatically updated on each push to this PR. |
![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 (1)
app/Http/Controllers/UserController.php (1)
250-266: ⚡ Quick winConsider adding method documentation.
The implementation correctly handles signature validation, session mismatch logout, and redirect logic as confirmed by the test suite. Adding a PHPDoc block would help future maintainers understand the intended behavior for each scenario (guest, owner, different user).
📝 Suggested documentation
+ /** + * Handle signed profile link requests with session-overlap protection. + * + * Validates the request signature and manages session transitions: + * - If a different user is logged in, logs them out + * - Redirects authenticated matching users to their profile + * - Redirects guests to login with email pre-filled + * + * `@param` LaravelRequest $request Must contain signed user_id parameter + * `@return` \Illuminate\Http\RedirectResponse + */ public function getProfileLink(LaravelRequest $request)🤖 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/Http/Controllers/UserController.php` around lines 250 - 266, Add a PHPDoc block above the getProfileLink(LaravelRequest $request) method describing its purpose, parameters, return value, and side effects: state that it validates the signed request (abort 403 on invalid), logs out the current session if the signed user_id differs from the current user, redirects authenticated owners to getProfile, and redirects guests to getLogin with an optional login_hint derived from the target user's email; include `@param` LaravelRequest $request, `@return` \Illuminate\Http\RedirectResponse (or appropriate return type), and note possible abort/side-effect of auth_service->logout() and that it uses auth_service->getUserById().
🤖 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/Mail/WelcomeNewUserEmail.php`:
- Line 93: The signed profile link in WelcomeNewUserEmail is generated without
an expiration at the $this->profile_link assignment using
URL::signedRoute('auth.profile-link', ['user_id' => $user->getId()]); add an
expiration argument (e.g., a Carbon timestamp like Carbon::now()->addDays(14))
as the third parameter to URL::signedRoute to limit validity (choose 7–30 days
per policy) and add the Carbon import if missing; ensure the route verification
logic still accepts signed URLs with an expiration.
In `@tests/ProfileLinkSessionTest.php`:
- Around line 37-38: The test assigns results of
EntityManager::getRepository(User::class)->findOneBy(...) directly to the typed
properties $this->user_a and $this->user_b which can be null; add explicit
assertions (e.g. assertNotNull or similar) immediately after each findOneBy call
to verify the fixture exists before assigning to the User-typed properties, and
keep the subsequent assignment only after the assertion so failures are clear
and actionable.
---
Nitpick comments:
In `@app/Http/Controllers/UserController.php`:
- Around line 250-266: Add a PHPDoc block above the
getProfileLink(LaravelRequest $request) method describing its purpose,
parameters, return value, and side effects: state that it validates the signed
request (abort 403 on invalid), logs out the current session if the signed
user_id differs from the current user, redirects authenticated owners to
getProfile, and redirects guests to getLogin with an optional login_hint derived
from the target user's email; include `@param` LaravelRequest $request, `@return`
\Illuminate\Http\RedirectResponse (or appropriate return type), and note
possible abort/side-effect of auth_service->logout() and that it uses
auth_service->getUserById().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 281b405b-a410-4d79-9bba-5aa6c7dd29b9
📒 Files selected for processing (11)
app/Http/Controllers/UserController.phpapp/Mail/UserEmailVerificationRequest.phpapp/Mail/UserEmailVerificationSuccess.phpapp/Mail/WelcomeNewUserEmail.phpresources/views/emails/auth/email_verification_request.blade.phpresources/views/emails/auth/email_verification_request_success.blade.phpresources/views/emails/welcome_new_user_email.blade.phpresources/views/emails/welcome_new_user_email_fn.blade.phproutes/web.phpstart_local_server.shtests/ProfileLinkSessionTest.php
| $this->user_email = $user->getEmail(); | ||
| $this->user_fullname = $user->getFullName(); | ||
| $this->bio_link = URL::action("UserController@getLogin"); | ||
| $this->profile_link = URL::signedRoute('auth.profile-link', ['user_id' => $user->getId()]); |
There was a problem hiding this comment.
Add expiration to the signed profile link for security.
The signed URL is generated without an expiration time, which means it remains valid indefinitely. If the email is forwarded, leaked, or the user's mailbox is compromised at any point in the future, this link could be used for unauthorized profile access.
Laravel's URL::signedRoute() accepts an optional third parameter for expiration. Consider adding a reasonable lifetime (e.g., 7-30 days) to limit the window of exposure.
🔒 Proposed fix to add expiration
- $this->profile_link = URL::signedRoute('auth.profile-link', ['user_id' => $user->getId()]);
+ $this->profile_link = URL::signedRoute('auth.profile-link', ['user_id' => $user->getId()], now()->addDays(30));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->profile_link = URL::signedRoute('auth.profile-link', ['user_id' => $user->getId()]); | |
| $this->profile_link = URL::signedRoute('auth.profile-link', ['user_id' => $user->getId()], now()->addDays(30)); |
🤖 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/Mail/WelcomeNewUserEmail.php` at line 93, The signed profile link in
WelcomeNewUserEmail is generated without an expiration at the
$this->profile_link assignment using URL::signedRoute('auth.profile-link',
['user_id' => $user->getId()]); add an expiration argument (e.g., a Carbon
timestamp like Carbon::now()->addDays(14)) as the third parameter to
URL::signedRoute to limit validity (choose 7–30 days per policy) and add the
Carbon import if missing; ensure the route verification logic still accepts
signed URLs with an expiration.
| $this->user_a = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); | ||
| $this->user_b = EntityManager::getRepository(User::class)->findOneBy(['identifier' => '2']); |
There was a problem hiding this comment.
Add explicit fixture existence assertions before typed-property assignment (Line 37, Line 38).
findOneBy(...) may return null; assigning that to User-typed properties can fail with a low-signal TypeError. Add explicit assertions so failures are actionable.
Suggested patch
- $this->user_a = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']);
- $this->user_b = EntityManager::getRepository(User::class)->findOneBy(['identifier' => '2']);
+ $userA = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']);
+ $userB = EntityManager::getRepository(User::class)->findOneBy(['identifier' => '2']);
+
+ $this->assertNotNull($userA, 'Expected fixture user "sebastian.marcet" to exist');
+ $this->assertNotNull($userB, 'Expected fixture user with identifier "2" to exist');
+
+ $this->user_a = $userA;
+ $this->user_b = $userB;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->user_a = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); | |
| $this->user_b = EntityManager::getRepository(User::class)->findOneBy(['identifier' => '2']); | |
| $userA = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); | |
| $userB = EntityManager::getRepository(User::class)->findOneBy(['identifier' => '2']); | |
| $this->assertNotNull($userA, 'Expected fixture user "sebastian.marcet" to exist'); | |
| $this->assertNotNull($userB, 'Expected fixture user with identifier "2" to exist'); | |
| $this->user_a = $userA; | |
| $this->user_b = $userB; |
🤖 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 `@tests/ProfileLinkSessionTest.php` around lines 37 - 38, The test assigns
results of EntityManager::getRepository(User::class)->findOneBy(...) directly to
the typed properties $this->user_a and $this->user_b which can be null; add
explicit assertions (e.g. assertNotNull or similar) immediately after each
findOneBy call to verify the fixture exists before assigning to the User-typed
properties, and keep the subsequent assignment only after the assertion so
failures are clear and actionable.
1 participant
ref https://app.clickup.com/t/86b9tx32u
Summary by CodeRabbit
New Features
Bug Fixes
Tests