[Core] Add voter to check for sylius impersonation#16650
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
3 times, most recently
from
284e3e0 to
6fbdd8a
Compare
lusimeon
changed the title
WalkthroughThis pull request introduces user impersonation functionality. A new event subscriber cleans up impersonation-related session data upon logout, and the session management in the impersonation service is updated to store a unique parameter. In addition, a new security voter is implemented to determine if a user is impersonated, with corresponding tests added to validate the behavior. The changes also include new service definitions to register these components. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Kernel as Kernel
participant Subscriber as UserImpersonatorSubscriber
participant FM as FirewallMap
participant Session as Session
User->>Kernel: Triggers Logout
Kernel->>Subscriber: Dispatch LogoutEvent
Subscriber->>Subscriber: Retrieve Request from event
Subscriber->>FM: Get firewall configuration
FM-->>Subscriber: Return firewall config / null
Subscriber->>Session: Remove impersonation session variable
Session-->>Subscriber: Acknowledge removal
sequenceDiagram
participant Security as Security System
participant Voter as ImpersonationVoter
participant RequestStack as RequestStack
participant FM as FirewallMap
participant Session as Session
Security->>Voter: vote(token, subject, attributes)
Voter->>RequestStack: Get current request
RequestStack-->>Voter: Return request
Voter->>FM: Get firewall config for request
FM-->>Voter: Return firewall name or null
Voter->>Session: Check impersonation session variable
Session-->>Voter: Return impersonation status or error
Voter-->>Security: Return ACCESS_GRANTED/ACCESS_DENIED/ACCESS_ABSTAIN
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php (1)
34-46: Check session existence before removal.Symfony usually ensures a session is available, but in rare or custom setups the session might not be started or could be null. A brief defensive check would prevent possible edge-case exceptions:
public function unimpersonate(LogoutEvent $event): void { $request = $event->getRequest(); $config = $this->firewallMap->getFirewallConfig($request); if (!$config) { return; } + if (!$request->hasSession()) { + return; + } $request->getSession()->remove( sprintf('_security_impersonate_sylius_%s', $config->getName()), ); }src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php (1)
32-46: Add spec for missing firewall config scenario.Currently, you only test the normal path where
getFirewallConfig()returns a config. Consider verifying the behavior whennullis returned, ensuring the method exits gracefully. This covers an edge case and improves test robustness.
Would you like me to generate a complementary spec snippet that tests the scenario whengetFirewallConfigreturnsnull?src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (1)
69-82: Slight caution with session-based checks.In
isImpersonated, theSessionProvider::getSession(...)call is convenient. However, edge cases might arise if the session is not started or if$firewallresolution is null. If that’s acceptable, no changes needed; otherwise, consider a safety check to avoid potential null returns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
- GitHub Check: Static checks / PHP 8.2, Symfony ^6.4
🔇 Additional comments (11)
src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml (1)
134-138: LGTM: New event subscriber service for impersonation cleanup is well-structuredThe new
sylius.listener.user_impersonatorservice is properly configured as a kernel event subscriber with the necessarysecurity.firewall.mapdependency. This implementation aligns perfectly with the PR objectives to detect user impersonation.src/Sylius/Bundle/CoreBundle/Resources/config/services.xml (1)
284-289: LGTM: Security voter decorator properly integratedThe
AuthenticatedVoterservice is correctly configured as a decorator for Symfony'ssecurity.access.authenticated_voter, with appropriate dependencies injected. This implementation allows the system to check if a user is being impersonated through the standard security authorization system.src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php (3)
29-29: LGTM: Added property for tracking impersonation stateThe new private property
$sessionImpersonatorParameteris well-named and properly typed.
39-39: LGTM: Consistent session parameter naming conventionThe initialization of
$sessionImpersonatorParameterfollows the same pattern as the existing$sessionTokenParameterwith a clear naming convention that indicates its purpose.
53-53: LGTM: Session now tracks impersonation stateSetting the impersonation flag in the session enables the voter to later determine if a user is being impersonated, which is the core functionality needed for this feature.
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (1)
1-60: LGTM: Comprehensive test coverage for impersonation voterThe spec file provides excellent test coverage for the new
AuthenticatedVoterfunctionality:
- It verifies that the voter supports the custom
IS_IMPERSONATOR_SYLIUSattribute- It tests the positive case (user is impersonated)
- It tests the negative case (user is not impersonated)
The tests are well-structured and cover the essential functionality of the voter.
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php (2)
22-25: Constructor usage looks good.This constructor neatly injects the
FirewallMapwithout cluttering or additional logic.
27-32: Event subscription is straightforward and follows best practices.Subscribing to the
LogoutEventwithunimpersonateis clear and aligns with Symfony’s canonical event-subscriber model.src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php (1)
27-30: Constructor (let method) is concise.The
letmethod cleanly instantiates the subscriber with the mockedFirewallMap. This helps ensure each spec runs in isolation.src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (2)
25-31: Constructor usage looks fine.Injecting the original voter, the
RequestStack, andFirewallMapkeeps this class small and aligned with your design, without duplicating logic.
34-55: Clarify how the subject relates to the firewall.The code calls
$this->isImpersonated($subject)to check impersonation, but$subjectmay be a domain object, not a string (firewall name). Consider ensuring$subjectis indeed the firewall name or refactor to retrieve the firewall name from the active request to avoid confusion:if (self::IS_IMPERSONATOR_SYLIUS === $attribute && $this->isImpersonated($subject)) { return VoterInterface::ACCESS_GRANTED; }For instance:
$this->isImpersonated(); // rely solely on the main request or a passed firewall name
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (1)
68-82: Handle missing session more gracefully.
$this->requestStack->getSession()may be null if no session is available. Consider a safeguard to avoid potential\LogicException. For instance:-if (!$firewall) { +if (!$firewall || !$this->requestStack->getSession()) { return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.3, Symfony ^7.1, MariaDB 11.4.3, State Machine Adapter symfony_workflow
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.2, Symfony ^6.4, MariaDB 10.11.9, State Machine Adapter symfony_workflow
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.3, Symfony ^7.1, PostgreSQL 16.4
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.2, Symfony ^6.4, PostgreSQL 15.8
- GitHub Check: Packages / PHP 8.3, Symfony ^7.1
- GitHub Check: Packages / PHP 8.2, Symfony ^6.4
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
🔇 Additional comments (6)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (6)
22-25: Well-structured class purpose and naming.The class name
AuthenticatedVoterand the constantIS_IMPERSONATOR_SYLIUSare clearly named and aligned with the intended functionality for handling Sylius-specific impersonation checks.
26-31: Constructor injection is concise and maintainable.All dependencies are injected via the constructor, keeping the class loosely coupled and testable.
33-54: Potential misuse of$subjectwhen checking impersonation.When the attribute is
IS_IMPERSONATOR_SYLIUS, the method callsisImpersonated($subject). However,$subjectmight not be a string or even relevant to the firewall name. Consider ensuring$subjectis either null or a string representing the firewall name before passing it toisImpersonated.Do you want to validate
$subjecttype or rename that parameter to clarify it’s meant to be the firewall name if provided?
56-61: Consistent support for custom and inherited attributes.By deferring to
$authenticatedVoter->supportsAttribute($attribute), the code seamlessly handles attributes beyond impersonation, following Symfony best practices.
63-66: Proper delegation for subject type checks.Forwarding
supportsTypecalls to the underlying voter nicely adheres to the Liskov Substitution Principle (LSP) and ensures consistent behavior.
84-87: Clear key naming.Using
_security_impersonate_sylius_%sfor the session key is both distinctive and unlikely to collide, making it straightforward to identify impersonation status in session data.
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
from
30db055 to
858e558
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (1)
25-32: Consider adding a test for automatic firewall detection.The tests cover explicit firewall specification, but there's no test for when the firewall name is automatically detected from the request (when
nullis passed toisImpersonated).function it_votes_with_automatically_detected_firewall( TokenInterface $token, RequestStack $requestStack, SessionInterface $session, Request $request, FirewallConfig $firewallConfig, FirewallMap $firewallMap ): void { $requestStack->getMainRequest()->willReturn($request); $firewallMap->getFirewallConfig($request)->willReturn($firewallConfig); $firewallConfig->getName()->willReturn(self::FIREWALL_NAME); $requestStack->getSession()->willReturn($session); $session->get(sprintf('_security_impersonate_sylius_%s', self::FIREWALL_NAME), false)->willReturn(true); $this->vote($token, null, [AuthenticatedVoter::IS_IMPERSONATOR_SYLIUS])->shouldReturn(VoterInterface::ACCESS_GRANTED); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
- src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
- src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (3) (3)
AuthenticatedVoter(22-88)supportsAttribute(56-61)vote(33-54)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Packages / Get matrix
- GitHub Check: End-to-end tests (PostgreSQL) / Get matrix
- GitHub Check: End-to-end tests (MariaDB) / Get matrix
- GitHub Check: End-to-end tests (MySQL) / Get matrix
🔇 Additional comments (4)
src/Sylius/Bundle/CoreBundle/Resources/config/services.xml (1)
286-290: Great service integration!The service definition correctly decorates the standard Symfony
security.access.authenticated_voterservice, extending it with impersonation detection capability. The approach leverages Symfony's decorator pattern effectively to add functionality without modifying the core implementation.src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (3)
34-37: LGTM! Support for the custom attribute is properly tested.The test correctly verifies that the voter supports the new
IS_IMPERSONATOR_SYLIUSattribute.
39-48: LGTM! The positive impersonation case is well tested.This test properly verifies that when a user is being impersonated (session returns true for the impersonation key), the voter grants access as expected.
50-59: LGTM! The negative impersonation case is well tested.This test correctly verifies that when a user is not being impersonated (session returns false for the impersonation key), the voter denies access as expected.
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php
Outdated
Show resolved
Hide resolved
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
from
858e558 to
5445fe9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml (1)
139-142: New Service Declaration for User Impersonator SubscriberThe addition of the
sylius.listener.user_impersonatorservice is well-configured. It properly injects thesecurity.firewall.mapdependency and is registered as an event subscriber. Please ensure that the associatedUserImpersonatorSubscribercorrectly implementsEventSubscriberInterfaceand that its event handling (e.g., for logout events) aligns with your intended impersonation cleanup logic. Also, verify that the dependency injected is the correct service needed to determine the current firewall context.src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (1)
38-48: Consider breaking once the impersonator attribute is found for efficiency.Currently, the loop continues checking new attributes even after encountering
IS_IMPERSONATOR_SYLIUS. If only one impersonator attribute is expected per vote, you could break early for a minor performance improvement:foreach ($attributes as $attribute) { if (null === $attribute || (self::IS_IMPERSONATOR_SYLIUS !== $attribute)) { continue; } $result = VoterInterface::ACCESS_DENIED; if ($this->isImpersonated($subject)) { return VoterInterface::ACCESS_GRANTED; } - // continue checking further attributes + break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Sylius/Bundle/CoreBundle/Resources/config/services.xml
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (2)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php (1) (1)
__construct(22-25)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php (1) (1)
__construct(33-41)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.2, Symfony ^6.4, PostgreSQL 15.8
- GitHub Check: Packages / PHP 8.3, Symfony ^7.1
- GitHub Check: Packages / PHP 8.2, Symfony ^6.4
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
🔇 Additional comments (4)
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (3)
35-38: Test coverage for attribute support is comprehensive.This test neatly proves that
IS_IMPERSONATOR_SYLIUSis recognized as a supported attribute, preventing regressions in attribute handling.
40-49: Well-structured test for impersonation check.Verifies the session-based impersonation flag effectively grants the correct access. This ensures that the voter logic aligns with the feature's promises.
61-69: Commendable coverage of the no-session edge case.Testing the outcome when a session does not exist ensures graceful handling and prevents runtime errors in non-session contexts, such as API requests.
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (1)
69-87:Details
❓ Verification inconclusive
Verify the subject assumption when passing the firewall name.
The
isImpersonated(?string $firewall = null)method expects$subjectto be the firewall name if provided. If$subjectmight sometimes be a different type or if other code paths set$subjectto a non-string, it could lead to unexpected results insprintf.Run this script to confirm all usage sites pass valid string firewall names:
🏁 Script executed:
#!/bin/bash # Description: Ensure that AuthenticatedVoter is only called with string or null subject values. # Look for calls to AuthenticatedVoter::vote(...) across the codebase. rg -A 3 'AuthenticatedVoter->vote\s*\('Length of output: 41
Action Required: Manual Verification of Firewall Name Passing
The automated search for call sites of
AuthenticatedVoter::vote(...)did not produce any outputs, leaving us without conclusive evidence that the firewall name (subject) is consistently passed as a string (or null). This means there’s no automated confirmation that all usages enforce the correct type. To be safe, please manually verify that any calls to or uses of the firewall name in theAuthenticatedVoter(especially as used in methods likegetImpersonateKeyName) indeed pass a string or null, ensuring that no non-string value is inadvertently used in a context (such assprintf) that expects a string.
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
from
5445fe9 to
10df5e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (2)
17-17: Remove unused importThe
Psr\Http\Message\RequestInterfaceimport is not used anywhere in this specification class.-use Psr\Http\Message\RequestInterface;
63-63: Fix method name grammarThe method name has a grammatical error.
-function it_votes_abstain_when_session_do_not_exists( +function it_votes_abstain_when_session_does_not_exist(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php
- src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (3) (3)
AuthenticatedVoter(23-102)supportsAttribute(47-52)vote(34-45)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Packages / Get matrix
- GitHub Check: End-to-end tests (PostgreSQL) / Get matrix
🔇 Additional comments (6)
src/Sylius/Bundle/CoreBundle/Resources/config/services.xml (1)
286-290: Properly defined voter service with appropriate dependenciesThe AuthenticatedVoter service is correctly defined as a decorator for the Symfony authenticated voter, with all necessary dependencies injected. This implementation follows the decorator pattern best practices by passing the inner service as the first argument and adding the required request stack and firewall map.
src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php (5)
37-40: LGTM: Correctly tests attribute supportThis test properly verifies that the voter supports the
IS_IMPERSONATOR_SYLIUSattribute.
42-51: LGTM: Proper test for the impersonated user scenarioThis test correctly verifies that the voter grants access when the session contains impersonation data.
53-62: LGTM: Proper test for the non-impersonated user scenarioThis test correctly verifies that the voter denies access when the session does not contain impersonation data.
63-70: LGTM: Addresses the previous review comment about session absenceThis test properly handles the edge case when a session doesn't exist by verifying the voter abstains from voting.
72-82: LGTM: Proper test for handling null firewallThis test correctly verifies that the voter abstains when the firewall configuration is not available.
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
2 times, most recently
from
d781634 to
990bdd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (3)
23-25: Suggest adding a docblock for the class.Adding a short PHPDoc explaining the purpose of this voter would enhance clarity for future maintainers.
33-56: Handle silent catch for SessionNotFoundException.Breaking out of the loop when the session isn't found might obscure potential debugging. Consider logging or explicitly handling the case if needed for troubleshooting.
90-93: Optional: Extract the key prefix as a constant.For consistency, consider extracting
'_security_impersonate_sylius_'into a class-level constant, as it’s repeated in similar classes likeUserImpersonator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
- src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php
- src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (2)
src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php (1) (1)
__construct(33-41)src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php (1) (1)
__construct(22-25)
🔇 Additional comments (6)
src/Sylius/Bundle/CoreBundle/Resources/config/services.xml (1)
286-290: Nicely configured service decoration.The constructor arguments align well with your custom
AuthenticatedVoterclass. This setup should seamlessly replace the default Symfonysecurity.access.authenticated_voter.src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (5)
1-13: File header and strict typing.The file header and the
declare(strict_types=1);usage adhere to best practices.
25-31: Well-structured constructor.Declaring constructor properties as private is a neat approach and aligns with modern PHP standards.
58-66: Simple and focused support checks.The
supportsAttributeandsupportsTypemethods are concise and correct for a voter that targets one specific attribute.
68-77: Straightforward impersonation check.Fetching the session value under a specific key is clear. The method is well-isolated, improving testability.
79-88: Graceful null handling for firewall.Returning
nullif the main request or its firewall configuration is absent avoids potential errors.
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
from
990bdd8 to
2329dbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (3)
23-26: Add a short docblock explaining the purpose of this custom voter.
Even though the class name is self-explanatory, a brief docblock describing its high-level role (e.g., checking if a user is impersonated) is helpful for documentation purposes.
33-56: Consider logging the SessionNotFoundException.
Silently catching theSessionNotFoundException(lines 49-50) may hide real issues in production. Logging a warning would help troubleshoot misconfigurations.Below is a possible approach if you decide to add a logger:
use Psr\Log\LoggerInterface; class AuthenticatedVoter implements CacheableVoterInterface { private ?LoggerInterface $logger = null; public function __construct( private RequestStack $requestStack, private FirewallMap $firewallMap, + ?LoggerInterface $logger = null, ) { $this->logger = $logger; } public function vote(TokenInterface $token, mixed $subject, array $attributes): int { foreach ($attributes as $attribute) { if (self::IS_IMPERSONATOR_SYLIUS !== $attribute) { continue; } $firewall = $subject ?? $this->getMainRequestFirewallName(); if ($firewall) { try { return $this->isImpersonated($firewall) ? VoterInterface::ACCESS_GRANTED : VoterInterface::ACCESS_DENIED; } catch (SessionNotFoundException) { + if (null !== $this->logger) { + $this->logger->warning('Session not found while checking impersonation.'); + } } } break; } return VoterInterface::ACCESS_ABSTAIN; }
79-93: Firewall name resolution & key computation.
Using the main request’s firewall viafirewallMapis consistent with typical Symfony security voter patterns. Be mindful that in edge cases (e.g., sub-requests or multiple firewalls), you may need additional logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Sylius/Bundle/CoreBundle/Resources/config/services.xml
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
- src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
- src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (2)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php (1) (1)
__construct(22-25)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php (1) (1)
__construct(33-41)
🔇 Additional comments (3)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (3)
27-31: Constructor usage.
Injecting theRequestStackandFirewallMapis appropriate for retrieving firewall configurations and session data. No issues here.
58-66: All attributes & subject types.
ReturningtruefromsupportsType()ensures this voter can process any subject type. Given that impersonation checks don't depend on any specific object, this approach is valid.
68-77:Details
✅ Verification successful
Session key-based check.
This straightforward approach correctly retrieves the boolean flag from the session. Ensure it remains in sync with how keys are set (e.g., inUserImpersonator) so that naming never diverges.Would you like to verify that all code paths setting this
_security_impersonate_sylius_{firewall}key align with usage here?
🏁 Script executed:
#!/bin/bash rg '_security_impersonate_sylius_' -A 3Length of output: 2708
Session Key Consistency Verified
The impersonation flag inisImpersonated()correctly retrieves the session key formatted as_security_impersonate_sylius_{firewall}. Our verification confirms that all code paths—such as those inUserImpersonator,AuthenticatedVoterSpec.php, andUserImpersonatorSubscriber—use the same naming convention. Please ensure that any future changes preserve this consistency.
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
from
2329dbf to
99f4ffc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php (1)
40-42: Consider adding a test for when FirewallConfig is null.The current tests verify behavior when FirewallConfig is available, but it would be beneficial to also test the case where
getFirewallConfig()returns null, which could happen in a real application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Sylius/Bundle/CoreBundle/Resources/config/services.xml
- src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php
- src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php
🔇 Additional comments (2)
src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php (2)
1-62: Well-structured spec file with comprehensive test coverage.The spec file properly tests the subscriber's behavior for removing impersonation session variables during logout events. The test coverage includes both the happy path and error handling for when the session is not available.
44-44:Details
✅ Verification successful
Verify the session variable name format.
Make sure the session variable format
_security_impersonate_sylius_%smatches what's used in the actual implementation. This is critical for the impersonation feature to work correctly.
🏁 Script executed:
#!/bin/bash # Search for the session variable name pattern in the implementation rg "_security_impersonate_sylius_" --type php --glob "src/Sylius/Bundle/CoreBundle/**/*.php" --no-line-numberLength of output: 1142
Session variable naming verified and consistent
The session variable format
_security_impersonate_sylius_%sis used consistently across the codebase (as seen inUserImpersonator,AuthenticatedVoter, and related specs). No changes are required for this naming format.
src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php
Outdated
Show resolved
Hide resolved
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
from
99f4ffc to
d28539b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Sylius/Bundle/CoreBundle/Resources/config/services.xml (1)
286-291: Consider clarifying the voter name to avoid confusion with Symfony’s built-in AuthenticatedVoter.
While this voter is scoped under Sylius namespaces, the identical name “AuthenticatedVoter” can create ambiguity, especially in discussions or debugging. You might consider a unique naming convention (e.g.,SyliusImpersonationVoter) for clarity.src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php (1)
24-85: Well-designed test coverage for the UserImpersonatorSubscriberThis spec file provides excellent test coverage for the UserImpersonatorSubscriber, including all edge cases (session not found, missing firewall config) and successfully addresses the previous review comment about testing the
getSubscribedEventsmethod. The tests are well-structured and follow PhpSpec best practices.Consider adding a descriptive PHPDoc comment to the class to explain its purpose and relationship to the implementation class it's testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml(1 hunks)src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php(1 hunks)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php
- src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php
- src/Sylius/Bundle/CoreBundle/spec/Security/AuthenticatedVoterSpec.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (2)
src/Sylius/Bundle/CoreBundle/EventListener/UserImpersonatorSubscriber.php (1) (1)
__construct(23-26)src/Sylius/Bundle/CoreBundle/Security/UserImpersonator.php (1) (1)
__construct(33-41)
🔇 Additional comments (6)
src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml (1)
139-143: Looks good!
Registering the UserImpersonatorSubscriber as a kernel event subscriber with the firewall map argument is consistent with Symfony’s recommended approach. This enables proper cleanup or management of impersonation data during logout events.src/Sylius/Bundle/CoreBundle/Security/AuthenticatedVoter.php (4)
23-31: Excellent approach to using CacheableVoterInterface for performance.
ImplementingCacheableVoterInterfacecan improve performance by caching outcomes for given attributes and subjects. The constructor injection ofRequestStackandFirewallMapis clear and aligns with your impersonation logic.
33-56: Validate the default abstention logic when the session is unavailable.
In thevote()method, if a session is missing or throws aSessionNotFoundException, this voter abstains. This design is typically correct in Symfony to avoid incorrectly denying access. However, confirm that your application does not require a stricter denial under these conditions.Do you want to verify the intended behavior by testing a scenario where no session is available (e.g., a stateless firewall)? I can generate a quick test script if needed.
58-77: Straightforward support checks and impersonation retrieval.
YoursupportsAttribute()method ensures the voter only processes theIS_IMPERSONATOR_SYLIUSattribute, andisImpersonated()neatly reads the session flag. This is well-encapsulated and avoids confusion with other attributes.
79-93: Firewall-based session key retrieval is well-structured.
UsinggetMainRequestFirewallName()and constructing the_security_impersonate_sylius_%ssession key is clear and ensures impersonation state is properly sandboxed by firewall name. This approach should correctly handle multiple firewalls.src/Sylius/Bundle/CoreBundle/spec/EventListener/UserImpersonatorSubscriberSpec.php (1)
33-37: Good job addressing the previous review commentThis test for
getSubscribedEvents()directly addresses the previous review comment requesting verification of the event subscription implementation.
lusimeon
force-pushed
the
sylius-impersonator-voter
branch
2 times, most recently
from
30a944e to
1e49608
Compare
Decorate Symfony AuthenticatedVoter to be able to check if customer is impersonated
GSadee
force-pushed
the
sylius-impersonator-voter
branch
from
1e49608 to
6e28f97
Compare
GSadee
added
Feature
|
Thank you @lusimeon! 🎉 |
3 participants
Description
At this time, it's impossible to check if logged-in customer is impersonated or not. This feature can be useful to be able to allow or deny some actions to admin users when impersonate customer.
This PR add an Authorization voter to be able to check if customer is impersonated. The firewall can be pass as voter attribute subject, it defaults to request's firewall instead.
Usage
In controller:
In view:
{% if is_granted('IS_IMPERSONATOR_SYLIUS') %} 'You are currently impersonate %s'|format(app.user.username) {% endif %} {% if is_granted('IS_IMPERSONATOR_SYLIUS', 'my_firewall_name') %}…{% endif %}Summary by CodeRabbit
New Features
Tests