Document command payload resolution behavior #10
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the server-dotnet implementation by replacing EntityInfo and RoomManager with a more robust session management system (EntitySession, EntitySpec, SessionStore) and introduces a comprehensive permission service. The changes add authentication/authorization checks, improve entity visibility controls, and document command payload resolution behavior.
Key changes:
- Replaced
EntityInfo/RoomManagerwithEntitySpec/EntitySession/SessionStorefor better session and entity management - Added
PermissionServicefor centralized authorization logic (command permissions, workspace access, direct messaging, artifact promotion) - Introduced authentication requirements for owner-visibility entities and documented command payload resolution patterns
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| SecurityTests.cs | New test suite covering authentication, authorization, and permission scenarios |
| RoomHub_SmokeTests.cs | Updated to use EntitySpec instead of EntityInfo |
| SessionStore.cs | New service for managing entity sessions with indexed lookups |
| RoomManager.cs | Removed (replaced by SessionStore) |
| PermissionService.cs | New service implementing authorization rules for commands, workspaces, direct messages, and promotions |
| ErrorFactory.cs | New utility for creating standardized error responses |
| Program.cs | Updated dependency injection to register new services |
| PolicySpec.cs | New model defining entity command and sandbox policies |
| ErrorResponse.cs | New model for standardized error responses |
| EntitySpec.cs | New model representing entity specifications with visibility and ownership |
| EntitySession.cs | New model representing active entity sessions |
| EntityInfo.cs | Removed (replaced by EntitySpec/EntitySession) |
| RoomHub.cs | Extensively refactored to use new session/permission infrastructure and document command payload resolution |
| ArtifactEndpoints.cs | Added permission checks for artifact operations using new services |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public string Kind { get; set; } = default!; // human|agent|npc|orchestrator | ||
| public string? DisplayName { get; set; } | ||
| public string Visibility { get; set; } = "team"; // public|team|owner | ||
| public string? OwnerUserId { get; set; } // obrigatório se visibility==owner |
There was a problem hiding this comment.
Comment contains Portuguese text 'obrigatório'. Should be changed to English: 'required if visibility==owner'.
| public string? OwnerUserId { get; set; } // obrigatório se visibility==owner | |
| public string? OwnerUserId { get; set; } // required if visibility==owner |
Copilot uses AI. Check for mistakes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var userId = ResolveUserId(); | ||
| if (string.Equals(entity.Visibility, "owner", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(entity.OwnerUserId)) | ||
| { | ||
| throw ErrorFactory.HubBadRequest("INVALID_ENTITY_SPEC", "owner visibility requires owner_user_id"); | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(userId)) | ||
| { | ||
| throw ErrorFactory.HubForbidden("AUTH_REQUIRED", "owner visibility requires authentication"); | ||
| } | ||
|
|
||
| if (!string.Equals(entity.OwnerUserId, userId, StringComparison.Ordinal)) | ||
| { | ||
| throw ErrorFactory.HubForbidden("PERM_DENIED", "owner mismatch"); | ||
| } | ||
| } | ||
|
|
||
| userId ??= entity.OwnerUserId; | ||
|
|
||
| _sessions.RemoveByConnection(Context.ConnectionId); | ||
|
|
||
| var normalized = NormalizeEntity(entity); | ||
| var session = new EntitySession | ||
| { | ||
| ConnectionId = Context.ConnectionId, | ||
| RoomId = roomId, | ||
| Entity = normalized, | ||
| JoinedAt = DateTime.UtcNow, | ||
| UserId = userId |
There was a problem hiding this comment.
The Join method assigns userId ??= entity.OwnerUserId even when the caller is unauthenticated and the entity is not marked as owner. This lets any client join with visibility="team" (or public) while populating OwnerUserId with an arbitrary value; the session is then recorded as if it were that user. Because permission checks in PermissionService rely on EntitySession.UserId (for example to authorize commands or direct messages to owner‑protected entities), an unauthenticated client can impersonate an owner simply by setting the field in the payload. Only authenticated joins should be allowed to set UserId or the value should be ignored when authentication is absent.
Useful? React with 👍 / 👎.
1 participant
Summary
ResolveCommandTargetTesting
https://chatgpt.com/codex/tasks/task_e_68f30ef788c0832e8b94ef5ab7c4dbd5