|
| 1 | +# patterns-entity-sources |
| 2 | + |
| 3 | +CRITICAL: Which entity provides which data in Grant/Revoke operations. |
| 4 | + |
| 5 | +--- |
| 6 | + |
| 7 | +## The Problem |
| 8 | + |
| 9 | +Grant and Revoke operations receive two entities: |
| 10 | +- **Principal** - who is receiving/losing access |
| 11 | +- **Entitlement** - what access is being granted/revoked |
| 12 | + |
| 13 | +Confusion about which entity provides which data caused 3 production reverts and is the #1 high-impact bug pattern. |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## The Rule |
| 18 | + |
| 19 | +| Data Type | Source | Example | |
| 20 | +|-----------|--------|---------| |
| 21 | +| Context (workspace, org, tenant) | **Principal** | `principal.ParentResourceId.Resource` | |
| 22 | +| User/account ID | **Principal** | `principal.Id.Resource` | |
| 23 | +| Permission/role being granted | **Entitlement** | `entitlement.Resource.Id.Resource` | |
| 24 | +| Group being added to | **Entitlement** | `entitlement.Resource.Id.Resource` | |
| 25 | + |
| 26 | +**Principal = WHO. Entitlement = WHAT.** |
| 27 | + |
| 28 | +--- |
| 29 | + |
| 30 | +## Correct Pattern |
| 31 | + |
| 32 | +```go |
| 33 | +func (g *groupBuilder) Grant(ctx context.Context, principal *v2.Resource, |
| 34 | + entitlement *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { |
| 35 | + |
| 36 | + // WHO is receiving the grant |
| 37 | + userID := principal.Id.Resource |
| 38 | + |
| 39 | + // WHAT is being granted |
| 40 | + groupID := entitlement.Resource.Id.Resource |
| 41 | + |
| 42 | + // Context comes from principal (the user's workspace) |
| 43 | + var workspaceID string |
| 44 | + if principal.ParentResourceId != nil { |
| 45 | + workspaceID = principal.ParentResourceId.Resource |
| 46 | + } |
| 47 | + |
| 48 | + // API call uses correct values |
| 49 | + err := g.client.AddMember(ctx, workspaceID, groupID, userID) |
| 50 | + // ... |
| 51 | +} |
| 52 | +``` |
| 53 | + |
| 54 | +--- |
| 55 | + |
| 56 | +## Wrong Pattern (Caused Reverts) |
| 57 | + |
| 58 | +```go |
| 59 | +func (g *groupBuilder) Grant(ctx context.Context, principal *v2.Resource, |
| 60 | + entitlement *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { |
| 61 | + |
| 62 | + // WRONG: Getting workspace from entitlement |
| 63 | + workspaceID := entitlement.Resource.ParentResourceId.Resource // BUG! |
| 64 | + |
| 65 | + // This grants access in the WRONG workspace |
| 66 | + err := g.client.AddMember(ctx, workspaceID, groupID, userID) |
| 67 | +} |
| 68 | +``` |
| 69 | + |
| 70 | +This bug grants user access in the entitlement's workspace, not the user's workspace. |
| 71 | + |
| 72 | +--- |
| 73 | + |
| 74 | +## Revoke Pattern |
| 75 | + |
| 76 | +In Revoke, the grant contains both: |
| 77 | + |
| 78 | +```go |
| 79 | +func (g *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) { |
| 80 | + |
| 81 | + // Principal is in grant.Principal |
| 82 | + userID := grant.Principal.Id.Resource |
| 83 | + |
| 84 | + // Entitlement is in grant.Entitlement |
| 85 | + groupID := grant.Entitlement.Resource.Id.Resource |
| 86 | + |
| 87 | + // Context still comes from principal |
| 88 | + var workspaceID string |
| 89 | + if grant.Principal.ParentResourceId != nil { |
| 90 | + workspaceID = grant.Principal.ParentResourceId.Resource |
| 91 | + } |
| 92 | + |
| 93 | + err := g.client.RemoveMember(ctx, workspaceID, groupID, userID) |
| 94 | + // ... |
| 95 | +} |
| 96 | +``` |
| 97 | + |
| 98 | +--- |
| 99 | + |
| 100 | +## API Argument Order (6+ PRs with this bug) |
| 101 | + |
| 102 | +Functions with multiple `string` parameters of the same type are easy to call incorrectly. |
| 103 | + |
| 104 | +**The Problem**: |
| 105 | +```go |
| 106 | +// API client signature |
| 107 | +func (c *Client) AddMember(groupID, userID string) error |
| 108 | + |
| 109 | +// Connector code - which is which? |
| 110 | +err := client.AddMember(principal.Id.Resource, entitlement.Resource.Id.Resource) |
| 111 | +// Is this (userID, groupID) or (groupID, userID)? Easy to get wrong. |
| 112 | +``` |
| 113 | + |
| 114 | +**WRONG - Arguments swapped**: |
| 115 | +```go |
| 116 | +// Grants userID to... userID? And groupID gets added to group groupID? |
| 117 | +err := client.AddMember(userID, groupID) // BACKWARDS! |
| 118 | +``` |
| 119 | + |
| 120 | +**CORRECT - Match API signature**: |
| 121 | +```go |
| 122 | +// Check API docs: AddMember(groupID, userID) |
| 123 | +err := client.AddMember(groupID, userID) |
| 124 | +``` |
| 125 | + |
| 126 | +**Prevention - Use named variables**: |
| 127 | +```go |
| 128 | +func (g *groupBuilder) Grant(...) { |
| 129 | + // Name variables to match their PURPOSE |
| 130 | + targetGroupID := entitlement.Resource.Id.Resource // WHAT |
| 131 | + userToAdd := principal.Id.Resource // WHO |
| 132 | + |
| 133 | + // Now the call is self-documenting |
| 134 | + err := g.client.AddMember(targetGroupID, userToAdd) |
| 135 | +} |
| 136 | +``` |
| 137 | + |
| 138 | +**Prevention - Verify against API docs**: |
| 139 | +```go |
| 140 | +// Before writing the call, check: |
| 141 | +// 1. Open API documentation |
| 142 | +// 2. Find the exact parameter order |
| 143 | +// 3. Write a comment if order is non-obvious: |
| 144 | +// AddMember adds userID to groupID (not the other way around) |
| 145 | +err := client.AddMember(groupID, userID) |
| 146 | +``` |
| 147 | + |
| 148 | +**Code smell**: Multiple adjacent `string` parameters = high swap risk. |
| 149 | + |
| 150 | +--- |
| 151 | + |
| 152 | +## Detection in Code Review |
| 153 | + |
| 154 | +**Red flags:** |
| 155 | +1. `entitlement.Resource.ParentResourceId` - probably should be `principal.ParentResourceId` |
| 156 | +2. Multiple `string` parameters in API calls - verify order |
| 157 | +3. Getting "context" (workspace/org/tenant) from entitlement |
| 158 | + |
| 159 | +**Questions to ask:** |
| 160 | +- "Where does the workspace ID come from?" |
| 161 | +- "Is this getting data from the right entity?" |
| 162 | +- "Does the argument order match the API signature?" |
| 163 | + |
| 164 | +--- |
| 165 | + |
| 166 | +## Test Cases |
| 167 | + |
| 168 | +Test these scenarios: |
| 169 | +1. Principal in workspace A, entitlement in workspace B - should use workspace A |
| 170 | +2. Arguments in correct order for API |
| 171 | +3. Grant to correct user (not some other user) |
| 172 | +4. Revoke from correct user |
| 173 | + |
| 174 | +```go |
| 175 | +func TestGrant_UsesCorrectWorkspace(t *testing.T) { |
| 176 | + // Principal is in workspace "ws-user" |
| 177 | + // Entitlement is in workspace "ws-entitlement" |
| 178 | + // Grant should happen in "ws-user" |
| 179 | +} |
| 180 | +``` |
0 commit comments