ECOSYSTEM-2859 | feat: Implement authorization#1022
Open
tupyy wants to merge 1 commit intokubev2v:mainfrom
Open
ECOSYSTEM-2859 | feat: Implement authorization#1022tupyy wants to merge 1 commit intokubev2v:mainfrom
tupyy wants to merge 1 commit intokubev2v:mainfrom
Conversation
tupyy
requested review from
AvielSegev and
ronenav
and removed request for
a team
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds relation-based authorization: new authz store/model/migrations, an AssessmentServicer interface, an AuthzAssessmentService wrapper enforcing permissions, handler refactors to rely on service-level auth checks (mapping ErrForbidden → 403), and tests exercising authz behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as API Handler
participant AuthzSvc as AuthzAssessmentService
participant Store as Store (Data + Authz)
participant DB as Database
rect rgba(200,200,255,0.5)
Client->>Handler: HTTP request (Get/List/Create/Update/Delete)
end
rect rgba(200,255,200,0.5)
Handler->>AuthzSvc: call assessment operation (ctx,...)
AuthzSvc->>Store: Authz().ListResources / GetPermissions / WriteRelationships
Store->>DB: SQL queries/inserts/deletes (relations / assessments / transactions)
DB-->>Store: rows / ack
Store-->>AuthzSvc: permissions / resources / ack
AuthzSvc->>AuthzSvc: enforce permission check
alt allowed
AuthzSvc->>AuthzSvc: delegate to inner AssessmentServicer
AuthzSvc->>Store: (on create/delete) write/delete relations in tx
AuthzSvc-->>Handler: result
else forbidden
AuthzSvc-->>Handler: ErrForbidden
end
end
rect rgba(255,200,200,0.5)
Handler->>Client: HTTP 200 / 403 / 404 / 500
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/handlers/v1alpha1/estimation_test.go (1)
386-433: 🧹 Nitpick | 🔵 TrivialAuthorization tests refactored consistently.
The tests now use
&ForbiddenAssessmentService{}to verify handler behavior when the service returns a forbidden error. This is cleaner than the previous approach of seeding mismatched ownership data.Minor observation: The two tests "returns 403 when user doesn't own assessment (different username)" and "returns 403 when user doesn't own assessment (different organization)" are now functionally identical since both use the same mock. Consider consolidating them or renaming to better reflect what's actually being tested (handler response mapping).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/v1alpha1/estimation_test.go` around lines 386 - 433, The two test cases "returns 403 when user doesn't own assessment (different username)" and "returns 403 when user doesn't own assessment (different organization)" are identical because both use the same mock (&ForbiddenAssessmentService{}); either consolidate them into a single test or rename one to reflect that both validate the handler's mapping of a forbidden error to a 403 response. Update the spec by removing one redundant It block or changing its description to something like "returns 403 when assessment service returns forbidden" and ensure the test still constructs the handler via handlers.NewServiceHandler(...) and calls handler.CalculateMigrationEstimation(...) expecting a server.CalculateMigrationEstimation403JSONResponse with "forbidden" in Message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/assessment.go`:
- Around line 58-68: The code currently treats an empty slice in filter.IDs the
same as "no ID filter", causing an unintended unfiltered query; change the
handling so that if filter.IDs is non-nil and len(filter.IDs) == 0 you
short-circuit and return an empty result set instead of building storeFilter.
Specifically, in the function that builds storeFilter (where storeFilter :=
store.NewAssessmentQueryFilter() and you check filter.IDs, filter.Username,
filter.OrgID), detect the case where filter.IDs was provided but is empty (e.g.,
filter.IDs != nil && len(filter.IDs) == 0) and immediately return an empty
assessments slice (or an appropriate empty response) before calling the store
Find; otherwise keep the existing WithIDs(...) behavior to represent a real ID
filter. This preserves the distinction between “no ID filter requested” and
“requested but empty” and avoids hitting internal/store/assessment.go
Find(&assessments) with no predicates.
In `@internal/service/authz_assessment_test.go`:
- Around line 102-118: The test uses a raw SQL lookup to pick an arbitrary
resource_id for user1 which may pick the wrong assessment; instead reference the
specific assessment ID created in the test setup (e.g., use the known variable
for the first assessment such as a1.ID or the stored assessment ID from
BeforeEach) when inserting the viewer relation and creating ctx, so replace the
Raw("SELECT resource_id ...") lookup with the explicit assessment ID variable
and keep the insert call (insertRelationStm) and subsequent assertions
unchanged.
In `@internal/service/authz_assessment.go`:
- Around line 35-42: The loop that builds ids from resources silently skips
unparseable UUIDs (uuid.Parse(r.ID)), which can hide data issues; update the
code in authz_assessment.go where ids, resources and uuid.Parse are used to log
a warning or error when uuid.Parse fails (include r.ID and the parse error)
instead of just continuing — use the existing logger/context in this service (or
return an error if appropriate) so invalid UUIDs are observable and traceable.
- Around line 44-48: The code mutates the caller's filter by setting filter.IDs,
filter.Username, and filter.OrgID before calling a.inner.ListAssessments; create
a shallow copy of the incoming filter (e.g., newFilter := *filter) and modify
newFilter.IDs/Username/OrgID instead, then pass newFilter (or &newFilter) to
a.inner.ListAssessments to avoid surprising external callers and preserve the
original filter object.
In `@internal/store/authz.go`:
- Around line 36-58: WriteRelationships currently performs per-item DB
operations in a loop (AuthzStore.WriteRelationships iterating over
[]model.RelationshipUpdate and calling Create or Delete for each), which is slow
for larger batches; fix it by collecting all touch-operation rows (use
u.Relationship.ToSql() for each model.OperationTouch) and call GORM's
CreateInBatches for that slice, and handle deletes by either batching them with
a single WHERE ... IN query (collect keys from rows and call Delete once) or
keep per-item deletes if low volume, ensuring you still honor the existing
behavior for model.OperationDelete; keep the switch over model.OperationTouch
and model.OperationDelete but replace individual db.Create calls with batch
insert for touch rows and batch delete for delete rows where appropriate.
In `@internal/store/doc.go`:
- Around line 101-111: The two org-sharing examples stop at Build() and never
persist the relationship batch; update each snippet that constructs updates via
store.NewRelationshipBuilder().With(...).Build() to open a transaction using
store.NewTransactionContext (or the same transaction helper used earlier), call
store.WriteRelationships(ctx, updates) to persist the relationships, and then
call tx.Commit() (or Commit on the transaction object used elsewhere) to
finalize the transaction so the relationships are actually saved.
In `@internal/store/store.go`:
- Around line 49-60: NewStore currently returns a DataStore with authz == nil
which lets store.Authz() be dereferenced elsewhere and cause a panic; update
NewStore (and the other constructor variants around NewStoreWithAuthz/NewStore)
to always initialize the authz field (call NewAuthzStore(db) like
NewStoreWithAuthz does) or alternatively make the constructor return an
error/panic when authz cannot be constructed so callers never receive a
DataStore with a nil authz; ensure references to DataStore.authz and the Authz()
accessor no longer can return nil.
In `@pkg/migrations/sql/20260325120000_add_relations_table.sql`:
- Around line 12-13: The new index (idx_relations_resource_relation) doesn't
match the predicates used in internal/store/authz.go; remove or replace it with
two composite indexes that match the hot read paths: one on (resource,
subject_namespace, subject_id) and another on (resource, resource_id,
subject_namespace, subject_id). Update the migration so the relations table has
idx_relations_resource_subject covering (resource, subject_namespace,
subject_id) and idx_relations_resource_resid_subject covering (resource,
resource_id, subject_namespace, subject_id) to ensure queries in
internal/store/authz.go are covered.
In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql`:
- Around line 1-7: The INSERT into relations selects assessments.username which
may be NULL; update the SELECT query in the migration (the INSERT ... SELECT ...
FROM assessments a block) to exclude rows where a.username IS NULL by adding a
WHERE clause (e.g., WHERE a.username IS NOT NULL) so no relation rows are
inserted with subject_id = NULL; keep the rest of the INSERT (ON CONFLICT ... DO
NOTHING) unchanged.
---
Outside diff comments:
In `@internal/handlers/v1alpha1/estimation_test.go`:
- Around line 386-433: The two test cases "returns 403 when user doesn't own
assessment (different username)" and "returns 403 when user doesn't own
assessment (different organization)" are identical because both use the same
mock (&ForbiddenAssessmentService{}); either consolidate them into a single test
or rename one to reflect that both validate the handler's mapping of a forbidden
error to a 403 response. Update the spec by removing one redundant It block or
changing its description to something like "returns 403 when assessment service
returns forbidden" and ensure the test still constructs the handler via
handlers.NewServiceHandler(...) and calls
handler.CalculateMigrationEstimation(...) expecting a
server.CalculateMigrationEstimation403JSONResponse with "forbidden" in Message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e30eb728-52d1-499e-aa26-9382ea2f79d7
📒 Files selected for processing (23)
internal/api_server/server.gointernal/handlers/v1alpha1/assessment.gointernal/handlers/v1alpha1/assessment_test.gointernal/handlers/v1alpha1/estimation.gointernal/handlers/v1alpha1/estimation_test.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/handlers/v1alpha1/source.gointernal/service/assessment.gointernal/service/authz_assessment.gointernal/service/authz_assessment_test.gointernal/service/errors.gointernal/service/sizer_test.gointernal/store/authz.gointernal/store/authz_builder.gointernal/store/authz_test.gointernal/store/doc.gointernal/store/model/authz.gointernal/store/options.gointernal/store/store.gointernal/store/transaction.gopkg/migrations/sql/20260325120000_add_relations_table.sqlpkg/migrations/sql/20260325120001_backfill_owner_relations.sql
Comment on lines
+102
to
+118
| It("viewer relation grants read access for listing", func() { | ||
| // Give user3 viewer access to one of user1's assessments | ||
| var resourceID string | ||
| tx := gormdb.Raw("SELECT resource_id FROM relations WHERE subject_id = 'user1' LIMIT 1").Scan(&resourceID) | ||
| Expect(tx.Error).To(BeNil()) | ||
|
|
||
| tx = gormdb.Exec(fmt.Sprintf(insertRelationStm, "assessment", resourceID, "viewer", "user", "user3")) | ||
| Expect(tx.Error).To(BeNil()) | ||
|
|
||
| ctx := ctxWithUser("user3", "org1") | ||
| filter := &service.AssessmentFilter{} | ||
|
|
||
| assessments, err := svc.ListAssessments(ctx, filter) | ||
|
|
||
| Expect(err).To(BeNil()) | ||
| Expect(assessments).To(HaveLen(1)) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Query may select unintended assessment in edge cases.
The query SELECT resource_id FROM relations WHERE subject_id = 'user1' LIMIT 1 could return any assessment owned by user1, not necessarily one created in the current test's BeforeEach. Since BeforeEach creates assessments a1 and a2 for user1, the test works, but the intent would be clearer by explicitly using one of those IDs.
Consider using a known assessment ID from the setup instead:
- var resourceID string
- tx := gormdb.Raw("SELECT resource_id FROM relations WHERE subject_id = 'user1' LIMIT 1").Scan(&resourceID)
- Expect(tx.Error).To(BeNil())
+ // Use one of the assessments created in BeforeEach
+ var resourceID string
+ tx := gormdb.Raw("SELECT resource_id FROM relations WHERE subject_id = 'user1' AND relation = 'owner' LIMIT 1").Scan(&resourceID)
+ Expect(tx.Error).To(BeNil())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/authz_assessment_test.go` around lines 102 - 118, The test
uses a raw SQL lookup to pick an arbitrary resource_id for user1 which may pick
the wrong assessment; instead reference the specific assessment ID created in
the test setup (e.g., use the known variable for the first assessment such as
a1.ID or the stored assessment ID from BeforeEach) when inserting the viewer
relation and creating ctx, so replace the Raw("SELECT resource_id ...") lookup
with the explicit assessment ID variable and keep the insert call
(insertRelationStm) and subsequent assertions unchanged.
Comment on lines
+35
to
+42
| ids := make([]uuid.UUID, 0, len(resources)) | ||
| for _, r := range resources { | ||
| id, err := uuid.Parse(r.ID) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| ids = append(ids, id) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent skip of unparseable resource IDs may mask data corruption.
When uuid.Parse(r.ID) fails, the code silently skips the resource. This could hide database corruption or schema migration issues where invalid UUIDs were written to the relations table.
Consider logging when a resource ID fails to parse:
🔧 Proposed fix
ids := make([]uuid.UUID, 0, len(resources))
for _, r := range resources {
id, err := uuid.Parse(r.ID)
if err != nil {
+ // Log warning - this indicates data inconsistency in relations table
continue
}
ids = append(ids, id)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/authz_assessment.go` around lines 35 - 42, The loop that
builds ids from resources silently skips unparseable UUIDs (uuid.Parse(r.ID)),
which can hide data issues; update the code in authz_assessment.go where ids,
resources and uuid.Parse are used to log a warning or error when uuid.Parse
fails (include r.ID and the parse error) instead of just continuing — use the
existing logger/context in this service (or return an error if appropriate) so
invalid UUIDs are observable and traceable.
Comment on lines
+44
to
+48
| filter.IDs = ids | ||
| filter.Username = "" | ||
| filter.OrgID = "" | ||
|
|
||
| return a.inner.ListAssessments(ctx, filter) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Filter mutation may surprise callers.
The method modifies the caller's filter object by setting IDs, Username, and OrgID. This could cause unexpected behavior if the caller reuses the filter.
Consider working with a copy:
♻️ Proposed fix
- filter.IDs = ids
- filter.Username = ""
- filter.OrgID = ""
-
- return a.inner.ListAssessments(ctx, filter)
+ authzFilter := &AssessmentFilter{
+ IDs: ids,
+ }
+ return a.inner.ListAssessments(ctx, authzFilter)📝 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.
Suggested change
| filter.IDs = ids | |
| filter.Username = "" | |
| filter.OrgID = "" | |
| return a.inner.ListAssessments(ctx, filter) | |
| authzFilter := &AssessmentFilter{ | |
| IDs: ids, | |
| } | |
| return a.inner.ListAssessments(ctx, authzFilter) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/authz_assessment.go` around lines 44 - 48, The code mutates
the caller's filter by setting filter.IDs, filter.Username, and filter.OrgID
before calling a.inner.ListAssessments; create a shallow copy of the incoming
filter (e.g., newFilter := *filter) and modify newFilter.IDs/Username/OrgID
instead, then pass newFilter (or &newFilter) to a.inner.ListAssessments to avoid
surprising external callers and preserve the original filter object.
Comment on lines
+36
to
+58
| func (a *AuthzStore) WriteRelationships(ctx context.Context, updates []model.RelationshipUpdate) error { | ||
| if len(updates) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| db := a.getDB(ctx) | ||
| for _, u := range updates { | ||
| row := u.Relationship.ToSql() | ||
| switch u.Operation { | ||
| case model.OperationTouch: | ||
| if err := db.Clauses(clause.OnConflict{DoNothing: true}).Create(&row).Error; err != nil { | ||
| return err | ||
| } | ||
| case model.OperationDelete: | ||
| if err := db.Where("resource = ? AND resource_id = ? AND relation = ? AND subject_namespace = ? AND subject_id = ?", | ||
| row.Resource, row.ResourceID, row.Relation, row.SubjectNamespace, row.SubjectID, | ||
| ).Delete(&model.RelationSqlModel{}).Error; err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider batch insert for improved performance.
WriteRelationships creates rows one-by-one in a loop. For large batches, this could be slow due to multiple round-trips.
GORM supports batch creation via CreateInBatches. However, since this would complicate the mixed touch/delete logic and typical usage appears to be small batches (1-3 relations per assessment), this is a low-priority optimization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/store/authz.go` around lines 36 - 58, WriteRelationships currently
performs per-item DB operations in a loop (AuthzStore.WriteRelationships
iterating over []model.RelationshipUpdate and calling Create or Delete for
each), which is slow for larger batches; fix it by collecting all
touch-operation rows (use u.Relationship.ToSql() for each model.OperationTouch)
and call GORM's CreateInBatches for that slice, and handle deletes by either
batching them with a single WHERE ... IN query (collect keys from rows and call
Delete once) or keep per-item deletes if low volume, ensuring you still honor
the existing behavior for model.OperationDelete; keep the switch over
model.OperationTouch and model.OperationDelete but replace individual db.Create
calls with batch insert for touch rows and batch delete for delete rows where
appropriate.
Comment on lines
+12
to
+13
| CREATE INDEX idx_relations_subject ON relations (subject_namespace, subject_id); | ||
| CREATE INDEX idx_relations_resource_relation ON relations (resource, relation); |
There was a problem hiding this comment.
Index the predicates the authz queries actually use.
The hot read paths in internal/store/authz.go filter relations by (resource, subject_namespace, subject_id) and (resource, resource_id, subject_namespace, subject_id). The new (resource, relation) index does not match those predicates, so permission checks will degrade into broader scans as the table grows.
📈 Proposed index layout
CREATE INDEX idx_relations_subject ON relations (subject_namespace, subject_id);
-CREATE INDEX idx_relations_resource_relation ON relations (resource, relation);
+CREATE INDEX idx_relations_resource_subject ON relations (resource, subject_namespace, subject_id);
+CREATE INDEX idx_relations_resource_id_subject ON relations (resource, resource_id, subject_namespace, subject_id);📝 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.
Suggested change
| CREATE INDEX idx_relations_subject ON relations (subject_namespace, subject_id); | |
| CREATE INDEX idx_relations_resource_relation ON relations (resource, relation); | |
| CREATE INDEX idx_relations_subject ON relations (subject_namespace, subject_id); | |
| CREATE INDEX idx_relations_resource_subject ON relations (resource, subject_namespace, subject_id); | |
| CREATE INDEX idx_relations_resource_id_subject ON relations (resource, resource_id, subject_namespace, subject_id); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/migrations/sql/20260325120000_add_relations_table.sql` around lines 12 -
13, The new index (idx_relations_resource_relation) doesn't match the predicates
used in internal/store/authz.go; remove or replace it with two composite indexes
that match the hot read paths: one on (resource, subject_namespace, subject_id)
and another on (resource, resource_id, subject_namespace, subject_id). Update
the migration so the relations table has idx_relations_resource_subject covering
(resource, subject_namespace, subject_id) and
idx_relations_resource_resid_subject covering (resource, resource_id,
subject_namespace, subject_id) to ensure queries in internal/store/authz.go are
covered.
Comment on lines
+1
to
+7
| -- +goose Up | ||
| -- +goose StatementBegin | ||
| INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id) | ||
| SELECT 'assessment', a.id::text, 'owner', 'user', a.username | ||
| FROM assessments a | ||
| ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING; | ||
| -- +goose StatementEnd |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the assessments table schema for NOT NULL constraints
rg -n "username" pkg/migrations/sql --type sql -B 2 -A 2Repository: kubev2v/migration-planner
Length of output: 5041
🏁 Script executed:
#!/bin/bash
# Search for any ALTER TABLE statements that modify username column constraints
rg -n "ALTER TABLE.*assessments.*username" pkg/migrations/sql --type sql
# Also check if there are any NOT NULL constraints added
rg -n "NOT NULL" pkg/migrations/sql --type sql | grep -i usernameRepository: kubev2v/migration-planner
Length of output: 631
🏁 Script executed:
#!/bin/bash
# Check if there's a DEFAULT value or other constraints on the username column
rg -n "username" pkg/migrations/sql/20251002131125_union_migrations.sql --type sql -B 1 -A 1
# Also check the full assessments table definition in the initial migration
rg -n "CREATE TABLE.*assessments" pkg/migrations/sql --type sql -A 15Repository: kubev2v/migration-planner
Length of output: 1584
🏁 Script executed:
#!/bin/bash
# Check if there are any other migrations that might modify username constraints after assessments table was first created
git ls-files pkg/migrations/sql/*.sql | sort | tail -20
# Let's also verify the full schema by looking at the migration that drops the username column
cat -n pkg/migrations/sql/20251002131125_union_migrations.sql | sed -n '45,60p'Repository: kubev2v/migration-planner
Length of output: 1729
Add WHERE clause to filter out NULL usernames.
The INSERT statement will create relations with subject_id = NULL for any assessments with NULL usernames. Since the username column was added without a NOT NULL constraint and can accept NULL values from source data, these rows must be filtered out:
Suggested fix
INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id)
SELECT 'assessment', a.id::text, 'owner', 'user', a.username
FROM assessments a
+WHERE a.username IS NOT NULL
ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING;📝 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.
Suggested change
| -- +goose Up | |
| -- +goose StatementBegin | |
| INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id) | |
| SELECT 'assessment', a.id::text, 'owner', 'user', a.username | |
| FROM assessments a | |
| ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING; | |
| -- +goose StatementEnd | |
| -- +goose Up | |
| -- +goose StatementBegin | |
| INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id) | |
| SELECT 'assessment', a.id::text, 'owner', 'user', a.username | |
| FROM assessments a | |
| WHERE a.username IS NOT NULL | |
| ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING; | |
| -- +goose StatementEnd |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql` around lines
1 - 7, The INSERT into relations selects assessments.username which may be NULL;
update the SELECT query in the migration (the INSERT ... SELECT ... FROM
assessments a block) to exclude rows where a.username IS NULL by adding a WHERE
clause (e.g., WHERE a.username IS NOT NULL) so no relation rows are inserted
with subject_id = NULL; keep the rest of the INSERT (ON CONFLICT ... DO NOTHING)
unchanged.
4c8accc to
8a77388
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/store/store.go (1)
107-118:⚠️ Potential issue | 🟡 MinorAlways close the base DB even if authz cleanup fails.
Returning immediately on
s.authz.Close()skipss.db.DB().Close(). That leaves the main connection pool open on partial shutdown. Capture the authz error, continue closings.db, then return the combined failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/store.go` around lines 107 - 118, DataStore.Close currently returns immediately if s.authz.Close() fails, skipping closing the underlying SQL connection; change Close so it calls s.authz.Close() and stores that error (e.g., authzErr), then always proceed to get sqlDB via s.db.DB() and call sqlDB.Close(), capture that error (e.g., dbErr), and finally return a combined error when both exist or the single non-nil error otherwise; update the logic in DataStore.Close to reference s.authz.Close, s.db.DB(), and sqlDB.Close to ensure the DB is always closed even if authz cleanup fails.
♻️ Duplicate comments (4)
internal/store/store.go (1)
13-13:⚠️ Potential issue | 🔴 CriticalDon't expose a nullable
Authz()from a validStore.
Authz()is now part of the interface, butNewStorestill constructsDataStorewithauthz == nil. Any caller that keeps usingNewStorecan now pass a seemingly valid store into authz-aware assessment paths and panic on the first authorization lookup. InitializeauthzinNewStoretoo, or fail fast instead of returning nil.Also applies to: 49-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/store.go` at line 13, The Store interface now exposes Authz(), but NewStore currently constructs a DataStore with authz == nil which can cause panics; update NewStore to initialize the authz field on DataStore (or return an error/fail fast) so Authz() never returns nil for a valid Store, or alternatively change NewStore to return an error when it cannot create a non-nil authz; locate DataStore, NewStore and the Authz() method and ensure DataStore.authz is assigned a concrete implementation before returning the store (or make NewStore fail fast).internal/store/doc.go (1)
101-111:⚠️ Potential issue | 🟡 MinorFinish the org-sharing examples.
Both snippets stop at
Build(), so a reader copying them never persists the relationships. Add the same transaction/WriteRelationships/Commitflow used in the earlier examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/doc.go` around lines 101 - 111, The two org-sharing examples stop at Build() and need the same persistence flow as earlier examples: after calling Build() on NewRelationshipBuilder produce a transaction variable (tx), call WriteRelationships(ctx, tx, updates) to persist the built relationships, then call tx.Commit() to finalize; update both snippets to include creating the transaction, passing it into WriteRelationships, and committing the transaction so the relationships are actually saved.pkg/migrations/sql/20260325120001_backfill_owner_relations.sql (1)
3-6:⚠️ Potential issue | 🟠 MajorFilter out assessments with
NULLusernames before backfilling.
relations.subject_idisNOT NULL, so a single legacy assessment row withusername = NULLwill make this whole migration fail. Exclude those rows in theSELECT.Suggested fix
INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id) SELECT 'assessment', a.id::text, 'owner', 'user', a.username FROM assessments a +WHERE a.username IS NOT NULL ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql` around lines 3 - 6, The INSERT ... SELECT into relations is failing if assessments.username is NULL because relations.subject_id is NOT NULL; update the SELECT from assessments (table assessments, alias a) used by the INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id) to exclude rows with NULL usernames by adding a filter (e.g., WHERE a.username IS NOT NULL) so only non-null subject_id values are backfilled; keep the ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING as-is.internal/service/authz_assessment.go (1)
35-46:⚠️ Potential issue | 🟠 MajorFail closed on malformed authz resource IDs.
Lines 37-39 silently drop bad
resource_idvalues, then Lines 44-46 clear the originalUsername/OrgIDfilters before delegating. If the inner list path treats an emptyIDsslice as "no ID filter", one malformed authz row turns into a broader query instead of a denial. Return an error on the first parse failure, or at minimum short-circuit to an empty result once no valid IDs remain.Run the following script to confirm whether an empty
AssessmentFilter.IDssuppresses the DB query or falls back to the unfiltered path:#!/bin/bash set -euo pipefail echo "=== ListAssessments implementations ===" rg -n -C10 --type go 'func \(.*\) ListAssessments\(' internal/service echo echo "=== AssessmentFilter field usage ===" rg -n -C5 --type go '\b(IDs|Username|OrgID)\b' internal/service internal/storeIf the inner implementation only applies the ID predicate when
len(filter.IDs) > 0, this wrapper needs to fail closed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/authz_assessment.go` around lines 35 - 46, The code currently silently skips malformed resource IDs (in the loop using uuid.Parse on r.ID) then clears filter.Username and filter.OrgID before delegating, which can broaden the query; change the behavior in the authz assessment wrapper (the block that builds ids and sets filter.IDs) to fail closed: on the first uuid.Parse error return an error to the caller (do not continue), and only clear filter.Username/filter.OrgID and assign filter.IDs if all provided IDs parsed successfully and ids is non-empty; alternatively, if you prefer allowing some valid IDs but none remain, short-circuit and return an empty result immediately rather than delegating to ListAssessments (so that an empty IDs slice does not drop the ID predicate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/v1alpha1/estimation_test.go`:
- Around line 763-764: The test descriptions in estimation_test.go still say
"returns 403 when user has a different username" but the setup uses
ForbiddenAssessmentService and exercises a forbidden error path; rename the It
blocks (the test descriptions near the handlers.NewServiceHandler(...) calls
that pass ForbiddenAssessmentService) to something like "returns 403 when
assessment service forbids access" (or consolidate both similar cases into one
test) so the name accurately reflects that the handler behavior is driven by
ForbiddenAssessmentService rather than a username/org mismatch.
In `@internal/handlers/v1alpha1/sizer_test.go`:
- Around line 1013-1018: The test replaced the real assessment service with
ForbiddenAssessmentService causing the "different username" and "different
organization" cases to only validate status-code mapping rather than real
ownership rules; update the test to keep at least one concrete, service-backed
auth case by restoring or adding a real assessment service (or equivalent
behavior on AuthzAssessmentService) when constructing the handler in
handlers.NewServiceHandler so that same-username/different-org flows exercise
actual ownership logic (e.g., use the real AssessmentService instead of
ForbiddenAssessmentService for one test case or add an equivalent concrete test
for AuthzAssessmentService while leaving ForbiddenAssessmentService for purely
403-mapping checks).
In `@internal/service/authz_assessment.go`:
- Around line 24-28: The owner resolution currently only uses user.Username
(seen around auth.MustHaveUser(ctx) and the call to
a.store.Authz().ListResources(ctx, user.Username, model.AssessmentResource)),
which drops the organization component and allows same usernames in different
orgs to be treated as the same owner; update the owner authorization checks in
authz_assessment.go (all blocks that call ListResources or resolve owner tuples)
to include the user's organization (e.g., user.Org or equivalent) as part of the
subject/key passed to Authz.ListResources or to explicitly validate org equality
when matching owners, and add a regression test that verifies two users with the
same username in different orgs do not share owner-only permissions.
- Around line 54-60: The authorization logic currently returns NewErrForbidden
whenever permissions are missing; instead, preserve 404 semantics by checking
whether the assessment actually exists before mapping lack of access to 403.
After calling a.store.Authz().GetPermissions (and before returning
NewErrForbidden when model.ReadPermission.In(resource.Permissions) is false),
attempt to fetch the assessment (e.g., via a.store.Assessments().Get(ctx, id) or
the existing inner service method like a.svc.GetAssessment/Get) and if that call
returns ErrResourceNotFound, return that error directly; only if the assessment
exists should you return NewErrForbidden. Ensure the same change is applied to
the other two authorization blocks (the ones around lines 100-106 and 115-121)
that use model.ReadPermission.In and NewErrForbidden.
In `@internal/service/sizer_test.go`:
- Around line 38-40: The MockStore.Authz() implementation currently returns nil
which can cause nil pointer panics when code (e.g., AuthzAssessmentService)
calls store.Authz().ListResources() or other methods; replace the nil return
with a no-op/mock implementation of the store.Authz interface (a lightweight
mock that implements the same methods used in tests, such as ListResources) and
have MockStore.Authz() return that instance so tests that reuse MockStore won't
panic; update the mock type (e.g., add a noopAuthzMock struct and constructor)
and ensure MockStore.Authz() returns that mock instead of nil.
In `@internal/store/transaction.go`:
- Around line 29-31: The commit/rollback functions silently return when the
caller is not the transaction owner (checked via ctx.Value(txOwnerKey).(bool)),
which hampers debugging; update the Commit and Rollback methods to log a
debug-level message when the ownership check fails (include identifying context
such as a tx id or caller info if available) before returning, using the package
logger or context-aware logger so non-owner attempts are recorded without
changing behavior.
---
Outside diff comments:
In `@internal/store/store.go`:
- Around line 107-118: DataStore.Close currently returns immediately if
s.authz.Close() fails, skipping closing the underlying SQL connection; change
Close so it calls s.authz.Close() and stores that error (e.g., authzErr), then
always proceed to get sqlDB via s.db.DB() and call sqlDB.Close(), capture that
error (e.g., dbErr), and finally return a combined error when both exist or the
single non-nil error otherwise; update the logic in DataStore.Close to reference
s.authz.Close, s.db.DB(), and sqlDB.Close to ensure the DB is always closed even
if authz cleanup fails.
---
Duplicate comments:
In `@internal/service/authz_assessment.go`:
- Around line 35-46: The code currently silently skips malformed resource IDs
(in the loop using uuid.Parse on r.ID) then clears filter.Username and
filter.OrgID before delegating, which can broaden the query; change the behavior
in the authz assessment wrapper (the block that builds ids and sets filter.IDs)
to fail closed: on the first uuid.Parse error return an error to the caller (do
not continue), and only clear filter.Username/filter.OrgID and assign filter.IDs
if all provided IDs parsed successfully and ids is non-empty; alternatively, if
you prefer allowing some valid IDs but none remain, short-circuit and return an
empty result immediately rather than delegating to ListAssessments (so that an
empty IDs slice does not drop the ID predicate).
In `@internal/store/doc.go`:
- Around line 101-111: The two org-sharing examples stop at Build() and need the
same persistence flow as earlier examples: after calling Build() on
NewRelationshipBuilder produce a transaction variable (tx), call
WriteRelationships(ctx, tx, updates) to persist the built relationships, then
call tx.Commit() to finalize; update both snippets to include creating the
transaction, passing it into WriteRelationships, and committing the transaction
so the relationships are actually saved.
In `@internal/store/store.go`:
- Line 13: The Store interface now exposes Authz(), but NewStore currently
constructs a DataStore with authz == nil which can cause panics; update NewStore
to initialize the authz field on DataStore (or return an error/fail fast) so
Authz() never returns nil for a valid Store, or alternatively change NewStore to
return an error when it cannot create a non-nil authz; locate DataStore,
NewStore and the Authz() method and ensure DataStore.authz is assigned a
concrete implementation before returning the store (or make NewStore fail fast).
In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql`:
- Around line 3-6: The INSERT ... SELECT into relations is failing if
assessments.username is NULL because relations.subject_id is NOT NULL; update
the SELECT from assessments (table assessments, alias a) used by the INSERT INTO
relations (resource, resource_id, relation, subject_namespace, subject_id) to
exclude rows with NULL usernames by adding a filter (e.g., WHERE a.username IS
NOT NULL) so only non-null subject_id values are backfilled; keep the ON
CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f3a11a5-ba59-42ec-908c-acfb071c3765
📒 Files selected for processing (24)
internal/api_server/server.gointernal/handlers/v1alpha1/assessment.gointernal/handlers/v1alpha1/assessment_test.gointernal/handlers/v1alpha1/estimation.gointernal/handlers/v1alpha1/estimation_test.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/handlers/v1alpha1/source.gointernal/service/assessment.gointernal/service/authz_assessment.gointernal/service/authz_assessment_test.gointernal/service/errors.gointernal/service/sizer_test.gointernal/store/authz.gointernal/store/authz_builder.gointernal/store/authz_test.gointernal/store/doc.gointernal/store/model/authz.gointernal/store/options.gointernal/store/store.gointernal/store/transaction.gopkg/migrations/migrations_test.gopkg/migrations/sql/20260325120000_add_relations_table.sqlpkg/migrations/sql/20260325120001_backfill_owner_relations.sql
Comment on lines
763
to
+764
| It("returns 403 when user has a different username", func() { | ||
| mockStore.assessments[assessmentID] = createTestAssessmentForComplexityHandler(assessmentID, "other-user", user.Organization, clusterID) | ||
| handler = handlers.NewServiceHandler(nil, service.NewAssessmentService(mockStore, nil), nil, nil, service.NewEstimationService(mockStore)) | ||
| handler = handlers.NewServiceHandler(nil, &ForbiddenAssessmentService{}, nil, nil, service.NewEstimationService(mockStore)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider updating test descriptions to reflect the new test approach.
The tests now verify handler behavior when the assessment service returns a forbidden error, rather than testing username/org mismatch scenarios. While the current names are acceptable, updating them would improve clarity:
📝 Optional: Clearer test names
-It("returns 403 when user has a different username", func() {
+It("returns 403 when assessment service returns forbidden error", func() {Or consolidate into a single test since both cases now exercise identical code paths.
Also applies to: 777-778
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/v1alpha1/estimation_test.go` around lines 763 - 764, The
test descriptions in estimation_test.go still say "returns 403 when user has a
different username" but the setup uses ForbiddenAssessmentService and exercises
a forbidden error path; rename the It blocks (the test descriptions near the
handlers.NewServiceHandler(...) calls that pass ForbiddenAssessmentService) to
something like "returns 403 when assessment service forbids access" (or
consolidate both similar cases into one test) so the name accurately reflects
that the handler behavior is driven by ForbiddenAssessmentService rather than a
username/org mismatch.
Comment on lines
1013
to
1018
| handler = handlers.NewServiceHandler( | ||
| nil, // sourceService | ||
| service.NewAssessmentService(mockStore, nil), | ||
| &ForbiddenAssessmentService{}, | ||
| nil, // jobService | ||
| service.NewSizerService(sizerClient, mockStore), | ||
| nil, // estimationService |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep one service-backed auth regression in these 403 cases.
Lines 1015 and 1045 swap the real assessment service for a stub that always returns ErrForbidden, so the "different username" and "different organization" cases no longer exercise the actual ownership rule—only the handler's status-code mapping. Since this PR moves auth into AuthzAssessmentService, keep at least one concrete service-backed case (or add the equivalent there) so same-username/different-org regressions do not go green unnoticed.
As per coding guidelines, **/*_test.go: "Coverage: Strive for high test coverage on critical logic paths."
Also applies to: 1043-1048
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/v1alpha1/sizer_test.go` around lines 1013 - 1018, The test
replaced the real assessment service with ForbiddenAssessmentService causing the
"different username" and "different organization" cases to only validate
status-code mapping rather than real ownership rules; update the test to keep at
least one concrete, service-backed auth case by restoring or adding a real
assessment service (or equivalent behavior on AuthzAssessmentService) when
constructing the handler in handlers.NewServiceHandler so that
same-username/different-org flows exercise actual ownership logic (e.g., use the
real AssessmentService instead of ForbiddenAssessmentService for one test case
or add an equivalent concrete test for AuthzAssessmentService while leaving
ForbiddenAssessmentService for purely 403-mapping checks).
Comment on lines
+24
to
+28
| user := auth.MustHaveUser(ctx) | ||
|
|
||
| resources, err := a.store.Authz().ListResources(ctx, user.Username, model.AssessmentResource) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("authz: failed to list resources: %w", err) |
There was a problem hiding this comment.
Keep the org component in owner authorization.
Lines 24-28, 54-60, 82-87, 100-106, and 115-121 key owner resolution entirely on user.Username. The old flow compared both username and organization, so this drops the org half of the ownership boundary: the same username in another org now resolves the same owner tuple. Either encode org into the authz subject/key or retain the org check in this wrapper before granting owner-only permissions. Please add a same-username/different-org regression once this is fixed.
Also applies to: 54-60, 82-87, 100-106, 115-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/authz_assessment.go` around lines 24 - 28, The owner
resolution currently only uses user.Username (seen around auth.MustHaveUser(ctx)
and the call to a.store.Authz().ListResources(ctx, user.Username,
model.AssessmentResource)), which drops the organization component and allows
same usernames in different orgs to be treated as the same owner; update the
owner authorization checks in authz_assessment.go (all blocks that call
ListResources or resolve owner tuples) to include the user's organization (e.g.,
user.Org or equivalent) as part of the subject/key passed to Authz.ListResources
or to explicitly validate org equality when matching owners, and add a
regression test that verifies two users with the same username in different orgs
do not share owner-only permissions.
Comment on lines
+54
to
+60
| resource, err := a.store.Authz().GetPermissions(ctx, user.Username, model.NewAssessmentResource(id.String())) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("authz: failed to get permissions: %w", err) | ||
| } | ||
|
|
||
| if !model.ReadPermission.In(resource.Permissions) { | ||
| return nil, NewErrForbidden("assessment", id.String()) |
There was a problem hiding this comment.
Preserve 404 semantics for missing assessments.
Lines 54-60, 100-106, and 115-121 authorize before the inner service runs. With no matching relation, these methods now return ErrForbidden even when the assessment does not exist, which is a behavior change from the previous fetch-then-authorize flow and from the PR objective to preserve the existing ownership behavior. Distinguish ErrResourceNotFound before turning lack of access into 403.
Also applies to: 100-106, 115-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/authz_assessment.go` around lines 54 - 60, The authorization
logic currently returns NewErrForbidden whenever permissions are missing;
instead, preserve 404 semantics by checking whether the assessment actually
exists before mapping lack of access to 403. After calling
a.store.Authz().GetPermissions (and before returning NewErrForbidden when
model.ReadPermission.In(resource.Permissions) is false), attempt to fetch the
assessment (e.g., via a.store.Assessments().Get(ctx, id) or the existing inner
service method like a.svc.GetAssessment/Get) and if that call returns
ErrResourceNotFound, return that error directly; only if the assessment exists
should you return NewErrForbidden. Ensure the same change is applied to the
other two authorization blocks (the ones around lines 100-106 and 115-121) that
use model.ReadPermission.In and NewErrForbidden.
Comment on lines
+38
to
+40
| func (m *MockStore) Authz() store.Authz { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Returning nil from Authz() may cause nil pointer panics if this mock is reused.
The SizerService tests don't exercise authz code paths currently, so this works. However, if this MockStore is used with AuthzAssessmentService (or similar wrappers), calls like store.Authz().ListResources() will panic due to nil dereference (see internal/service/authz_assessment.go lines 26, 54, 86, 136).
Consider returning a no-op mock implementation instead of nil to prevent accidental panics if test coverage expands.
💡 Suggested safer alternative
func (m *MockStore) Authz() store.Authz {
- return nil
+ return &MockAuthzStore{}
}
+
+type MockAuthzStore struct{}
+
+func (m *MockAuthzStore) ListResources(ctx context.Context, subjectID string, resource model.Resource) ([]uuid.UUID, error) {
+ return nil, nil
+}
+
+func (m *MockAuthzStore) GetPermissions(ctx context.Context, subjectID string, resource model.Resource) (*model.Resource, error) {
+ return nil, nil
+}
+
+func (m *MockAuthzStore) WriteRelationships(ctx context.Context, updates []model.RelationUpdate) error {
+ return nil
+}
+
+func (m *MockAuthzStore) DeleteRelationships(ctx context.Context, resource model.Resource) error {
+ return nil
+}📝 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.
Suggested change
| func (m *MockStore) Authz() store.Authz { | |
| return nil | |
| } | |
| func (m *MockStore) Authz() store.Authz { | |
| return &MockAuthzStore{} | |
| } | |
| type MockAuthzStore struct{} | |
| func (m *MockAuthzStore) ListResources(ctx context.Context, subjectID string, resource model.Resource) ([]uuid.UUID, error) { | |
| return nil, nil | |
| } | |
| func (m *MockAuthzStore) GetPermissions(ctx context.Context, subjectID string, resource model.Resource) (*model.Resource, error) { | |
| return nil, nil | |
| } | |
| func (m *MockAuthzStore) WriteRelationships(ctx context.Context, updates []model.RelationUpdate) error { | |
| return nil | |
| } | |
| func (m *MockAuthzStore) DeleteRelationships(ctx context.Context, resource model.Resource) error { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/sizer_test.go` around lines 38 - 40, The MockStore.Authz()
implementation currently returns nil which can cause nil pointer panics when
code (e.g., AuthzAssessmentService) calls store.Authz().ListResources() or other
methods; replace the nil return with a no-op/mock implementation of the
store.Authz interface (a lightweight mock that implements the same methods used
in tests, such as ListResources) and have MockStore.Authz() return that instance
so tests that reuse MockStore won't panic; update the mock type (e.g., add a
noopAuthzMock struct and constructor) and ensure MockStore.Authz() returns that
mock instead of nil.
Comment on lines
+29
to
+31
| if owner, _ := ctx.Value(txOwnerKey).(bool); !owner { | ||
| return ctx, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Transaction ownership pattern is sound, but silent no-ops may complicate debugging.
The ownership tracking correctly prevents nested code from committing/rolling back the outer transaction. However, the silent no-op returns could make it harder to debug scenarios where a caller incorrectly assumes they own the transaction.
Consider adding debug-level logging when a non-owner attempts commit/rollback to aid troubleshooting:
🔍 Optional: Add debug logging for non-owner attempts
func Commit(ctx context.Context) (context.Context, error) {
tx, ok := ctx.Value(transactionKey).(*Tx)
if !ok {
return ctx, nil
}
if owner, _ := ctx.Value(txOwnerKey).(bool); !owner {
+ zap.S().Named("transaction").Debugf("commit skipped: caller is not transaction owner")
return ctx, nil
}
// ...
}
func Rollback(ctx context.Context) (context.Context, error) {
tx, ok := ctx.Value(transactionKey).(*Tx)
if !ok {
return ctx, nil
}
if owner, _ := ctx.Value(txOwnerKey).(bool); !owner {
+ zap.S().Named("transaction").Debugf("rollback skipped: caller is not transaction owner")
return ctx, nil
}
// ...
}Also applies to: 43-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/store/transaction.go` around lines 29 - 31, The commit/rollback
functions silently return when the caller is not the transaction owner (checked
via ctx.Value(txOwnerKey).(bool)), which hampers debugging; update the Commit
and Rollback methods to log a debug-level message when the ownership check fails
(include identifying context such as a tx id or caller info if available) before
returning, using the package logger or context-aware logger so non-owner
attempts are recorded without changing behavior.
Introduce a ReBAC simple model using sql. This is a simple authz model that handlers 3 relationships types: - resource-owner-user - resource-viewer-user/org - user-member-org It allows to keep the current ownership logic and provide support for the partner feature. Signed-off-by: Cosmin Tupangiu <cosmin@redhat.com>
tupyy
force-pushed
the
authz-sql-relations
branch
from
8a77388 to
d233425
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/store/transaction.go (1)
114-114:⚠️ Potential issue | 🟡 MinorIncorrect error format verb in log statement.
Line 114 uses
%wfor error wrapping in a log statement, butzap.S().Errorfdoesn't support error wrapping - it's just formatting a string. The%wverb is only meaningful withfmt.Errorffor wrapping errors.🔧 Proposed fix
- zap.S().Named("transaction").Errorf("failed to commit transaction %d: %w", t.txId, err) + zap.S().Named("transaction").Errorf("failed to commit transaction %d: %v", t.txId, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/transaction.go` at line 114, The log call using zap.S().Named("transaction").Errorf("failed to commit transaction %d: %w", t.txId, err) incorrectly uses the fmt %w verb; change the format verb to %v or %s (e.g. "failed to commit transaction %d: %v") so the error is formatted correctly, or alternatively construct a wrapped error with fmt.Errorf and log that message; update the call that references t.txId and err accordingly.internal/handlers/v1alpha1/assessment.go (1)
136-145:⚠️ Potential issue | 🔴 CriticalDon’t remove the last ownership check unless every wiring path uses the authz wrapper.
These handlers now rely entirely on
*service.ErrForbidden, butinternal/api_server/server.goonly wrapsassessmentSrvwithNewAuthzAssessmentServicewhenAuthenticationType == "local". The rawAssessmentServicenever returnsErrForbidden, so any non-localwiring path loses assessment authorization for GET/PUT/DELETE. Either keep the legacy owner/org guard here or make the wrapper mandatory wherever authenticated users can reach these endpoints.Also applies to: 177-185, 218-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/v1alpha1/assessment.go` around lines 136 - 145, The handler currently assumes authorization is enforced by service.ErrForbidden from h.assessmentSrv.GetAssessment, but non-local wiring may not wrap the service with NewAuthzAssessmentService; restore the legacy ownership/org guard in this handler (and the similar spots for Update/Delete) instead of relying solely on ErrForbidden: after fetching assessment via h.assessmentSrv.GetAssessment (and before returning success), verify the current caller identity/organization from the request context (same logic used previously to check owner/org on the assessment) and return a 403 JSON response if the caller is not the owner/org; alternatively, if you prefer centralizing auth, make NewAuthzAssessmentService mandatory for all wiring paths so the raw AssessmentService never reaches the handlers unwrapped.
♻️ Duplicate comments (6)
internal/store/doc.go (1)
101-111:⚠️ Potential issue | 🟡 MinorFinish these examples with
WriteRelationshipsandCommit.Both snippets stop at
Build(), so a reader copying them never persists the relationship batch.Suggested fix
// updates := store.NewRelationshipBuilder(). // With(model.NewAssessmentResource(assessmentID), model.ViewerRelation, model.NewOrgSubject(partnerOrgID)). // Build() +// +// ctx, _ = s.NewTransactionContext(ctx) +// err := s.Authz().WriteRelationships(ctx, updates) +// ctx, _ = store.Commit(ctx) // // ## Adding org members // // updates := store.NewRelationshipBuilder(). // With(model.NewOrgResource(orgID), model.MemberRelation, model.NewUserSubject(userID)). // Build() +// +// ctx, _ = s.NewTransactionContext(ctx) +// err := s.Authz().WriteRelationships(ctx, updates) +// ctx, _ = store.Commit(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/doc.go` around lines 101 - 111, The examples showing how to create relationship batches stop at Build() and never persist changes; update both snippets to call the store's WriteRelationships method with the built batch and then call Commit on the returned transaction (or handle errors) so the relationships are actually written. Specifically, after model.NewRelationshipBuilder().With(...).Build() call WriteRelationships(...) (or the appropriate method on your store) with the resulting batch, then call Commit() on the returned transaction/response to persist the changes.pkg/migrations/sql/20260325120000_add_relations_table.sql (1)
12-13:⚠️ Potential issue | 🟠 MajorReplace the
(resource, relation)index with indexes that match the authz lookups.The read paths filter by resource plus subject keys, and sometimes
resource_idas well. The current index still leaves those checks doing broader scans oncerelationsgrows.Suggested fix
CREATE INDEX idx_relations_subject ON relations (subject_namespace, subject_id); -CREATE INDEX idx_relations_resource_relation ON relations (resource, relation); +CREATE INDEX idx_relations_resource_subject ON relations (resource, subject_namespace, subject_id); +CREATE INDEX idx_relations_resource_id_subject ON relations (resource, resource_id, subject_namespace, subject_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/migrations/sql/20260325120000_add_relations_table.sql` around lines 12 - 13, The existing index idx_relations_resource_relation on (resource, relation) is too narrow for authz read paths; drop that index and replace it with indexes that match lookups: create a composite index on relations (resource, subject_namespace, subject_id) to accelerate queries filtering by resource + subject, and create an index on relations (resource, resource_id) to cover queries that filter by resource_id; update migration SQL to DROP INDEX idx_relations_resource_relation and CREATE INDEXes idx_relations_resource_subject and idx_relations_resource_resourceid with those column orders.pkg/migrations/sql/20260325120001_backfill_owner_relations.sql (1)
3-6:⚠️ Potential issue | 🔴 CriticalFilter out assessments with
NULLusernames before the backfill.
relations.subject_idisNOT NULL, so a single legacy assessment withusername IS NULLwill make the wholeINSERT ... SELECTfail and block the migration.Suggested fix
INSERT INTO relations (resource, resource_id, relation, subject_namespace, subject_id) SELECT 'assessment', a.id::text, 'owner', 'user', a.username FROM assessments a +WHERE a.username IS NOT NULL ON CONFLICT ON CONSTRAINT uq_resource_relation DO NOTHING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql` around lines 3 - 6, The INSERT ... SELECT can fail if any assessment has username NULL because relations.subject_id is NOT NULL; modify the SELECT used to backfill relations (in the INSERT into relations with columns resource, resource_id, relation, subject_namespace, subject_id) to filter out rows where assessments.username IS NULL (e.g., add a WHERE a.username IS NOT NULL) so only non-null subject_id values are inserted and the ON CONFLICT uq_resource_relation still applies.internal/service/authz_assessment.go (2)
51-63:⚠️ Potential issue | 🟠 MajorPreserve not-found behavior before returning 403.
These methods authorize before delegating to the inner service. When an assessment is missing and there is no relation row,
GetPermissionsyields an empty permission set and the wrapper now returnsErrForbiddenwhere the previous flow returnedErrResourceNotFound. That is a wire-level behavior change for GET/PUT/DELETE; check existence first, or translate inner not-found results before mapping lack of access to 403.Also applies to: 97-109, 112-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/authz_assessment.go` around lines 51 - 63, The wrapper currently calls a.store.Authz().GetPermissions in AuthzAssessmentService.GetAssessment and returns NewErrForbidden when permissions are empty, which hides the upstream "not found" returned by a.inner.GetAssessment; change the flow to first call a.inner.GetAssessment (or, if you must avoid duplicate work, call a.inner to check existence and translate its ErrResourceNotFound/ErrNotFound into the same error before doing permission checks), then call a.store.Authz().GetPermissions and only return NewErrForbidden if the resource exists but the user lacks model.ReadPermission; do the same fix for the other affected methods (the other AuthzAssessmentService wrappers at the referenced ranges) so missing assessments remain ErrResourceNotFound rather than 403.
23-27:⚠️ Potential issue | 🟠 MajorUsername-only tuples regress the old org boundary.
All authz lookups and writes key users by
user.Usernamealone. The previous ownership logic compared both username and organization, so if usernames are only org-scoped, same-named users in different orgs now collapse onto the same owner/member tuple and inherit each other’s permissions. Encode org into the authz subject key (or re-check org equality) before shipping, and add a same-username/different-org regression.Also applies to: 54-55, 67-84, 100-101, 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/authz_assessment.go` around lines 23 - 27, The current authz lookups in AuthzAssessmentService.ListAssessments (and related calls around the same file: the uses at lines referenced) use only user.Username as the subject key, which collapses same usernames across orgs; update all calls to a.store.Authz().ListResources, Create/Write/Check calls in this file to include the organization (e.g. combine user.Username and user.Organization into the subject key or use a struct/namespace that encodes org) and/or explicitly verify org equality when resolving ownership (replace uses of user.Username with a composite subject like fmt.Sprintf("%s:%s", user.Organization, user.Username) or equivalent Subject{Org,Username}). Ensure all subject comparisons (owner/member checks) in ListAssessments, the create/update/delete flows (functions/methods around the noted spots), and any Authz reads/writes are changed consistently, and add a regression test that creates two users with the same username in different orgs and asserts they do not share permissions.internal/service/assessment.go (1)
58-62:⚠️ Potential issue | 🔴 CriticalPreserve explicit empty-ID semantics.
AuthzAssessmentService.ListAssessmentsnow usesfilter.IDsto carry the authorized assessment set and clearsUsername/OrgIDbefore delegating. IfIDsis non-nil but empty, this block adds no predicate and the query widens past the authorized set. Please treatfilter.IDs != nil && len(filter.IDs) == 0as “return no rows” instead of falling through.🔒 Suggested fix
func (as *AssessmentService) ListAssessments(ctx context.Context, filter *AssessmentFilter) ([]model.Assessment, error) { logger := as.logger.WithContext(ctx) tracer := logger.Operation("list_assessments"). WithString("username", filter.Username). @@ WithInt("offset", filter.Offset). Build() + if filter.IDs != nil && len(filter.IDs) == 0 { + tracer.Success().WithInt("count", 0).Log() + return []model.Assessment{}, nil + } + storeFilter := store.NewAssessmentQueryFilter() if len(filter.IDs) > 0 { storeFilter = storeFilter.WithIDs(filter.IDs) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/assessment.go` around lines 58 - 62, In AuthzAssessmentService.ListAssessments, preserve the semantics that a non-nil but empty filter.IDs means “no rows”: after creating storeFilter (store.NewAssessmentQueryFilter()) check if filter.IDs != nil && len(filter.IDs) == 0 and immediately return an empty result set (or appropriate zero-value response) instead of falling through; otherwise keep the existing branch that calls storeFilter = storeFilter.WithIDs(filter.IDs) when len(filter.IDs) > 0. This ensures filter.IDs being an explicitly empty slice does not widen the query beyond the authorized set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api_server/server.go`:
- Around line 175-179: The authorization wrapper NewAuthzAssessmentService is
only applied when s.cfg.Service.Auth.AuthenticationType == "local", which may be
surprising to future readers; add a brief comment above the block explaining why
non-local auth types skip the authorization wrapper (e.g., because they rely on
external/third-party authorization or partner-integrated auth) and reference the
involved symbols AssessmentServicer, service.NewAssessmentService and
service.NewAuthzAssessmentService so maintainers can quickly understand the
intended design and where to change it if needed.
In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql`:
- Around line 11-14: The rollback DELETE is too broad and would remove live
assessment#owner@user tuples created after this migration; narrow the WHERE
clause in the rollback of the relations DELETE (the DELETE targeting resource =
'assessment' AND relation = 'owner' AND subject_namespace = 'user') to only
remove rows that predate the migration (e.g., add created_at < '2026-03-25
12:00:01') or, if your up migration records provenance (e.g., a
provenance/migrated_by column), restrict the DELETE to only those
provenance-marked rows so you don't delete runtime-created owner relations.
---
Outside diff comments:
In `@internal/handlers/v1alpha1/assessment.go`:
- Around line 136-145: The handler currently assumes authorization is enforced
by service.ErrForbidden from h.assessmentSrv.GetAssessment, but non-local wiring
may not wrap the service with NewAuthzAssessmentService; restore the legacy
ownership/org guard in this handler (and the similar spots for Update/Delete)
instead of relying solely on ErrForbidden: after fetching assessment via
h.assessmentSrv.GetAssessment (and before returning success), verify the current
caller identity/organization from the request context (same logic used
previously to check owner/org on the assessment) and return a 403 JSON response
if the caller is not the owner/org; alternatively, if you prefer centralizing
auth, make NewAuthzAssessmentService mandatory for all wiring paths so the raw
AssessmentService never reaches the handlers unwrapped.
In `@internal/store/transaction.go`:
- Line 114: The log call using zap.S().Named("transaction").Errorf("failed to
commit transaction %d: %w", t.txId, err) incorrectly uses the fmt %w verb;
change the format verb to %v or %s (e.g. "failed to commit transaction %d: %v")
so the error is formatted correctly, or alternatively construct a wrapped error
with fmt.Errorf and log that message; update the call that references t.txId and
err accordingly.
---
Duplicate comments:
In `@internal/service/assessment.go`:
- Around line 58-62: In AuthzAssessmentService.ListAssessments, preserve the
semantics that a non-nil but empty filter.IDs means “no rows”: after creating
storeFilter (store.NewAssessmentQueryFilter()) check if filter.IDs != nil &&
len(filter.IDs) == 0 and immediately return an empty result set (or appropriate
zero-value response) instead of falling through; otherwise keep the existing
branch that calls storeFilter = storeFilter.WithIDs(filter.IDs) when
len(filter.IDs) > 0. This ensures filter.IDs being an explicitly empty slice
does not widen the query beyond the authorized set.
In `@internal/service/authz_assessment.go`:
- Around line 51-63: The wrapper currently calls a.store.Authz().GetPermissions
in AuthzAssessmentService.GetAssessment and returns NewErrForbidden when
permissions are empty, which hides the upstream "not found" returned by
a.inner.GetAssessment; change the flow to first call a.inner.GetAssessment (or,
if you must avoid duplicate work, call a.inner to check existence and translate
its ErrResourceNotFound/ErrNotFound into the same error before doing permission
checks), then call a.store.Authz().GetPermissions and only return
NewErrForbidden if the resource exists but the user lacks model.ReadPermission;
do the same fix for the other affected methods (the other AuthzAssessmentService
wrappers at the referenced ranges) so missing assessments remain
ErrResourceNotFound rather than 403.
- Around line 23-27: The current authz lookups in
AuthzAssessmentService.ListAssessments (and related calls around the same file:
the uses at lines referenced) use only user.Username as the subject key, which
collapses same usernames across orgs; update all calls to
a.store.Authz().ListResources, Create/Write/Check calls in this file to include
the organization (e.g. combine user.Username and user.Organization into the
subject key or use a struct/namespace that encodes org) and/or explicitly verify
org equality when resolving ownership (replace uses of user.Username with a
composite subject like fmt.Sprintf("%s:%s", user.Organization, user.Username) or
equivalent Subject{Org,Username}). Ensure all subject comparisons (owner/member
checks) in ListAssessments, the create/update/delete flows (functions/methods
around the noted spots), and any Authz reads/writes are changed consistently,
and add a regression test that creates two users with the same username in
different orgs and asserts they do not share permissions.
In `@internal/store/doc.go`:
- Around line 101-111: The examples showing how to create relationship batches
stop at Build() and never persist changes; update both snippets to call the
store's WriteRelationships method with the built batch and then call Commit on
the returned transaction (or handle errors) so the relationships are actually
written. Specifically, after model.NewRelationshipBuilder().With(...).Build()
call WriteRelationships(...) (or the appropriate method on your store) with the
resulting batch, then call Commit() on the returned transaction/response to
persist the changes.
In `@pkg/migrations/sql/20260325120000_add_relations_table.sql`:
- Around line 12-13: The existing index idx_relations_resource_relation on
(resource, relation) is too narrow for authz read paths; drop that index and
replace it with indexes that match lookups: create a composite index on
relations (resource, subject_namespace, subject_id) to accelerate queries
filtering by resource + subject, and create an index on relations (resource,
resource_id) to cover queries that filter by resource_id; update migration SQL
to DROP INDEX idx_relations_resource_relation and CREATE INDEXes
idx_relations_resource_subject and idx_relations_resource_resourceid with those
column orders.
In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql`:
- Around line 3-6: The INSERT ... SELECT can fail if any assessment has username
NULL because relations.subject_id is NOT NULL; modify the SELECT used to
backfill relations (in the INSERT into relations with columns resource,
resource_id, relation, subject_namespace, subject_id) to filter out rows where
assessments.username IS NULL (e.g., add a WHERE a.username IS NOT NULL) so only
non-null subject_id values are inserted and the ON CONFLICT uq_resource_relation
still applies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3ebc4f3-caeb-4603-bde0-84a3597cacf5
📒 Files selected for processing (24)
internal/api_server/server.gointernal/handlers/v1alpha1/assessment.gointernal/handlers/v1alpha1/assessment_test.gointernal/handlers/v1alpha1/estimation.gointernal/handlers/v1alpha1/estimation_test.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/handlers/v1alpha1/source.gointernal/service/assessment.gointernal/service/authz_assessment.gointernal/service/authz_assessment_test.gointernal/service/errors.gointernal/service/sizer_test.gointernal/store/authz.gointernal/store/authz_builder.gointernal/store/authz_test.gointernal/store/doc.gointernal/store/model/authz.gointernal/store/options.gointernal/store/store.gointernal/store/transaction.gopkg/migrations/migrations_test.gopkg/migrations/sql/20260325120000_add_relations_table.sqlpkg/migrations/sql/20260325120001_backfill_owner_relations.sql
Comment on lines
+175
to
+179
| var assessmentSvc service.AssessmentServicer | ||
| assessmentSvc = service.NewAssessmentService(s.store, s.opaValidator) | ||
| if s.cfg.Service.Auth.AuthenticationType == "local" { | ||
| assessmentSvc = service.NewAuthzAssessmentService(assessmentSvc, s.store) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Authorization wrapper conditionally applied based on auth type.
The AuthzAssessmentService wrapper is only applied when AuthenticationType == "local". This appears intentional for the partner feature mentioned in the PR description.
Consider documenting why authorization wrapping is skipped for non-local authentication types, or verify this is the intended design. If other auth types rely on external authorization systems, a brief comment would help future maintainers understand the distinction.
📝 Optional: Add clarifying comment
var assessmentSvc service.AssessmentServicer
assessmentSvc = service.NewAssessmentService(s.store, s.opaValidator)
if s.cfg.Service.Auth.AuthenticationType == "local" {
+ // Apply SQL-based ReBAC authorization for local auth mode.
+ // Other auth types rely on external authorization mechanisms.
assessmentSvc = service.NewAuthzAssessmentService(assessmentSvc, s.store)
}📝 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.
Suggested change
| var assessmentSvc service.AssessmentServicer | |
| assessmentSvc = service.NewAssessmentService(s.store, s.opaValidator) | |
| if s.cfg.Service.Auth.AuthenticationType == "local" { | |
| assessmentSvc = service.NewAuthzAssessmentService(assessmentSvc, s.store) | |
| } | |
| var assessmentSvc service.AssessmentServicer | |
| assessmentSvc = service.NewAssessmentService(s.store, s.opaValidator) | |
| if s.cfg.Service.Auth.AuthenticationType == "local" { | |
| // Apply SQL-based ReBAC authorization for local auth mode. | |
| // Other auth types rely on external authorization mechanisms. | |
| assessmentSvc = service.NewAuthzAssessmentService(assessmentSvc, s.store) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api_server/server.go` around lines 175 - 179, The authorization
wrapper NewAuthzAssessmentService is only applied when
s.cfg.Service.Auth.AuthenticationType == "local", which may be surprising to
future readers; add a brief comment above the block explaining why non-local
auth types skip the authorization wrapper (e.g., because they rely on
external/third-party authorization or partner-integrated auth) and reference the
involved symbols AssessmentServicer, service.NewAssessmentService and
service.NewAuthzAssessmentService so maintainers can quickly understand the
intended design and where to change it if needed.
Comment on lines
+11
to
+14
| DELETE FROM relations | ||
| WHERE resource = 'assessment' | ||
| AND relation = 'owner' | ||
| AND subject_namespace = 'user'; |
There was a problem hiding this comment.
Don’t let the rollback delete owner relations created after this migration.
The create path now writes the same assessment#owner@user:* tuples at runtime, so rolling back only this migration wipes live authz data that was never backfilled here. Please scope the delete to rows that predate the migration timestamp 2026-03-25 12:00:01, or record provenance during the up migration and delete only those rows.
Safer rollback pattern
-DELETE FROM relations
-WHERE resource = 'assessment'
- AND relation = 'owner'
- AND subject_namespace = 'user';
+DELETE FROM relations r
+USING assessments a
+WHERE r.resource = 'assessment'
+ AND r.relation = 'owner'
+ AND r.subject_namespace = 'user'
+ AND r.resource_id = a.id::text
+ AND r.subject_id = a.username
+ AND a.created_at < TIMESTAMP '2026-03-25 12:00:01';📝 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.
Suggested change
| DELETE FROM relations | |
| WHERE resource = 'assessment' | |
| AND relation = 'owner' | |
| AND subject_namespace = 'user'; | |
| DELETE FROM relations r | |
| USING assessments a | |
| WHERE r.resource = 'assessment' | |
| AND r.relation = 'owner' | |
| AND r.subject_namespace = 'user' | |
| AND r.resource_id = a.id::text | |
| AND r.subject_id = a.username | |
| AND a.created_at < TIMESTAMP '2026-03-25 12:00:01'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/migrations/sql/20260325120001_backfill_owner_relations.sql` around lines
11 - 14, The rollback DELETE is too broad and would remove live
assessment#owner@user tuples created after this migration; narrow the WHERE
clause in the rollback of the relations DELETE (the DELETE targeting resource =
'assessment' AND relation = 'owner' AND subject_namespace = 'user') to only
remove rows that predate the migration (e.g., add created_at < '2026-03-25
12:00:01') or, if your up migration records provenance (e.g., a
provenance/migrated_by column), restrict the DELETE to only those
provenance-marked rows so you don't delete runtime-created owner relations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a ReBAC simple model using sql. This is a simple authz model that handlers 3 relationships types:
It allows to keep the current ownership logic and provide support for the partner feature.
Signed-off-by: Cosmin Tupangiu cosmin@redhat.com
Summary by CodeRabbit