Add from capability #154
Add from capability #154
Conversation
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Context as EntityContext/OrchestrationContext
participant Event as Event
participant WithFrom as WithFrom
participant Dispatcher as EventDispatcher/TaskController
Context->>Event: Create event (e.g., Signal, Delay, Orchestration)
Context->>WithFrom: addFrom(event)
WithFrom-->>Context: Event wrapped with source StateId
Context->>Dispatcher: fire(wrapped event)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #154 +/- ##
============================================
+ Coverage 46.31% 46.85% +0.54%
- Complexity 1250 1313 +63
============================================
Files 99 103 +4
Lines 3565 3677 +112
============================================
+ Hits 1651 1723 +72
- Misses 1914 1954 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Performance Metrics
Performance Metrics
Performance Metrics
Performance Metrics
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/EntityContext.php(8 hunks)src/Events/Event.php(1 hunks)src/Events/HasInnerEventInterface.php(1 hunks)src/Events/RevokeUser.php(1 hunks)src/Events/ShareOwnership.php(2 hunks)src/Events/ShareWithRole.php(1 hunks)src/Events/ShareWithUser.php(1 hunks)src/Events/WithActivity.php(1 hunks)src/Events/WithDelay.php(1 hunks)src/Events/WithEntity.php(1 hunks)src/Events/WithFrom.php(1 hunks)src/Events/WithOrchestration.php(1 hunks)src/Events/WithPriority.php(2 hunks)src/OrchestrationContext.php(10 hunks)tests/Unit/EventDescriptionTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/Events/ShareWithRole.php (5)
src/Events/RevokeUser.php (1)
__toString(44-47)src/Events/ShareOwnership.php (1)
__toString(44-47)src/Events/ShareWithUser.php (1)
__toString(45-48)src/Events/GiveOwnership.php (1)
__toString(41-44)src/Events/RevokeRole.php (1)
__toString(44-47)
src/Events/RevokeUser.php (11)
src/Events/WithActivity.php (1)
__toString(53-56)src/Events/WithFrom.php (1)
__toString(19-22)src/Events/WithOrchestration.php (1)
__toString(55-58)src/Events/WithPriority.php (1)
__toString(56-59)tests/Unit/EventDescriptionTest.php (6)
__toString(55-58)__toString(69-72)__toString(83-86)__toString(103-106)__toString(125-128)__toString(140-143)src/Events/ShareOwnership.php (1)
__toString(44-47)src/Events/ShareWithRole.php (1)
__toString(45-48)src/Events/ShareWithUser.php (1)
__toString(45-48)src/State/Ids/StateId.php (1)
__toString(178-181)src/Events/RevokeRole.php (1)
__toString(44-47)src/State/OrchestrationInstance.php (1)
__toString(41-44)
src/Events/ShareWithUser.php (5)
src/Events/RevokeUser.php (1)
__toString(44-47)src/Events/ShareOwnership.php (1)
__toString(44-47)src/Events/ShareWithRole.php (1)
__toString(45-48)src/Events/GiveOwnership.php (1)
__toString(41-44)src/Events/RevokeRole.php (1)
__toString(44-47)
src/Events/WithEntity.php (3)
src/Events/WithActivity.php (1)
__construct(33-36)src/Events/Event.php (1)
__construct(41-53)src/Events/WithFrom.php (1)
__construct(9-12)
tests/Unit/EventDescriptionTest.php (8)
src/Events/WithActivity.php (2)
getInnerEvent(43-46)__toString(53-56)src/Events/WithEntity.php (2)
getInnerEvent(45-48)__toString(55-58)src/Events/WithFrom.php (2)
getInnerEvent(24-27)__toString(19-22)src/Events/WithOrchestration.php (2)
getInnerEvent(45-48)__toString(55-58)src/Events/WithPriority.php (1)
__toString(56-59)src/Events/WithDelay.php (2)
getInnerEvent(46-49)__toString(51-54)src/Events/HasInnerEventInterface.php (1)
getInnerEvent(31-31)src/Events/WithLock.php (2)
getInnerEvent(59-62)__toString(54-57)
src/Events/WithActivity.php (5)
src/Events/Event.php (1)
__construct(41-53)src/Events/WithEntity.php (1)
__construct(31-34)src/Events/WithOrchestration.php (1)
__construct(32-38)src/Events/AwaitResult.php (1)
__construct(35-38)cli/ids/id.go (1)
StateId(176-179)
src/Events/WithDelay.php (6)
src/Events/WithActivity.php (2)
__construct(33-36)forEvent(38-41)src/Events/Event.php (1)
__construct(41-53)src/Events/WithFrom.php (2)
__construct(9-12)forEvent(14-17)src/Events/WithOrchestration.php (1)
__construct(32-38)src/Events/WithPriority.php (1)
__construct(31-34)tests/Unit/EventDescriptionTest.php (1)
__construct(50-53)
src/Events/WithFrom.php (6)
src/Events/WithActivity.php (4)
__construct(33-36)forEvent(38-41)__toString(53-56)getInnerEvent(43-46)src/Events/Event.php (1)
__construct(41-53)src/Events/WithEntity.php (3)
__construct(31-34)__toString(55-58)getInnerEvent(45-48)src/Events/WithOrchestration.php (3)
__construct(32-38)__toString(55-58)getInnerEvent(45-48)src/EntityContext.php (1)
__construct(66-80)src/Events/HasInnerEventInterface.php (1)
getInnerEvent(31-31)
🪛 PHPMD (2.15.0)
src/Events/WithDelay.php
32-32: Avoid unused parameters such as '$eventId'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (22)
src/Events/ShareWithRole.php (1)
45-45: LGTM: Explicit return type improves type safety.Adding the explicit
: stringreturn type declaration enhances type safety and IDE support while maintaining backward compatibility. This change aligns with similar updates across other event classes in the codebase.src/Events/ShareWithUser.php (1)
45-45: LGTM: Consistent type safety improvement.Adding the explicit
: stringreturn type declaration maintains consistency with other event classes and improves type safety. The implementation remains functionally identical.src/Events/WithOrchestration.php (1)
35-35: LGTM: Property visibility change aligns with interface requirements.Changing the
innerEventproperty fromprivate readonlytopublic readonlyprovides direct access to the inner event while maintaining immutability. This change aligns with the updatedHasInnerEventInterfacerequirements and supports the event wrapping pattern without compromising encapsulation.src/Events/RevokeUser.php (1)
44-44: LGTM: Consistent return type declaration.Adding the explicit
: stringreturn type follows the established pattern across event classes and improves type safety without any functional changes.src/Events/HasInnerEventInterface.php (1)
29-30: No action needed: PHP ≥8.4 is already enforced and all event types exposeinnerEvent.– composer.json requires
"php": ">=8.4"
– All classes carrying aninnerEvent(e.g. in src/Events/*) already declare it as a public property
– No additional implementers of HasInnerEventInterface need updatingtests/Unit/EventDescriptionTest.php (1)
148-161: LGTM! Test mock properly aligned with interface changes.The MockWrapperEvent class correctly implements the updated HasInnerEventInterface pattern by:
- Standardizing property name from
$innerto$innerEvent- Making the property public for consistency with other wrapper classes
- Updating all references accordingly
This ensures test coverage remains accurate for the new event wrapper architecture.
src/Events/WithPriority.php (1)
61-62: LGTM! Improved attribute formatting.Moving the
#[\Override]attribute to its own line improves readability and follows PHP coding standards.src/Events/ShareOwnership.php (3)
27-28: LGTM! Proper imports for event sourcing attributes.The imports correctly bring in the NeedsSource attribute and Operation enum to support the event provenance system.
31-31: LGTM! Event properly annotated for ownership operations.The
#[NeedsSource(Operation::Owner)]attribute correctly indicates this event requires source context for ownership operations, supporting the WithFrom wrapping pattern.
44-44: LGTM! Explicit return type improves type safety.Adding the explicit
stringreturn type to__toString()improves type safety and follows PHP best practices.src/OrchestrationContext.php (3)
38-38: LGTM!The import is correctly placed and follows the existing import organization pattern.
81-86: LGTM!The event wrapping is correctly applied to add source context to the activity call.
158-164: Consistent application of source trackingAll event dispatches are properly wrapped with
addFrom(), ensuring consistent source tracking throughout the orchestration context. The implementation correctly handles both synchronous and asynchronous event patterns.Also applies to: 169-179, 215-221, 236-241, 407-407, 418-423, 648-648, 669-669
src/EntityContext.php (4)
64-64: Correct implementation of source trackingThe use of a private readonly property initialized in the constructor is the correct approach, avoiding the static variable issue found in
OrchestrationContext.Also applies to: 79-79
132-135: Clean and efficient wrapper methodThe
addFrommethod is concise and correctly wraps events with the source context.
101-104: Verify: Events inreturnmethod are not wrapped with source contextThe events dispatched in the
returnmethod are not wrapped withaddFrom(), unlike all other event dispatches in this class. If this is intentional (e.g., because these events target orchestrations rather than entities), please add a comment explaining why source tracking is not needed here.
120-125: Consistent source tracking across all entity operationsAll entity-related event dispatches are properly wrapped with
addFrom(), ensuring traceability of event origins.Also applies to: 155-160, 201-206, 218-223, 230-235, 242-247, 254-259, 266-271, 278-283
src/Events/WithFrom.php (1)
1-28: Well-designed event wrapper implementationThe
WithFromclass follows the established pattern of other event wrappers and correctly implements source tracking functionality. The use of a static factory method and the readable__toString()implementation are good design choices.src/Events/WithDelay.php (2)
27-27: Good refactoring to use importsUsing import statements instead of fully qualified class names improves readability.
Also applies to: 32-32, 37-37
46-49: Correct removal of eventId synchronizationThe removal of the
$this->innerEvent->eventId = $this->eventId;line is appropriate since the Event base class now handles eventId synchronization for inner events.src/Events/Event.php (2)
30-30: Clean refactoring with proper importsGood practice to use import statements and explicitly declare the
Stringableinterface implementation.Also applies to: 32-32, 35-35, 39-39
41-49: Elegant eventId synchronization using property hooksThe property hook implementation ensures that eventId changes are automatically propagated to inner events, maintaining consistency across wrapped events. This is a clean solution to the synchronization problem.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
cli/auth/resourceManager.go (1)
48-50: Update function documentation to include the newfromparameter.The function comment for
DiscoverResourceshould be updated to document the newfrom *ids.StateIdparameter and its purpose.// DiscoverResource is a method of the ResourceManager struct that is responsible for discovering a resource based on -// the provided context, state ID, logger, and preventCreation flag +// the provided context, state ID, from state ID, logger, and preventCreation flag func (r *ResourceManager) DiscoverResource(ctx context.Context, id *ids.StateId, from *ids.StateId, logger *zap.Logger, preventCreation bool) (*Resource, error) {cli/auth/resource.go (1)
55-56: Consider using a set structure for AllowedFromTypesFor consistency with the
Ownersfield which usesmap[UserId]struct{}as a set, consider usingmap[string]struct{}forAllowedFromTypesto avoid duplicates and improve lookup performance.- AllowedFromTypes []string `json:"allowed_from_types"` + AllowedFromTypes map[string]struct{} `json:"allowed_from_types"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cli/auth/createPermissions.go(1 hunks)cli/auth/resource.go(5 hunks)cli/auth/resourceManager.go(2 hunks)cli/ids/id.go(3 hunks)cli/lib/api.go(11 hunks)cli/lib/consumer.go(10 hunks)src/Events/EventDescription.php(2 hunks)src/Glue/glue.php(3 hunks)src/State/Attributes/AllowCreateFrom.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cli/auth/createPermissions.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
cli/lib/api.go (1)
cli/ids/id.go (1)
ApiSource(17-20)
src/State/Attributes/AllowCreateFrom.php (1)
cli/ids/id.go (1)
EntityId(139-142)
cli/auth/resourceManager.go (2)
cli/ids/id.go (1)
StateId(181-184)cli/auth/resource.go (1)
Resource(48-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (15)
cli/ids/id.go (2)
182-183: LGTM!The JSON struct tags are properly formatted and follow Go conventions. The
omitemptyoption is appropriate for these fields.
81-87: Breaking change impact verified – no updates neededI searched the entire repo for calls to
StateId.Name()and only found it used as a map key incli/auth/resource.go(viacache.Store(id.Name(), …)/cache.Load(id.Name())). SinceIdKindis an underlyingstring, using it as aninterface{}key continues to work, and there are no other callers expecting a plainstringreturn. No further changes are required here.src/Events/EventDescription.php (2)
44-44: LGTM!The new
fromproperty is properly typed as nullableStateIdand follows the existing pattern of other optional properties in the class.
100-102: LGTM!The
WithFromhandling logic follows the same pattern as other event wrapper checks in the loop. Theinstanceofcheck and property assignment are correctly implemented.cli/lib/api.go (1)
265-265: LGTM!All
DiscoverResourcemethod calls are consistently updated with the newids.ApiSourceparameter in the correct position. The changes align with the method signature enhancement to support "from" context in resource discovery.Also applies to: 315-315, 357-357, 433-433, 488-488, 574-574, 685-685, 761-761, 816-816, 881-881, 1035-1035
src/State/Attributes/AllowCreateFrom.php (3)
11-12: LGTM!The attribute definition correctly targets classes and allows repetition, which is appropriate for specifying multiple allowed creation sources.
14-19: LGTM!The type definitions are well-designed. The template annotation properly constrains the
$typeparameter toEntityStatesubclasses, and the union type for$idappropriately handles both entity and orchestration instances.
21-27: LGTM!The constructor validation correctly enforces mutual exclusivity between
$typeand$idparameters. The logic is sound and the error messages are clear and helpful.src/Glue/glue.php (2)
39-39: LGTM!The import statement for
AllowCreateFromis correctly placed and follows the existing pattern of importing attribute classes.
262-263: LGTM!The new array keys
'from'and'from-type'are properly initialized following the existing pattern of other permission arrays.cli/auth/resource.go (2)
106-107: Good simplification!The removal of the redundant nil check is appropriate since
currentUseris already validated at the beginning of the method.
128-135: Enhanced security with source-based authorizationThe addition of the
fromparameter and theAllowedFromcheck strengthens security by ensuring resources can only be created from authorized sources.cli/lib/consumer.go (3)
156-167: Good naming improvementThe rename from
shouldCreatetopreventCreationmakes the boolean's purpose clearer and the logic more intuitive.
268-303: Clarify the difference between ShareOwnership and GiveOwnershipBoth
ShareOwnershipandGiveOwnershiphave identical implementations. If they are meant to be different operations, the implementation should reflect that. Otherwise, consider consolidating them.The third parameter
truein both calls toresource.ShareOwnershipsuggests keeping permissions. ShouldGiveOwnershippassfalseto transfer ownership without retaining permissions?
170-170: Consistent updates to DiscoverResource callsAll calls to
DiscoverResourcehave been properly updated to include the newsourceIdparameter, maintaining consistency with the enhanced authorization model.Also applies to: 329-329, 369-369
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/State/Attributes/DenyAnyOperation.php (1)
31-39: Consider adding parameter validation.Unlike
AllowCreateFromwhich validates that at least one parameter is provided, this attribute accepts all null parameters, which would create a meaningless deny rule.public function __construct( public ?string $fromType = null, public EntityId|OrchestrationInstance|null $fromId = null, public ?string $fromUser = null, public ?string $fromRole = null, -) {} +) { + if ($fromType === null && $fromId === null && $fromUser === null && $fromRole === null) { + throw new \LogicException('At least one parameter must be provided to DenyAnyOperation'); + } +}Also consider if
AllowAnyOperationneeds similar validation for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
cli/auth/resource.go(6 hunks)cli/auth/resourceManager.go(3 hunks)cli/glue/glue.go(4 hunks)cli/ids/id.go(3 hunks)cli/lib/api.go(12 hunks)cli/lib/consumer.go(11 hunks)src/Contexts/AuthContext.php(1 hunks)src/Contexts/AuthContext/SecurityException.php(1 hunks)src/Contexts/AuthContext/functions.php(2 hunks)src/Glue/glue.php(5 hunks)src/State/AbstractHistory.php(4 hunks)src/State/ActivityHistory.php(3 hunks)src/State/Attributes/AccessControl.php(1 hunks)src/State/Attributes/AllowAnyOperation.php(1 hunks)src/State/Attributes/AllowCreateAll.php(1 hunks)src/State/Attributes/AllowCreateForAuth.php(1 hunks)src/State/Attributes/AllowCreateForRole.php(1 hunks)src/State/Attributes/AllowCreateForUser.php(1 hunks)src/State/Attributes/AllowCreateFrom.php(1 hunks)src/State/Attributes/DenyAnyOperation.php(1 hunks)src/State/EntityHistory.php(5 hunks)src/State/Ids/StateId.php(0 hunks)src/Task.php(1 hunks)tests/Unit/ActivityHistoryTest.php(2 hunks)tests/Unit/EntityHistoryTest.php(4 hunks)tests/Unit/LockIntegrationTest.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/State/Ids/StateId.php
✅ Files skipped from review due to trivial changes (6)
- src/State/Attributes/AccessControl.php
- src/Contexts/AuthContext/SecurityException.php
- src/Contexts/AuthContext/functions.php
- src/Task.php
- src/State/Attributes/AllowAnyOperation.php
- src/Contexts/AuthContext.php
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/lib/api.go
- cli/auth/resourceManager.go
- cli/lib/consumer.go
- src/Glue/glue.php
- cli/ids/id.go
- cli/auth/resource.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/Unit/ActivityHistoryTest.php (3)
src/State/EntityId.php (2)
from(42-45)EntityId(33-51)src/State/Ids/StateId.php (1)
fromEntityId(67-70)src/functions.php (1)
EntityId(13-16)
src/State/Attributes/AllowCreateForRole.php (4)
src/State/Attributes/AllowCreateAll.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateForUser.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateFrom.php (1)
Attribute(11-28)src/State/Attributes/AllowCreateForAuth.php (1)
Attribute(29-33)
src/State/Attributes/AllowCreateFrom.php (7)
src/State/Attributes/AllowAnyOperation.php (2)
Attribute(9-18)__construct(12-17)src/State/Attributes/AllowCreateAll.php (2)
Attribute(29-33)__construct(32-32)src/State/Attributes/AllowCreateForRole.php (2)
Attribute(29-33)__construct(32-32)src/State/Attributes/AllowCreateForUser.php (2)
Attribute(29-33)__construct(32-32)src/State/Attributes/AllowCreateForAuth.php (2)
Attribute(29-33)__construct(32-32)src/State/Attributes/DenyAnyOperation.php (2)
Attribute(31-40)__construct(34-39)cli/ids/id.go (1)
EntityId(144-147)
src/State/Attributes/AllowCreateForUser.php (11)
src/State/Attributes/AllowAnyOperation.php (1)
Attribute(9-18)src/State/Attributes/AllowCreateAll.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateForRole.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateFrom.php (1)
Attribute(11-28)src/State/Attributes/AllowCreateForAuth.php (1)
Attribute(29-33)src/State/Attributes/DenyAnyOperation.php (1)
Attribute(31-40)src/Events/Shares/NeedsSource.php (1)
Attribute(27-31)src/Events/Shares/NeedsTarget.php (1)
Attribute(27-31)src/Proxy/Pure.php (1)
Attribute(30-31)src/State/Attributes/EntryPoint.php (1)
Attribute(27-28)src/State/Attributes/Operation.php (1)
Attribute(27-31)
src/State/Attributes/AllowCreateAll.php (5)
src/State/Attributes/AllowCreateForRole.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateForUser.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateFrom.php (1)
Attribute(11-28)src/State/Attributes/AllowCreateForAuth.php (1)
Attribute(29-33)tests/Common/LauncherEntity.php (1)
AllowCreateAll(32-46)
src/State/Attributes/AllowCreateForAuth.php (11)
src/State/Attributes/AllowAnyOperation.php (1)
Attribute(9-18)src/State/Attributes/AllowCreateAll.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateForRole.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateForUser.php (1)
Attribute(29-33)src/State/Attributes/AllowCreateFrom.php (1)
Attribute(11-28)src/State/Attributes/DenyAnyOperation.php (1)
Attribute(31-40)src/Events/Shares/NeedsSource.php (1)
Attribute(27-31)src/Events/Shares/NeedsTarget.php (1)
Attribute(27-31)src/Proxy/Pure.php (1)
Attribute(30-31)src/State/Attributes/EntryPoint.php (1)
Attribute(27-28)src/State/Attributes/Operation.php (1)
Attribute(27-31)
src/State/Attributes/DenyAnyOperation.php (5)
src/State/Attributes/AllowAnyOperation.php (2)
Attribute(9-18)__construct(12-17)src/State/Attributes/AllowCreateForRole.php (2)
Attribute(29-33)__construct(32-32)src/State/Attributes/AllowCreateForUser.php (2)
Attribute(29-33)__construct(32-32)src/State/Attributes/AllowCreateFrom.php (2)
Attribute(11-28)__construct(19-27)cli/ids/id.go (1)
EntityId(144-147)
🪛 PHPMD (2.15.0)
src/State/AbstractHistory.php
67-67: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
67-67: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
77-77: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
77-77: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
87-87: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
87-87: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
102-102: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
102-102: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (19)
tests/Unit/ActivityHistoryTest.php (1)
68-68: LGTM! Proper provenance tracking setup.The addition of the
fromproperty assignment correctly establishes provenance information for the test scenario, aligning with the PR's objective to add "from capability" for tracking event origins.tests/Unit/LockIntegrationTest.php (2)
29-29: LGTM! Necessary imports for provenance tracking.The imports for
StateIdandOrchestrationInstanceare correctly added to support the new provenance functionality.Also applies to: 32-32
54-54: LGTM! Consistent provenance setup.The
fromproperty assignment properly establishes the orchestration instance as the source for this entity, maintaining consistency with the broader provenance tracking implementation.tests/Unit/EntityHistoryTest.php (1)
64-64: LGTM! Comprehensive provenance tracking in all test scenarios.The
fromproperty assignments are consistently applied across all three test cases (signal processing, locked events, and lock chaining), ensuring proper test coverage for the new provenance tracking functionality.Also applies to: 88-88, 141-141
src/State/Attributes/AllowCreateForRole.php (2)
27-27: LGTM! Clean import organization.Adding the explicit
Attributeimport improves code readability and follows PHP best practices.
29-30: LGTM! Consistent access control attribute pattern.The changes align
AllowCreateForRolewith the unified access control system:
- Simplified attribute declaration using only
IS_REPEATABLEflag (consistent withAllowCreateFrom)- Implementation of
AccessControlinterface for unified access control handlingreadonlymodifier for immutability (consistent withAllowCreateForAuth)src/State/Attributes/AllowCreateForUser.php (2)
27-27: LGTM! Clean import organization.Adding the explicit
Attributeimport improves code readability and follows PHP best practices.
29-30: LGTM! Consistent access control attribute pattern.The changes align
AllowCreateForUserwith the unified access control system:
- Simplified attribute declaration using only
IS_REPEATABLEflag (consistent withAllowCreateForRoleandAllowCreateFrom)- Implementation of
AccessControlinterface for unified access control handlingsrc/State/ActivityHistory.php (3)
27-27: LGTM!The new imports are properly organized and correctly used for the access control implementation.
Also applies to: 39-39, 45-45
91-95: LGTM!The access control check for callable tasks is correctly implemented using reflection to retrieve AccessControl attributes and properly throws a SecurityException with contextual information when access is denied.
99-108: Refactor access control checks for better efficiency and debugging.The current implementation performs duplicate work and has identical error messages. Consider:
- The entrypoint location (Line 104) happens even if class-level access is denied
- Both error messages are identical, making debugging difficult
$task = $this->container->get($task); $reflection = new ReflectionClass($task); $attrs = $reflection->getAttributes(AccessControl::class, ReflectionAttribute::IS_INSTANCEOF); if (! $this->checkAccessControl($this->user, $this->from, ...$attrs)) { - throw new SecurityException(sprintf('Access denied to activity %s', $this->activityId)); + throw new SecurityException(sprintf('Access denied to activity %s (class-level)', $this->activityId)); } $entrypoint = $this->locateEntrypoint($reflection) ?? throw new RuntimeException("Unable to locate entrypoint for {$event->name}"); $attrs = $entrypoint->getAttributes(AccessControl::class, ReflectionAttribute::IS_INSTANCEOF); if (! $this->checkAccessControl($this->user, $this->from, ...$attrs)) { - throw new SecurityException(sprintf('Access denied to activity %s', $this->activityId)); + throw new SecurityException(sprintf('Access denied to activity %s (method-level)', $this->activityId)); }Likely an incorrect or invalid review comment.
src/State/Attributes/AllowCreateAll.php (1)
27-30: Verify the removal of TARGET_CLASS restriction is intentional.The attribute declaration changed from
#[\Attribute(\Attribute::TARGET_CLASS)]to#[Attribute], making it applicable to any target (methods, properties, etc.) rather than just classes. This seems inconsistent with the attribute name "AllowCreateAll" which implies class-level authorization.Consider whether this attribute should maintain the
TARGET_CLASSrestriction or if the broader applicability is intentional. If broader usage is intended, document the valid targets.src/State/Attributes/AllowCreateForAuth.php (1)
27-30: LGTM!The changes properly align this attribute with the access control framework. The
readonlymodifier ensures immutability of the configuration.src/State/Attributes/AllowCreateFrom.php (1)
11-27: Well-designed access control attribute!The implementation correctly enforces mutual exclusivity between type and ID parameters, provides clear error messages, and uses proper type constraints. The repeatable attribute allows for flexible configuration of multiple allowed sources.
cli/glue/glue.go (1)
76-76: LGTM! Provenance tracking properly integrated.The addition of the
fromparameter and its propagation through the glue layer is implemented correctly. TheDPHP_SOURCEheader appropriately transmits the source StateId to the PHP backend.Also applies to: 106-106, 115-115, 129-129
src/State/EntityHistory.php (2)
210-213: Well-structured access control implementation.The multi-level access control checks (class, property, and method) provide comprehensive security coverage. The consistent use of
checkAccessControlmethod and appropriate exception handling ensures proper authorization enforcement.Also applies to: 257-259
230-231: PHP 8.4 property hooks are supported
The project’s composer.json specifies"php": ">=8.4", so usingPropertyHookType::GetandPropertyHookType::Setis fully compatible. No changes required.src/State/AbstractHistory.php (2)
144-217: Comprehensive access control implementation.The
checkAccessControlmethod provides a well-structured evaluation of various access control attributes. The logic correctly handles different permission scenarios and follows a clear precedence order.A few observations:
- The method returns
truewhen no access controls are defined (line 148), delegating to runtime checksAllowCreateAllimmediately grants access (line 154)- Deny rules are evaluated last and can override allow rules
- The default return is
false(line 216), following a secure-by-default approach
54-57: Proper configuration of the$fromproperty.The
$fromproperty is correctly marked with#[Field(exclude: true)]to prevent serialization, which is appropriate for runtime provenance tracking that shouldn't be persisted with the state.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/State/AbstractHistory.php(4 hunks)tests/Unit/AbstractHistoryTest.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/Unit/AbstractHistoryTest.php
46-46: Avoid unused parameters such as '$logger'. (Unused Code Rules)
(UnusedFormalParameter)
46-46: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
62-62: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
67-67: Avoid unused parameters such as '$logger'. (Unused Code Rules)
(UnusedFormalParameter)
72-72: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
src/State/AbstractHistory.php
67-67: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
67-67: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
77-77: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
77-77: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
87-87: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
87-87: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
102-102: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
102-102: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$original'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (5)
tests/Unit/AbstractHistoryTest.php (3)
42-77: LGTM! Test implementation is appropriate.The
TestableAbstractHistoryclass correctly exposes the protectedcheckAccessControlmethod for testing. The unused parameter warnings from static analysis are false positives since these methods must match the abstract interface signatures.
80-110: Well-designed test helper function.The
createAttributefunction cleverly creates mockReflectionAttributeinstances for testing without requiring actual PHP attributes. The dynamic instantiation correctly handles attribute arguments.
113-416: Comprehensive test coverage for access control.The test suite thoroughly covers all access control scenarios:
- All attribute types (
AllowCreateAll,AllowCreateForAuth,AllowCreateForRole,AllowCreateForUser,AllowCreateFrom,AllowAnyOperation,DenyAnyOperation)- Edge cases (null users, empty roles, no matches)
- Complex scenarios (multiple attributes, deny overriding allow)
The tests effectively validate the access control logic implemented in
AbstractHistory::checkAccessControl().src/State/AbstractHistory.php (2)
54-57: Property additions look good.The explicit nullable type for
$statusimproves clarity, and the new$fromproperty appropriately tracks state origin with serialization exclusion.
144-221: Align PHP version requirements and improve condition readabilityThe project’s root
composer.jsonalready requires PHP ≥8.4 (soarray_any()is available), but there’s a secondarycomposer.jsonset to PHP ≥8.3. Please ensure allcomposer.jsonfiles target PHP 8.4+ or supply a polyfill forarray_any()to avoid runtime errors.Separately, the combined
isEntityId()/toEntityId()->nameandisOrchestrationId()/toOrchestrationInstance()->instanceIdchecks on lines 166 and 214 are hard to follow. Consider extracting those blocks into named methods, for example:
private function matchesFromId(StateId $from, mixed $id): boolprivate function matchesFromType(StateId $from, string $type): boolThis will make the deny/allow flow in
checkAccessControl()much clearer.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes