Skip to content

Fix authz sidecar evaluating tokens#683

Merged
RyaliNvidia merged 1 commit intomainfrom
ryali/authz-token
Mar 11, 2026
Merged

Fix authz sidecar evaluating tokens#683
RyaliNvidia merged 1 commit intomainfrom
ryali/authz-token

Conversation

@RyaliNvidia
Copy link
Contributor

@RyaliNvidia RyaliNvidia commented Mar 11, 2026

Currently, we set jwt values to determine if it is from a token or from the user but the sidecar didn't compute those values so tokens had all the access the user had.

Issue #None

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Summary by CodeRabbit

  • Improvements
    • Enhanced request logging with additional traceability identifiers for improved system monitoring and debugging.
    • Updated role synchronization behavior to optimize handling of access tokens and workflow-originated requests.
    • Improved observability across authorization server operations with enhanced error and performance logging.

@RyaliNvidia RyaliNvidia requested a review from a team as a code owner March 11, 2026 17:27
@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

📝 Walkthrough

Walkthrough

Enhanced authorization sidecar server with new request headers for token name and workflow ID. Implemented conditional role synchronization to skip syncing when these headers are present, indicating access tokens or workflow-originated requests. Added corresponding traceability to logging throughout the service.

Changes

Cohort / File(s) Summary
Authorization Headers & Sync Logic
src/service/authz_sidecar/server/authz_server.go
Added x-osmo-token-name and x-osmo-workflow-id header extraction. Modified synchronization logic to skip role syncing when either header is present. Enhanced logging and error reporting with new tokenName and workflowID fields for improved request traceability.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit hops through headers new,
Where tokens dance and workflows brew,
Skip that sync when headers align,
Logs now sparkle, all things shine!
Traceability's the prize we chase,
With token names in every place. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix authz sidecar evaluating tokens' directly addresses the main change: correcting JWT-derived value computation in the sidecar to properly distinguish token vs. user requests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ryali/authz-token

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/service/authz_sidecar/server/authz_server.go (1)

178-185: Consider centralizing the repeated request log attrs.

The new token_name / workflow_id fields are now duplicated across several log sites. A small helper or shared []any attr slice would keep future auth-context changes consistent and reduce omission risk.

Also applies to: 190-195, 207-229, 242-253

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/service/authz_sidecar/server/authz_server.go` around lines 178 - 185,
Several logger.Debug sites repeat the same authorization request attributes
(user, path, method, token_name, workflow_id, roles); centralize them by
creating a small helper that returns the shared []any slice and use it wherever
s.logger.Debug(...) is called (e.g., replace the repeated attr lists in the
Debug calls around the authorization check with something like
authRequestLogAttrs(...) and call s.logger.Debug("authorization check request",
authRequestLogAttrs(user, path, method, tokenName, workflowID, roleNames)...)).
Update all occurrences noted (the blocks around lines 178-185, 190-195, 207-229,
242-253) to use this helper so future auth-context fields stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/service/authz_sidecar/server/authz_server.go`:
- Around line 178-185: Several logger.Debug sites repeat the same authorization
request attributes (user, path, method, token_name, workflow_id, roles);
centralize them by creating a small helper that returns the shared []any slice
and use it wherever s.logger.Debug(...) is called (e.g., replace the repeated
attr lists in the Debug calls around the authorization check with something like
authRequestLogAttrs(...) and call s.logger.Debug("authorization check request",
authRequestLogAttrs(user, path, method, tokenName, workflowID, roleNames)...)).
Update all occurrences noted (the blocks around lines 178-185, 190-195, 207-229,
242-253) to use this helper so future auth-context fields stay consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 820c4d9a-f6f4-40fa-801d-b154f294342b

📥 Commits

Reviewing files that changed from the base of the PR and between e58bd4f and 30395ed.

📒 Files selected for processing (1)
  • src/service/authz_sidecar/server/authz_server.go

@RyaliNvidia RyaliNvidia merged commit ba252dc into main Mar 11, 2026
10 checks passed
@RyaliNvidia RyaliNvidia deleted the ryali/authz-token branch March 11, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants