Repository navigation
feat(OMN-15877): add OmniWeb principal identity claim - #2726
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe omniweb Keycloak client maps the ChangesOmniweb token claims
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
scripts/tests/test_keycloak_desired_clients_contract.py (1)
36-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover all token-placement invariants in the contract test.
docker/keycloak/desired-clients.jsonsetsprincipal_idfor ID, access, and userinfo tokens at Lines 59-61. It excludesgateway-attachfrom ID tokens at Line 79. This test checks only the access-token flags at Line 40 and Line 45. A regression in the other placement flags would pass the test.Suggested assertions
assert principal["config"]["access.token.claim"] == "true" + assert principal["config"]["id.token.claim"] == "true" + assert principal["config"]["userinfo.token.claim"] == "true" audience = mappers["gateway-attach-audience"] assert audience["protocolMapper"] == "oidc-audience-mapper" assert audience["config"]["included.custom.audience"] == "gateway-attach" assert audience["config"]["access.token.claim"] == "true" + assert audience["config"]["id.token.claim"] == "false"🤖 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 `@scripts/tests/test_keycloak_desired_clients_contract.py` around lines 36 - 45, Extend the contract assertions for the principal_id and gateway-attach mappers in the test covering mappers to validate every token-placement flag configured in desired-clients.json: principal_id must be enabled for ID, access, and userinfo tokens, while gateway-attach must be excluded from ID tokens and retain its access-token setting.
🤖 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.
Nitpick comments:
In `@scripts/tests/test_keycloak_desired_clients_contract.py`:
- Around line 36-45: Extend the contract assertions for the principal_id and
gateway-attach mappers in the test covering mappers to validate every
token-placement flag configured in desired-clients.json: principal_id must be
enabled for ID, access, and userinfo tokens, while gateway-attach must be
excluded from ID tokens and retain its access-token setting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fdb63a7-e6c7-4d44-a433-776e04cb6fcf
📒 Files selected for processing (2)
docker/keycloak/desired-clients.jsonscripts/tests/test_keycloak_desired_clients_contract.py
daniyalabbas96
force-pushed
the
daniyal/omn-15877-keycloak-client-contract
branch
from
August 12, 2026 09:27
2f0c1c4 to
b4e9b1b
Compare
Contributor
✅ Hostile Reviewer — PASSEDBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
daniyalabbas96
changed the title
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 `@scripts/tests/test_keycloak_desired_clients_contract.py`:
- Around line 36-42: Extend the principal mapper assertions in the contract test
to validate that config["jsonType.label"] is set to "String", alongside the
existing principal_id mapper checks. Use the existing principal config object
and preserve all current assertions.
- Around line 44-49: Add a positive contract in the relevant Keycloak
desired-configuration test setup for the short-lived user token to include the
gateway-attach audience, then assert that source is present in the generated
configuration. Keep the existing gateway-attach mapper absence and
included.custom.audience checks unchanged as duplicate-prevention guards.
🪄 Autofix
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: 9cf4083e-22c6-4200-9126-174aebd7b632
📒 Files selected for processing (2)
docker/keycloak/desired-clients.jsonscripts/tests/test_keycloak_desired_clients_contract.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/keycloak/desired-clients.json
Comment on lines
+36
to
+42
| principal = mappers["principal_id"] | ||
| assert principal["protocolMapper"] == "oidc-usermodel-attribute-mapper" | ||
| assert principal["config"]["user.attribute"] == "principal_id" | ||
| assert principal["config"]["claim.name"] == "principal_id" | ||
| assert principal["config"]["id.token.claim"] == "true" | ||
| assert principal["config"]["access.token.claim"] == "true" | ||
| assert principal["config"]["userinfo.token.claim"] == "true" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the configured JSON type.
docker/keycloak/desired-clients.json Lines 54-63 set config["jsonType.label"] to "String", but this contract does not check it. Add the assertion so a non-string principal_id claim cannot pass the test.
Proposed assertion
assert principal["config"]["userinfo.token.claim"] == "true"
+ assert principal["config"]["jsonType.label"] == "String"📝 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
| principal = mappers["principal_id"] | |
| assert principal["protocolMapper"] == "oidc-usermodel-attribute-mapper" | |
| assert principal["config"]["user.attribute"] == "principal_id" | |
| assert principal["config"]["claim.name"] == "principal_id" | |
| assert principal["config"]["id.token.claim"] == "true" | |
| assert principal["config"]["access.token.claim"] == "true" | |
| assert principal["config"]["userinfo.token.claim"] == "true" | |
| principal = mappers["principal_id"] | |
| assert principal["protocolMapper"] == "oidc-usermodel-attribute-mapper" | |
| assert principal["config"]["user.attribute"] == "principal_id" | |
| assert principal["config"]["claim.name"] == "principal_id" | |
| assert principal["config"]["id.token.claim"] == "true" | |
| assert principal["config"]["access.token.claim"] == "true" | |
| assert principal["config"]["userinfo.token.claim"] == "true" | |
| assert principal["config"]["jsonType.label"] == "String" |
🤖 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 `@scripts/tests/test_keycloak_desired_clients_contract.py` around lines 36 -
42, Extend the principal mapper assertions in the contract test to validate that
config["jsonType.label"] is set to "String", alongside the existing principal_id
mapper checks. Use the existing principal config object and preserve all current
assertions.
Comment on lines
+44
to
+49
| assert "gateway-attach-audience" not in mappers | ||
| assert all( | ||
| mapper.get("config", {}).get("included.custom.audience") | ||
| != "gateway-attach" | ||
| for mapper in mappers.values() | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'"gateway-attach"|gateway-attach-audience|included.custom.audience' \
docker/keycloak/desired-clients.json \
scripts/tests/test_keycloak_desired_clients_contract.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1630
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- test file ---'
cat -n scripts/tests/test_keycloak_desired_clients_contract.py
printf '%s\n' '--- relevant Keycloak declarations ---'
rg -n -C 12 \
'gateway-attach|omniweb|clientScopes|clientScope|protocolMappers|audience' \
docker scripts \
-g '*.json' -g '*.py' -g '*.sh'
printf '%s\n' '--- repository-wide references ---'
rg -n -C 5 'gateway-attach|gateway-attach-audience' .Repository: OmniNode-ai/omnibase_infra
Length of output: 50382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
path = Path("docker/keycloak/desired-clients.json")
config = json.loads(path.read_text())
print("top-level keys:", sorted(config))
print("client scopes:")
for scope in config.get("clientScopes", []):
print(json.dumps({
"name": scope.get("name"),
"protocol": scope.get("protocol"),
"mappers": scope.get("protocolMappers", []),
}, sort_keys=True))
print("clients:")
for client in config.get("clients", []):
if client.get("clientId") == "omniweb":
print(json.dumps({
"clientId": client.get("clientId"),
"defaultClientScopes": client.get("defaultClientScopes", []),
"optionalClientScopes": client.get("optionalClientScopes", []),
"protocolMappers": client.get("protocolMappers", []),
}, indent=2, sort_keys=True))
PY
printf '%s\n' '--- exact gateway references in configuration and tests ---'
rg -n 'gateway-attach|gateway-attach-audience' \
docker/keycloak/desired-clients.json \
scripts/tests/test_keycloak_desired_clients_contract.py \
scripts/tests/test_seed_keycloak_clients.py || true
printf '%s\n' '--- focused diff ---'
git diff --stat -- scripts/tests/test_keycloak_desired_clients_contract.py docker/keycloak/desired-clients.json
git diff -- scripts/tests/test_keycloak_desired_clients_contract.py docker/keycloak/desired-clients.jsonRepository: OmniNode-ai/omnibase_infra
Length of output: 2032
Add a positive gateway-attach audience contract.
The desired configuration defines no gateway-attach mapper or client scope, so this test can pass while the short-lived user token lacks the required audience. Add the positive source and assert it here. Retain these checks as duplicate-prevention guards.
🤖 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 `@scripts/tests/test_keycloak_desired_clients_contract.py` around lines 44 -
49, Add a positive contract in the relevant Keycloak desired-configuration test
setup for the short-lived user token to include the gateway-attach audience,
then assert that source is present in the generated configuration. Keep the
existing gateway-attach mapper absence and included.custom.audience checks
unchanged as duplicate-prevention guards.
jonahgabriel
pushed a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 13, 2026
#6377) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2726 * evidence: OCC companion self-bind for #6377 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
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
3 participants
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.
Summary
principal_idclaim to OmniWeb access and user-info tokensThe gateway credential is minted separately by the guarded server-side exchange from a per-tenant attach-only client. This PR does not expose a broker credential or add a second browser-visible secret.
Verification
Deployment gate
Live completion requires reviewed backend exchange and attach hardening, canonical reconciliation, fresh signed-token readback, and the negative broker-auth proof.
Evidence-Ticket: OMN-15877
Evidence-Source: OCC#6377