Feat/GitHub issue agent rbac scope gating#619
Conversation
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
…ions into feat/github-issue-agent-rbac-scope-gating
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates AIAC demo output paths and wiring, adds Keycloak RBAC policy application, introduces a new ChangesGitHub Issue Demo Overhaul
Sequence Diagram(s)sequenceDiagram
participant Makefile
participant aiac_cli.py
participant show-result.py
Makefile->>aiac_cli.py: run pipeline with config/ outputs
aiac_cli.py->>aiac_cli.py: write config_file and policy_file under config/
Makefile->>show-result.py: pass --config-path $(RBAC_CONFIG)
show-result.py->>show-result.py: read config/ for latest policy files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ 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: 13
🤖 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 `@authbridge/demos/github-issue/aiac/Makefile`:
- Line 86: The setup/reset flow is building the RBAC config path incorrectly for
the default workflow, causing the policy file to resolve under the aiac
directory instead of the shared rbac directory. Update the Makefile targets that
invoke setup_keycloak.py so they pass the RBAC path relative to the Makefile’s
expected working directory, using the RBAC_CONFIG variable in a way that
resolves to the shared config for both setup and reset. Keep the fix localized
to the relevant Makefile recipe lines that reference setup_keycloak.py and the
RBAC_CONFIG setting.
- Around line 34-39: The help text for RBAC_CONFIG is inconsistent with the
actual Makefile default, so update the help output in the Makefile’s
usage/printf section to match the RBAC_CONFIG variable value. Keep the variable
definition and the displayed default aligned in the same place, using the
RBAC_CONFIG symbol so users see the correct path when running the demo help
target.
In `@authbridge/demos/github-issue/aiac/scripts/show-result.py`:
- Around line 50-56: The CLI option parsing in show-result.py leaves
args.config_path optional, which later causes a NoneType crash when the script
checks it. Update the argparse setup in the main flow around parser.add_argument
for --config-path, and either mark it as required or provide a valid default
before the code that calls config_path.exists(), so the script fails with a
proper CLI usage error instead of an attribute error.
- Line 101: The generated policy lookup in show-result.py is pointing at the
wrong directory, so it no longer matches where aiac_cli.py writes policy files.
Update the logic around gen_dir in the show-result script to scan the same
config location used by the writer in aiac_cli.py so the latest *_config.yaml
and *_policy.yaml files are found after apply-policy. Keep the existing
selection and “no generated policy files” handling, but make sure the path
resolution aligns with the output location used by the policy generation flow.
In `@authbridge/demos/github-issue/demo-aiac.md`:
- Around line 841-847: The reset step uses a repo path that is inconsistent with
the rest of the demo instructions, so update the Step 17 commands to use the
same authenticated repo-relative path as the earlier steps. Make the directory
change in the reset sequence match the `authbridge/demos/github-issue` location
used elsewhere in `demo-aiac.md`, and keep the cleanup and `setup_keycloak.py`
reprovision commands under that corrected path so they run from the intended
directory.
In `@authbridge/demos/github-issue/demo-rbac.md`:
- Around line 259-263: The environment setup instructions in the demo guide are
using a different virtualenv path than the Makefile/preflight flow. Update the
setup steps in the GitHub issue demo so the manual activation and pip commands
use the same `.venv` location expected by the Makefile and `make
setup-keycloak`, and keep the instructions aligned with the virtual environment
path used elsewhere in the demo.
- Around line 265-267: The Keycloak setup command in the RBAC demo still
references the old aiac/config/rbac paths, which no longer match the new demo
layout. Update the setup instruction to use the new rbac/config.yaml and
rbac/policy.yaml locations so it aligns with the demo’s Makefile and file
structure. Make sure the command shown in the demo guide matches the current
setup used by the rbac demo flow.
In `@authbridge/demos/github-issue/rbac/Makefile`:
- Around line 75-76: The preflight check in the Makefile is too permissive
because it accepts podman even though the build flow later uses docker directly
in the image build and kind load steps. Update the preflight logic and the
related build-image workflow so they use a consistent, configurable container
runtime via the existing Makefile targets/variables, or restrict preflight to
require docker if that path remains hardcoded; make sure the same runtime is
used by the build and load commands.
- Around line 22-23: The Makefile advertises a test target that does not exist,
so users will fail when running the documented command. In the Makefile, add a
`test: test-ab` alias near the existing `test-ab` target (or update the help
text to reference the real target) and make sure `.PHONY` and any help output
stay consistent so `make test` works as documented.
In `@authbridge/demos/github-issue/rbac/policy.yaml`:
- Around line 21-35: The RBAC policy hard-codes the
`spiffe://localtest.me/ns/team1/sa/git-issue-agent` client, which breaks
`apply_access_control_policy()` in non-team1 namespaces. Update the
`policy.yaml` mapping so the `github-agent` composite client is derived from the
selected namespace (matching the new `NAMESPACE` support), or explicitly
constrain the demo to team1-only and document that limitation consistently. Use
the existing `github-agent` and `github-tool` entries as the reference points
when making the policy namespace-aware.
In `@authbridge/demos/github-issue/setup_keycloak.py`:
- Around line 1363-1367: The composite-role handling in the setup flow is
masking real failures by catching every Exception and logging it as “already in
composite.” Update the try/except around
_add_client_role_to_realm_role_composite in setup_keycloak.py so only the known
idempotent duplicate-composite case is suppressed, and let missing roles,
Keycloak API errors, and other provisioning failures propagate or be re-raised
with context. Keep the success path unchanged, but narrow the exception handling
to the specific composite-write case rather than treating all errors as
harmless.
- Around line 1289-1297: The policy loading in setup_keycloak.py needs an
upfront validation of the YAML root shape before calling policy_config.get(). In
the code path that reads access_control_policy_file and parses it with
yaml.safe_load, explicitly handle None and any non-mapping result by raising the
existing policy-format error instead of letting AttributeError occur. Keep the
fix localized around the policy_config and policy extraction logic so the later
policy.items() loop only runs when the top-level document is a valid mapping.
- Around line 84-88: Update the typing in setup_keycloak.py to follow the Python
3.12 annotation style: add explicit return types to main_manual and main_rbac,
and replace any remaining Optional[...] parameter annotations such as scope_ids
and policy_file with the PEP 604 union form using | None. Use the existing
function names main_manual and main_rbac as the targets so the signatures match
the repo’s typing convention.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40a68e6f-33db-466d-a330-5203c66c34c4
📒 Files selected for processing (10)
authbridge/demos/github-issue/aiac/.gitignoreauthbridge/demos/github-issue/aiac/Makefileauthbridge/demos/github-issue/aiac/aiac_cli.pyauthbridge/demos/github-issue/aiac/scripts/show-result.pyauthbridge/demos/github-issue/demo-aiac.mdauthbridge/demos/github-issue/demo-rbac.mdauthbridge/demos/github-issue/rbac/Makefileauthbridge/demos/github-issue/rbac/config.yamlauthbridge/demos/github-issue/rbac/policy.yamlauthbridge/demos/github-issue/setup_keycloak.py
| ```bash | ||
| cd demos/github-issue/ | ||
| # delete generated policies | ||
| rm -f generated_configs/*.yaml | ||
| rm -f aiac/config/*.yaml | ||
|
|
||
| # re-provision the realm | ||
| python setup_keycloak.py -rbac config.yaml | ||
| python setup_keycloak.py -rbac rbac/config.yaml |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset instructions use an inconsistent repo path.
Earlier steps use authbridge/demos/github-issue/..., but Step 17 switches to cd demos/github-issue/. From repo root that likely lands in the wrong directory, so the cleanup and reprovision commands won't work as written.
🤖 Prompt for 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.
In `@authbridge/demos/github-issue/demo-aiac.md` around lines 841 - 847, The reset
step uses a repo path that is inconsistent with the rest of the demo
instructions, so update the Step 17 commands to use the same authenticated
repo-relative path as the earlier steps. Make the directory change in the reset
sequence match the `authbridge/demos/github-issue` location used elsewhere in
`demo-aiac.md`, and keep the cleanup and `setup_keycloak.py` reprovision
commands under that corrected path so they run from the intended directory.
| @command -v docker >/dev/null 2>&1 || command -v podman >/dev/null 2>&1 || { \ | ||
| echo "ERROR: docker or podman not found."; exit 1; \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preflight shouldn't accept Podman if the build path requires Docker.
preflight passes when only podman is installed, but build-images and kind load docker-image unconditionally invoke docker, so the documented workflow still fails later. Either require docker here or route the image commands through a configurable container runtime.
Also applies to: 104-115
🤖 Prompt for 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.
In `@authbridge/demos/github-issue/rbac/Makefile` around lines 75 - 76, The
preflight check in the Makefile is too permissive because it accepts podman even
though the build flow later uses docker directly in the image build and kind
load steps. Update the preflight logic and the related build-image workflow so
they use a consistent, configurable container runtime via the existing Makefile
targets/variables, or restrict preflight to require docker if that path remains
hardcoded; make sure the same runtime is used by the build and load commands.
| - client: kagenti | ||
| role: demo-ui | ||
| - client: spiffe://localtest.me/ns/team1/sa/git-issue-agent | ||
| role: github-agent | ||
| - client: github-tool | ||
| role: github-full-access | ||
| - client: github-tool | ||
| role: github-tool-aud | ||
| tech-support: | ||
| - client: kagenti | ||
| role: demo-ui | ||
| - client: spiffe://localtest.me/ns/team1/sa/git-issue-agent | ||
| role: github-agent | ||
| - client: github-tool | ||
| role: github-tool-aud |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
This policy hard-codes the team1 agent client.
The new Makefile exposes NAMESPACE, and the guide says another namespace is supported, but these mappings only match spiffe://localtest.me/ns/team1/.... In any other namespace apply_access_control_policy() will skip the github-agent composite with “Client not found”, so users lose access before the agent hop. Either template this SPIFFE client name from the selected namespace or document the demo as team1-only.
🤖 Prompt for 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.
In `@authbridge/demos/github-issue/rbac/policy.yaml` around lines 21 - 35, The
RBAC policy hard-codes the `spiffe://localtest.me/ns/team1/sa/git-issue-agent`
client, which breaks `apply_access_control_policy()` in non-team1 namespaces.
Update the `policy.yaml` mapping so the `github-agent` composite client is
derived from the selected namespace (matching the new `NAMESPACE` support), or
explicitly constrain the demo to team1-only and document that limitation
consistently. Use the existing `github-agent` and `github-tool` entries as the
reference points when making the policy namespace-aware.
| import json | ||
| import os | ||
| import sys | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Tuple | ||
| from typing import Any, Dict, List, Optional, Tuple |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'Optional\[|def main_manual\(|def main_rbac\(' authbridge/demos/github-issue/setup_keycloak.pyRepository: kagenti/kagenti-extensions
Length of output: 421
🏁 Script executed:
sed -n '240,270p' authbridge/demos/github-issue/setup_keycloak.py
printf '\n---\n'
sed -n '1320,1390p' authbridge/demos/github-issue/setup_keycloak.py
printf '\n---\n'
python3 - <<'PY'
from pathlib import Path
text = Path('authbridge/demos/github-issue/setup_keycloak.py').read_text()
for needle in ['Optional[', 'def main_manual(', 'def main_rbac(']:
print(f'--- {needle} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: kagenti/kagenti-extensions
Length of output: 4603
🏁 Script executed:
sed -n '240,270p' authbridge/demos/github-issue/setup_keycloak.py
printf '\n---\n'
sed -n '1320,1390p' authbridge/demos/github-issue/setup_keycloak.py
printf '\n---\n'
python3 - <<'PY'
from pathlib import Path
text = Path('authbridge/demos/github-issue/setup_keycloak.py').read_text()
for needle in ['Optional[', 'def main_manual(', 'def main_rbac(']:
print(f'--- {needle} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: kagenti/kagenti-extensions
Length of output: 4603
Use Python 3.12 annotations here
main_manual and main_rbac still omit -> None, and the new parameters still use Optional[...] (scope_ids, policy_file). Switch these signatures to | None and add explicit return types to match the repo’s Python 3.12 typing rule.
🤖 Prompt for 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.
In `@authbridge/demos/github-issue/setup_keycloak.py` around lines 84 - 88, Update
the typing in setup_keycloak.py to follow the Python 3.12 annotation style: add
explicit return types to main_manual and main_rbac, and replace any remaining
Optional[...] parameter annotations such as scope_ids and policy_file with the
PEP 604 union form using | None. Use the existing function names main_manual and
main_rbac as the targets so the signatures match the repo’s typing convention.
Source: Coding guidelines
| with open(access_control_policy_file, "r") as f: | ||
| policy_config = yaml.safe_load(f) | ||
|
|
||
| policy = policy_config.get("policy", {}) | ||
|
|
||
| if policy is None: | ||
| policy = {} | ||
|
|
||
| for user_role, client_roles in policy.items(): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the top-level YAML shape before calling .get().
yaml.safe_load() returns None for an empty file, and a non-mapping document will also bypass the intended validation path. Right now those cases raise AttributeError instead of a clear policy-format error.
🤖 Prompt for 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.
In `@authbridge/demos/github-issue/setup_keycloak.py` around lines 1289 - 1297,
The policy loading in setup_keycloak.py needs an upfront validation of the YAML
root shape before calling policy_config.get(). In the code path that reads
access_control_policy_file and parses it with yaml.safe_load, explicitly handle
None and any non-mapping result by raising the existing policy-format error
instead of letting AttributeError occur. Keep the fix localized around the
policy_config and policy extraction logic so the later policy.items() loop only
runs when the top-level document is a valid mapping.
…rabbitai review Signed-off-by: Omer Boehm <omerboehm@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
authbridge/demos/github-issue/demo-rbac.md (1)
284-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlice/Bob access is documented backwards here.
The PR objective says Alice should not get
github-full-accessand Bob should, but this table describes the opposite. Readers following the demo will validate the wrong RBAC outcome.🤖 Prompt for 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. In `@authbridge/demos/github-issue/demo-rbac.md` around lines 284 - 285, The RBAC demo table documents Alice and Bob with the wrong access outcome, which contradicts the PR intent. Update the entries in the demo-rbac.md access matrix so the descriptions for the User rows match the intended roles and permissions, and make sure the labels in the table align with the demo’s expected `github-full-access` behavior for Alice and Bob.
♻️ Duplicate comments (2)
authbridge/demos/github-issue/setup_keycloak.py (2)
1332-1335:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Python 3.12 union syntax in this signature.
Line 1334 still uses
Optional[...]; this repo’s Python rule requiresdict[str, str] | Noneinstead.🤖 Prompt for 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. In `@authbridge/demos/github-issue/setup_keycloak.py` around lines 1332 - 1335, The function signature still uses the older Optional[...] style for scope_ids; update the setup_keycloak helper to follow the repo’s Python 3.12 typing convention by replacing Optional[Dict[str, str]] with dict[str, str] | None while keeping the existing parameter name and default behavior unchanged.Source: Coding guidelines
1369-1371: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the ambiguous
ℹcharacter before Ruff blocks this file.
authbridge/**/*.pyis linted with Ruff, and Line 1370 is already flagged withRUF001. Swapping this for plain ASCII keeps the log readable and avoids pre-commit noise.Suggested fix
- f" ℹ Client role '{client_name}.{role_name}' already in composite for realm role '{user_role}'" + f" i Client role '{client_name}.{role_name}' already in composite for realm role '{user_role}'"🤖 Prompt for 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. In `@authbridge/demos/github-issue/setup_keycloak.py` around lines 1369 - 1371, The status message in setup_keycloak.py uses the ambiguous non-ASCII info symbol in the print call, which triggers Ruff’s RUF001 check. Update that print string to use plain ASCII wording only while keeping the same client/role context, so the log remains readable and the file passes linting.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Outside diff comments:
In `@authbridge/demos/github-issue/demo-rbac.md`:
- Around line 284-285: The RBAC demo table documents Alice and Bob with the
wrong access outcome, which contradicts the PR intent. Update the entries in the
demo-rbac.md access matrix so the descriptions for the User rows match the
intended roles and permissions, and make sure the labels in the table align with
the demo’s expected `github-full-access` behavior for Alice and Bob.
---
Duplicate comments:
In `@authbridge/demos/github-issue/setup_keycloak.py`:
- Around line 1332-1335: The function signature still uses the older
Optional[...] style for scope_ids; update the setup_keycloak helper to follow
the repo’s Python 3.12 typing convention by replacing Optional[Dict[str, str]]
with dict[str, str] | None while keeping the existing parameter name and default
behavior unchanged.
- Around line 1369-1371: The status message in setup_keycloak.py uses the
ambiguous non-ASCII info symbol in the print call, which triggers Ruff’s RUF001
check. Update that print string to use plain ASCII wording only while keeping
the same client/role context, so the log remains readable and the file passes
linting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b9e4f32-cdf0-4e62-a47f-b91bd1fd05ba
📒 Files selected for processing (4)
authbridge/demos/github-issue/aiac/scripts/show-result.pyauthbridge/demos/github-issue/demo-rbac.mdauthbridge/demos/github-issue/rbac/Makefileauthbridge/demos/github-issue/setup_keycloak.py
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Review notes (comments only — not an approval)
Solid RBAC-scope-gating demo extension. I read the security-critical core (setup_keycloak.py) closely and it's well done:
load_access_control_policyvalidates input thoroughly (list/dict types, requiredclient/rolekeys, string checks) with clear errors.apply_access_control_policyis idempotent (409 Conflict → skip) with correct error discipline — it re-raises missing-role lookups rather than swallowing them.- The argparse refactor cleanly replaces the hand-rolled
sys.argv.index("-rbac")parsing while preserving manual mode. - Policy model is coherent least-privilege:
developer→github-full-access+github-tool-aud;tech-support→github-tool-audonly, matching the call chainkagenti → git-issue-agent → github-tool.
Verified independently:
config.yamlis a clean 100%-similarity rename fromaiac/→rbac/— the+0/-0diff stat is just the rename, not an empty file. Content (clients/roles/realm-roles/users) is intact and matches the PR description.- Demo passwords (
alice123,admin, etc.) are intentional throwaway demo creds in a guide — not real secrets. - All 20 CI checks pass (CodeQL, Bandit, action pinning, shell/YAML lint, Go CI, Python tests).
- On CodeRabbit's aiac-Makefile "incorrect path" flag: I traced it —
aiac/$(RBAC_CONFIG)→aiac/../rbac/config.yamlnormalizes torbac/config.yaml, so it does resolve. The functional concern is a false alarm; the help-text mismatch below is the real one.
Two non-blocking notes inline. Leaving this as comments rather than an approval — deferring the approve decision to a human maintainer.
Areas reviewed: Python, Shell/Make, YAML, Docs, Security, Commits
Author: omerboehm (MEMBER)
Commits: 7, signed-off (DCO ✓; one merge commit unsigned but DCO-exempt)
CI: all passing (20 checks)
| $(MAKEFILE_LIST) | ||
| @printf "\nVariables:\n" | ||
| @printf " \033[36m%-20s\033[0m %s\n" "POLICY" "policy file to apply (default: policies/regular_policy.txt)" | ||
| @printf " \033[36m%-20s\033[0m %s\n" "RBAC_CONFIG" "RBAC config file (default: config/rbac/rbac_config.yaml)" |
There was a problem hiding this comment.
suggestion — This help text advertises the RBAC_CONFIG default as config/rbac/rbac_config.yaml, but the actual default (line 39) is ../rbac/config.yaml. Since this is a demo meant to be followed step-by-step, the mismatch will mislead anyone reading make help. Please align the help string with the real default.
| @printf "\nPrerequisites: uv, Keycloak running, aiac.env and llm_conf.yaml configured.\n\n" | ||
|
|
||
| POLICY ?= policies/regular_policy.txt | ||
| RBAC_CONFIG ?= ../rbac/config.yaml |
There was a problem hiding this comment.
nit — RBAC_CONFIG ?= ../rbac/config.yaml combined with the aiac/$(RBAC_CONFIG) invocation produces aiac/../rbac/config.yaml, which does resolve correctly to rbac/config.yaml (verified) — but the aiac/.. round-trip is fragile and confusing: main_rbac joins script_dir to a path that then escapes back out of aiac/. The sibling rbac/Makefile uses the cleaner RBAC_CONFIG ?= rbac/config.yaml invoked as -rbac $(RBAC_CONFIG) directly — consider matching that pattern here so the two Makefiles construct the path the same way.
mrsabath review comments Signed-off-by: Omer Boehm <omerboehm@gmail.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
LGTM — solid RBAC demo extension.
Core Python logic has thorough input validation, proper idempotent handling (409 → skip, other Keycloak errors → re-raise with context), and clean separation between manual and policy-driven modes. Two minor suggestions inline, neither blocking.
Areas reviewed: Python, Shell/Make, YAML, Docs, Security
Commits: 8, all signed-off (DCO ✓)
CI: all 20 checks passing
| policy = policy_config.get("policy", {}) | ||
|
|
||
| if policy is None: | ||
| policy = {} |
There was a problem hiding this comment.
suggestion: If the YAML file is empty, yaml.safe_load(f) returns None, and the next line (policy_config.get(...)) raises AttributeError. Consider guarding:
policy_config = yaml.safe_load(f)
if not isinstance(policy_config, dict):
raise ValueError(f"Policy file must be a YAML mapping, got: {type(policy_config).__name__}")This also catches the edge case where someone accidentally writes a YAML list at the top level.
| policy: | ||
| developer: | ||
| - client: kagenti | ||
| role: demo-ui |
There was a problem hiding this comment.
nit: This spiffe://localtest.me/ns/team1/sa/git-issue-agent client ID is namespace-coupled. The demo docs mention "use a different namespace" but this file would silently break if someone does. Consider adding a comment here like # NOTE: update ns/team1 if deploying to a different namespace.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
5 participants
Summary
Extends the GitHub Issue Agent demo with role-based scope gating, demonstrating how RBAC can be enforced across the full agent-to-tool call chain using OAuth2 token scopes via Keycloak and AuthBridge.
setup_keycloak.py— Added optional-policyflag; the RBAC mode now loads a declarativepolicy.yamland applies composite role mappings (realm roles → client roles → scopes), so each user's token carries only the scopes their role permits.rbac/config.yaml— Declares clients (kagenti,git-issue-agent,github-tool), their roles, realm roles (developer,tech-support,sales), audience targets, and demo users (alice,bob,charlie).rbac/policy.yaml— Access control policy mapping realm roles to client roles; e.g.developergetsgithub-full-access(private repos) whiletech-supportgets onlygithub-tool-aud(public repos only).demo-rbac.md— New end-to-end guide covering: Keycloak setup, agent/tool deployment, inbound JWT validation tests, Alice vs. Bob scope-differentiated access walkthrough, and full cleanup.aiac/+demo-aiac.md— Adjusted AI-based access control (AIAC) demo: a LangGraph agent that converts natural language policy descriptions intopolicy.yamland applies them to Keycloak automatically.The scope-gating mechanism works as follows: each client scope is mapped to a Keycloak client role; when AuthBridge performs RFC 8693 token exchange on the agent-to-tool hop, only scopes the user's realm role grants are included. The GitHub tool inspects
REQUIRED_SCOPE(github-full-access) to decide whether to use the privileged PAT or the public-only PAT.Related issue(s)
This is an alternative approach to the problem described in #139
(closed as not-planned). Instead of implementing SCOPE_FORWARDING in
the go-processor, scope decisions are centralized in the IdP via
Keycloak roles and client-scope bindings. See the discussion on the
issue for the trade-off between the two approaches.
Fixes #139
Testing Instructions
Follow authbridge/demos/github-issue/demo-manual.md
Summary by CodeRabbit
config/location..gitignoreto ignore the correct generated directory.