Skip to content

fix: merge RBAC verbs in createRBACMap instead of overwriting#2977

Open
joonas wants to merge 2 commits intodefenseunicorns:mainfrom
joonas:fix/rbac-map-merge-verbs
Open

fix: merge RBAC verbs in createRBACMap instead of overwriting#2977
joonas wants to merge 2 commits intodefenseunicorns:mainfrom
joonas:fix/rbac-map-merge-verbs

Conversation

@joonas
Copy link
Copy Markdown
Member

@joonas joonas commented Mar 1, 2026

Description

createRBACMap in src/lib/helpers.ts had two problems that caused RBAC verb loss when multiple bindings targeted the same resource kind:

  1. The isFinalize branch unconditionally replaced the entire RBAC entry (acc[key] = { verbs: ["patch"], ... }), discarding any "watch" verb that a prior isWatch binding had set.

  2. The isWatch branch was guarded by if (!acc[key] && ...), so it only registered "watch" for the first binding per resource key. If a finalize binding created the entry first, the subsequent watch binding was silently skipped — again losing the "watch" verb.

Both branches now check whether the key already exists and merge the new verb into the existing verbs array (with an includes guard to prevent duplicates) rather than overwriting the entry.

Four test changes cover the fix across two levels:

  • helpers.test.ts: Two unit tests for createRBACMap — watch-then-finalize and finalize-then-watch binding order — each using sorted equality (toSorted().toEqual(["patch", "watch"])) to assert exact verb content without duplicates.

  • rbac.test.ts: One integration test for clusterRole in scoped mode using a new capabilityWithWatchAndFinalize fixture from defaultTestObjects.ts. Asserts a single configmaps rule with exactly ["patch", "watch"] after deduplication.

  • rbac.test.ts: Updated the existing finalize-only test expectation — the peprstores entry now correctly retains all four verbs (["patch", "create", "get", "watch"]) from the merge of custom RBAC and the hardcoded peprstore entry, rather than the prior ["patch"] which was an artifact of the overwrite bug.

End to End Test:
(See Pepr Excellent Examples)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

`createRBACMap` in `src/lib/helpers.ts` had two problems that caused
RBAC verb loss when multiple bindings targeted the same resource
kind:

1. The `isFinalize` branch unconditionally replaced the entire RBAC
   entry (`acc[key] = { verbs: ["patch"], ... }`), discarding any
   `"watch"` verb that a prior `isWatch` binding had set.

2. The `isWatch` branch was guarded by `if (!acc[key] && ...)`, so
   it only registered `"watch"` for the first binding per resource
   key. If a finalize binding created the entry first, the
   subsequent watch binding was silently skipped — again losing
   the `"watch"` verb.

Both branches now check whether the key already exists and merge
the new verb into the existing `verbs` array (with an `includes`
guard to prevent duplicates) rather than overwriting the entry.

Four test changes cover the fix across two levels:

- **`helpers.test.ts`**: Two unit tests for `createRBACMap` —
  watch-then-finalize and finalize-then-watch binding order — each
  using sorted equality (`toSorted().toEqual(["patch", "watch"])`)
  to assert exact verb content without duplicates.

- **`rbac.test.ts`**: One integration test for `clusterRole` in
  scoped mode using a new `capabilityWithWatchAndFinalize` fixture
  from `defaultTestObjects.ts`. Asserts a single configmaps rule
  with exactly `["patch", "watch"]` after deduplication.

- **`rbac.test.ts`**: Updated the existing finalize-only test
  expectation — the peprstores entry now correctly retains all four
  verbs (`["patch", "create", "get", "watch"]`) from the merge of
  custom RBAC and the hardcoded peprstore entry, rather than the
  prior `["patch"]` which was an artifact of the overwrite bug.

Signed-off-by: Joonas Bergius <joonas@defenseunicorns.com>
@joonas joonas requested a review from a team as a code owner March 1, 2026 03:33
@samayer12
Copy link
Copy Markdown
Contributor

@greptileai

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 22, 2026

Greptile Summary

This PR fixes two verb-loss bugs in createRBACMap (src/lib/helpers.ts) where isFinalize unconditionally overwrote the RBAC entry and isWatch was silently skipped if a finalize binding had already created the entry for the same key. Both branches now merge into existing entries with a duplicate guard instead of replacing them. The fix is covered by two new unit tests (both binding orders) and one new integration test in scoped mode, plus a corrected expectation for the existing finalize-only test that was masking the bug.

Confidence Score: 5/5

Safe to merge — targeted bug fix with thorough test coverage across both binding orders and integration levels.

All findings are P2 or lower. The logic change is minimal and correct, tests cover both failure modes explicitly, and the updated expectation on the existing test confirms the prior behaviour was an artifact of the bug rather than intentional.

No files require special attention.

Important Files Changed

Filename Overview
src/lib/helpers.ts Core bug fix: createRBACMap now merges verbs instead of overwriting RBAC entries when multiple bindings share the same resource key
src/lib/helpers.test.ts Two new unit tests covering watch-then-finalize and finalize-then-watch binding orders; uses toSorted() for order-independent assertion
src/lib/assets/rbac.test.ts Integration test updated to reflect correct merged verbs on peprstores; new scoped-mode test for configmaps with both watch and finalize bindings
src/lib/assets/defaultTestObjects.ts New capabilityWithWatchAndFinalize fixture added to drive the new integration test

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[forEach binding] --> B{isWatch?}
    B -- yes --> C{acc-key exists?}
    C -- no --> D[create entry with watch]
    C -- yes --> E{has watch verb?}
    E -- no --> F[push watch]
    E -- yes --> G[skip]
    B -- no --> H{isFinalize?}
    H -- yes --> I{acc-key exists?}
    I -- no --> J[create entry with patch]
    I -- yes --> K{has patch verb?}
    K -- no --> L[push patch]
    K -- yes --> M[skip]
    H -- no --> N[no-op]
Loading

Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/rbac-map-me..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants