OSAC-58: filter unpublished catalog items from public API - #595
openshift-merge-bot[bot] merged 9 commits into
Conversation
|
@tzvatot: This pull request references OSAC-58 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds CEL syntax validation and an addPublishedFilter helper that composes user-provided CEL filters with ChangesCatalog Item Published Visibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Security ConsiderationsRisk Severity: Low | Impact: Access Control Hardening
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/servers/catalog_item_validation.go`:
- Around line 43-48: The current addPublishedFilter function naively
concatenates strings which lets a user-supplied filter inject/reshape CEL logic
and bypass the appended "this.published == true"; fix by moving from string
concatenation to AST-level composition: parse the user filter into a CEL AST
(use the same CEL parser/AST utilities used by
internal/database/dao/filter_translator.go), construct a literal AST node for
this.published == true, and combine them with a new logical AND AST node so the
published predicate is always enforced; if AST composition is not possible right
away, implement strict validation of the parsed user AST against an allow-list
of permitted node kinds/fields/operators before accepting the filter to prevent
grouping/injection attacks.
In `@internal/servers/compute_instance_catalog_items_server_test.go`:
- Around line 254-260: In the "Get returns not found for unpublished object"
test, explicitly set Published: false on the ComputeInstanceCatalogItem builder
so the created item is unambiguously unpublished; update the call that builds
the object (publicv1.ComputeInstanceCatalogItem_builder) used in
server.Create(...) to include Published: false before calling Build() so the
test doesn't rely on implicit defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: aab46044-5921-4ee7-ade9-cb68e64e713f
📒 Files selected for processing (6)
internal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/cluster_catalog_items_server.gointernal/servers/cluster_catalog_items_server_test.gointernal/servers/compute_instance_catalog_items_server.gointernal/servers/compute_instance_catalog_items_server_test.go
|
This means that tenant admins (which don't have access to the private API) will not be able to see the unpublished catalog items that they created: they will have to take note in a piece of paper of the identifier, because they will no longer be able to see them after changing the |
Not the intent. Unpublished items should be visible to the user who created them. Fixed by using creator-based visibility: the published filter is now this.published || this.metadata.creator == "<current_user>", so creators can see and manage their own unpublished items through the public API. The Get endpoint applies the same logic. Global catalog items (tenant="shared") are managed via the private API, so this does not affect them. |
The public List and Get endpoints for both cluster and compute instance catalog items now enforce published visibility. List injects a CEL filter (this.published == true) and Get returns NotFound for unpublished items. Private API endpoints remain unaffected. Generated with [Claude Code](https://claude.com/claude-code)
- Add table-driven unit test for addPublishedFilter (both branches) - Add tests for List with user filter combined with unpublished items - Use explicit Published: false in unpublished test cases Generated with [Claude Code](https://claude.com/claude-code)
Match the cluster catalog item test by explicitly setting Published: false instead of relying on proto zero-value default. Generated with [Claude Code](https://claude.com/claude-code)
- Simplify filter from `this.published == true` to `this.published` - Move addPublishedFilter from package-level function to methods on ClusterCatalogItemsServer and ComputeInstanceCatalogItemsServer to avoid naming clashes in the servers package - Fix CEL filter bypass: validate user filter is a syntactically valid CEL expression before composing, preventing injection like `true) || (true` from breaking out of parenthesized composition Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
- Use sync.Once to initialize CEL env once instead of per-call - Add test case for valid filter with OR to confirm safe composition Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
Published filter now uses creator-based visibility: unpublished items are visible to the user who created them via both List and Get. List filter: (this.published || this.metadata.creator == '<user>') Get: allow access if published or caller is the creator. This ensures tenant admins who create catalog items through the public API retain visibility after setting published to false. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
d1fd45d to
035ed85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/servers/catalog_item_validation.go`:
- Around line 60-63: buildPublishedClause currently injects raw
auth.SubjectFromContext(ctx).User into a CEL single-quoted literal, allowing a
username with quotes/backslashes to break the predicate; fix by escaping
backslashes and single quotes before embedding. In buildPublishedClause, compute
an escaped username (escape "\" -> "\\" first, then "'" -> "\'") and use that
escaped value in the fmt.Sprintf for the metadata.creator comparison; ensure
strings is imported and leave the rest of addPublishedFilter unchanged so the
constructed publishedClause is safe to concatenate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4acb44c7-f11f-417c-b9f1-6fdab79d73e3
📒 Files selected for processing (6)
internal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/cluster_catalog_items_server.gointernal/servers/cluster_catalog_items_server_test.gointernal/servers/compute_instance_catalog_items_server.gointernal/servers/compute_instance_catalog_items_server_test.go
…x tests Revert creator-based unpublished item visibility per reviewer feedback: unpublished items should be managed through the upcoming authorization and projects system instead of CEL filter composition. The published filter is now the simple `this.published` for all users. Also add direct unit tests for validateCELSyntax and verify malformed filter tests return InvalidArgument status code. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
|
does this change includes this exception: ? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/servers/cluster_catalog_items_server.go`:
- Around line 270-278: The addPublishedFilter function duplicates
published-filter composition logic across catalog servers (e.g.,
ClusterCatalogItemsServer.addPublishedFilter and
ComputeInstanceCatalogItemsServer.addPublishedFilter); extract this logic into a
single shared helper (e.g., PublishFilterForCEL or ComposePublishedFilter) and
have both servers call that helper instead of keeping local copies; the helper
should accept the incoming filter string, call validateCELSyntax(filter) and
return either the default "this.published" or "(" + filter + ") &&
this.published" and propagate the same grpcstatus error on invalid syntax so
behavior remains identical across services.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b5768071-4787-49c0-950d-63997c136938
📒 Files selected for processing (6)
internal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/cluster_catalog_items_server.gointernal/servers/cluster_catalog_items_server_test.gointernal/servers/compute_instance_catalog_items_server.gointernal/servers/compute_instance_catalog_items_server_test.go
💤 Files with no reviewable changes (1)
- internal/servers/catalog_item_validation.go
… resource Per the enhancement proposal, a user can always Get a catalog item referenced by one of their existing clusters or compute instances, even if that item is unpublished. This ensures users retain access to catalog item details after an admin unpublishes it. The check uses a tenant-scoped DAO query so users can only see references from their own visible resources. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
… resource Per the enhancement proposal, a user can always Get a catalog item referenced by one of their existing clusters or compute instances, even if that item is unpublished. Introduce catalogItemReferenceChecker interface with a DAO-backed implementation (daoReferenceChecker) shared by both cluster and compute instance catalog item servers. The DAO query is tenant-scoped so users can only see references from their own visible resources. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
Yes, this is now implemented. A user can Get an unpublished catalog item if they have an existing cluster or compute instance that references it. The check uses a tenant-scoped query so it only considers resources visible to the caller. The List endpoint still filters unpublished items as before - only direct Get by ID has this exception, matching the enhancement proposal. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, tzvatot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
4 participants
Summary
Listendpoints for cluster and compute instance catalog items now inject athis.published == trueCEL filter, excluding unpublished items from resultsGetendpoints returnNotFoundfor unpublished catalog itemsChanges
catalog_item_validation.go: addaddPublishedFilterhelper that appends the published CEL clause to user-provided filterscluster_catalog_items_server.go: apply filter inList, add published check inGetcompute_instance_catalog_items_server.go: same changescatalog_item_validation_test.go: table-driven tests foraddPublishedFilterPublished: trueexplicitlyTest plan
addPublishedFiltertable-driven tests cover empty, simple, and compound filter inputstest_unpublished_catalog_item_not_visible_in_public_api(blocked on deployment)Summary by CodeRabbit
New Features
Behavior Changes
Tests