Conversation
📝 WalkthroughWalkthroughThe changes extend JWT token generation with organization identity. The agent token controller and services now extract the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
console/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts (1)
22-31:⚠️ Potential issue | 🟡 MinorInconsistent
subclaim betweendemoUserInfoand JWT payload.The
demoUserInfoobject declaressub: 'default'(line 29), but the JWT token's payload contains"sub": "8f307351-25c5-4fc6-85e0-f51c2d458f06". Since the real Asgardio implementation spreadstokenInfo?.payloadinto the userInfo object, this mismatch could cause unpredictable behavior when code decodes the token versus usinguserInfodirectly.🔧 Align the JWT payload with demoUserInfo
Update the JWT payload to use
"sub": "default"to match line 29, or updatedemoUserInfo.subto match the JWT. For consistency, the stub token payload should mirror the stub userInfo.Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts` around lines 22 - 31, demoUserInfo.sub ('default') is inconsistent with the stub JWT payload's "sub" (GUID); update one to match the other so tokenInfo?.payload spread produces identical sub values—either change demoUserInfo.sub to "8f307351-25c5-4fc6-85e0-f51c2d458f06" or change the JWT payload's "sub" to "default" (also apply the same change to the other stub payload instance referenced around the second occurrence).agent-manager-service/middleware/jwtassertion/test_utils.go (1)
32-39:⚠️ Potential issue | 🔴 CriticalMock
OuIdvalue diverges from the test assertion.The mock seeds
OuId: "mock-org-id", butagent-manager-service/tests/agent_token_test.go(line 149) asserts the generated JWT claim equals"mock-ou-id". Whichever side is intended to be authoritative, these two literals must agree, otherwiseTestGenerateAgentToken/Generating a token for an external agent should return 200 with valid tokenwill fail. See the corresponding comment onagent_token_test.gofor the related JSON-tag mismatch (org_idvsou_id) that needs to be resolved at the same time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/middleware/jwtassertion/test_utils.go` around lines 32 - 39, The mock TokenClaims in test_utils.go sets OuId to "mock-org-id" which conflicts with the test assertion expecting "mock-ou-id" in TestGenerateAgentToken; update the mock TokenClaims.OuId value to exactly match the expected literal ("mock-ou-id") or update the test assertion to match the mock, and at the same time reconcile the JSON tag mismatch between the struct field and test expectation (ensure the claim field name used by TokenClaims/RegisteredClaims serialization matches the test's expected key: "ou_id" vs "org_id"). Locate and fix the TokenClaims.OuId value and the JSON tag on the TokenClaims struct so the generated JWT claim and the test assertion are consistent.
🧹 Nitpick comments (2)
console/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts (1)
42-42: JWT payload missing expected user claims.The JWT payload only contains standard claims (
iss,iat,exp,aud,sub) and the newouIdfield, but is missing several claims present indemoUserInfo(lines 22-31):username,displayName,orgHandle,orgId,orgName,sessionState, andallowedScopes.Since the real Asgardio implementation spreads the decoded JWT payload into the UserInfo object, the stub token should contain equivalent claims to ensure consistent behavior when code decodes and uses the token versus directly accessing
userInfo.💡 Populate JWT payload with demoUserInfo fields
Consider regenerating the JWT with a payload that includes all relevant fields from
demoUserInfo:{ "iss": "Agent Management Platform Local", "iat": 1761727469, "exp": 1793263469, "aud": "localhost", "sub": "default", "username": "john.doe", "displayName": "John Doe", "orgHandle": "default", "orgId": "default", "orgName": "Default", "ouId": "faa183f1-3983-4f63-bb35-86fa723fd5ef" }This ensures that decoding the stub token yields the same user information as the
demoUserInfoobject.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts` at line 42, The JWT returned by getToken() is missing user claims present in demoUserInfo (username, displayName, orgHandle, orgId, orgName, sessionState, allowedScopes), causing mismatch when code decodes the token vs using userInfo; regenerate or replace the hardcoded token in getToken to a JWT whose payload includes those demoUserInfo fields (matching names: username, displayName, orgHandle, orgId, orgName, sessionState, allowedScopes, plus existing ouId/iss/aud/sub/iat/exp) so decoded token and demoUserInfo are equivalent; update the token string in getToken to the new JWT.agent-manager-service/services/agent_token_manager.go (1)
244-300: Consider validatingreq.OrgIdat the service boundary.Both current call sites (
controllers/agent_token_controller.goandservices/agent_manager.go) validate thatOrgIdis non-empty before reaching here, so this is defensive only. However, the service is a public API and a future caller could passreq.OrgId == "", which would silently produce a token with"org_id": "". A short guard would localize the invariant and prevent that drift.🛡️ Proposed defensive check
func (s *agentTokenManagerService) GenerateToken(ctx context.Context, req GenerateTokenRequest) (*spec.TokenResponse, error) { s.logger.Info("Generating token for agent", "agentName", req.AgentName, "orgName", req.OrgName, "projectName", req.ProjectName, ) + + if req.OrgId == "" { + return nil, fmt.Errorf("OrgId is required: %w", utils.ErrInvalidInput) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_token_manager.go` around lines 244 - 300, Add a defensive guard at the start of agentTokenManagerService.GenerateToken to reject empty OrgId: check if req.OrgId == "" (before calling s.ocClient.GetComponent) and if so log an error and return an invalid-input error (e.g., combine utils.ErrInvalidInput with a descriptive error like "org id is required") so the service never issues a token with an empty OrgId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent-manager-service/services/agent_manager.go`:
- Around line 433-438: The code in generateAgentAPIKey checks
jwtassertion.GetTokenClaims(ctx) but when callerClaims is nil or
callerClaims.OuId == "" it logs and returns "", translatePipelineError(err)
which uses a nil err and thus returns nil; change this to return a meaningful
sentinel error (e.g., utils.ErrMissingOrgIdentity or utils.ErrForbidden) instead
of translatePipelineError, so callers get a non-nil error and upstream can map
to 403; update the return in the failing branch to return "",
utils.ErrMissingOrgIdentity (or your project’s equivalent) and keep the existing
log statement to preserve diagnostics.
In `@agent-manager-service/tests/agent_token_test.go`:
- Around line 141-149: The test is asserting the wrong claim key and value;
update the assertions in agent_token_test.go to match the implementation: check
for "org_id" (not "ou_id") and expect the value "mock-org-id" for the org claim
— this aligns with AgentTokenClaims.OrgId (json:"org_id") in
agent-manager-service/services/agent_token_manager.go and the mock setting
TokenClaims.OuId = "mock-org-id" in
agent-manager-service/middleware/jwtassertion/test_utils.go; change
require.Contains(t, claims, "ou_id") → require.Contains(t, claims, "org_id") and
require.Equal(t, "mock-ou-id", claims["ou_id"]) → require.Equal(t,
"mock-org-id", claims["org_id"]).
---
Outside diff comments:
In `@agent-manager-service/middleware/jwtassertion/test_utils.go`:
- Around line 32-39: The mock TokenClaims in test_utils.go sets OuId to
"mock-org-id" which conflicts with the test assertion expecting "mock-ou-id" in
TestGenerateAgentToken; update the mock TokenClaims.OuId value to exactly match
the expected literal ("mock-ou-id") or update the test assertion to match the
mock, and at the same time reconcile the JSON tag mismatch between the struct
field and test expectation (ensure the claim field name used by
TokenClaims/RegisteredClaims serialization matches the test's expected key:
"ou_id" vs "org_id"). Locate and fix the TokenClaims.OuId value and the JSON tag
on the TokenClaims struct so the generated JWT claim and the test assertion are
consistent.
In `@console/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts`:
- Around line 22-31: demoUserInfo.sub ('default') is inconsistent with the stub
JWT payload's "sub" (GUID); update one to match the other so tokenInfo?.payload
spread produces identical sub values—either change demoUserInfo.sub to
"8f307351-25c5-4fc6-85e0-f51c2d458f06" or change the JWT payload's "sub" to
"default" (also apply the same change to the other stub payload instance
referenced around the second occurrence).
---
Nitpick comments:
In `@agent-manager-service/services/agent_token_manager.go`:
- Around line 244-300: Add a defensive guard at the start of
agentTokenManagerService.GenerateToken to reject empty OrgId: check if req.OrgId
== "" (before calling s.ocClient.GetComponent) and if so log an error and return
an invalid-input error (e.g., combine utils.ErrInvalidInput with a descriptive
error like "org id is required") so the service never issues a token with an
empty OrgId.
In `@console/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts`:
- Line 42: The JWT returned by getToken() is missing user claims present in
demoUserInfo (username, displayName, orgHandle, orgId, orgName, sessionState,
allowedScopes), causing mismatch when code decodes the token vs using userInfo;
regenerate or replace the hardcoded token in getToken to a JWT whose payload
includes those demoUserInfo fields (matching names: username, displayName,
orgHandle, orgId, orgName, sessionState, allowedScopes, plus existing
ouId/iss/aud/sub/iat/exp) so decoded token and demoUserInfo are equivalent;
update the token string in getToken to the new JWT.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61275ac8-a18d-4faa-9e2b-26c95307e541
📒 Files selected for processing (6)
agent-manager-service/controllers/agent_token_controller.goagent-manager-service/middleware/jwtassertion/test_utils.goagent-manager-service/services/agent_manager.goagent-manager-service/services/agent_token_manager.goagent-manager-service/tests/agent_token_test.goconsole/workspaces/libs/auth/src/no-auth/hooks/authHooks.ts
| // Extract OrgId from the caller's JWT claims | ||
| callerClaims := jwtassertion.GetTokenClaims(ctx) | ||
| if callerClaims == nil || callerClaims.OuId == "" { | ||
| s.logger.Error("GenerateToken: missing organization identity in caller token") | ||
| return "", translatePipelineError(err) | ||
| } |
There was a problem hiding this comment.
Critical: returns ("", nil) when JWT claims are missing.
At Line 437, err is guaranteed to be nil here (the only assignment to err in scope is the pipeline fetch above, whose error path already returned). translatePipelineError(nil) returns nil, so when the caller's JWT claims are missing or lack OuId, this function silently returns an empty token string with no error. Every caller of generateAgentAPIKey (e.g., buildCreateTraitRequests, attachOTELInstrumentationTrait, attachEnvInjectionTrait, generateTracingEnvVars) will then propagate an empty API key into traits/env vars and continue as if nothing went wrong — producing broken agents that publish traces without authentication.
Additionally, even when an error is returned, translatePipelineError is the wrong semantic translation for an authentication/authorization failure. A dedicated sentinel (e.g., utils.ErrInvalidInput, utils.ErrForbidden, or a new utils.ErrMissingOrgIdentity) would let upstream HTTP handlers map this to 403 instead of "deployment pipeline not found".
🐛 Proposed fix
// Extract OrgId from the caller's JWT claims
callerClaims := jwtassertion.GetTokenClaims(ctx)
if callerClaims == nil || callerClaims.OuId == "" {
- s.logger.Error("GenerateToken: missing organization identity in caller token")
- return "", translatePipelineError(err)
+ s.logger.Error("generateAgentAPIKey: missing organization identity in caller token", "agentName", agentName)
+ return "", fmt.Errorf("missing organization identity in caller token: %w", utils.ErrInvalidInput)
}Adjust the wrapped sentinel to whatever your error model uses for "forbidden / missing auth context".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent-manager-service/services/agent_manager.go` around lines 433 - 438, The
code in generateAgentAPIKey checks jwtassertion.GetTokenClaims(ctx) but when
callerClaims is nil or callerClaims.OuId == "" it logs and returns "",
translatePipelineError(err) which uses a nil err and thus returns nil; change
this to return a meaningful sentinel error (e.g., utils.ErrMissingOrgIdentity or
utils.ErrForbidden) instead of translatePipelineError, so callers get a non-nil
error and upstream can map to 403; update the return in the failing branch to
return "", utils.ErrMissingOrgIdentity (or your project’s equivalent) and keep
the existing log statement to preserve diagnostics.
| require.Contains(t, claims, "ou_id") | ||
| require.Contains(t, claims, "iss") | ||
| require.Contains(t, claims, "exp") | ||
| require.Contains(t, claims, "iat") | ||
|
|
||
| // Validate the component_uid matches what we expect | ||
| require.Equal(t, tokenComponentUid, claims["component_uid"]) | ||
| require.Equal(t, tokenEnvUid, claims["environment_uid"]) | ||
| require.Equal(t, "mock-ou-id", claims["ou_id"]) |
There was a problem hiding this comment.
Test assertions for the new claim won't match the produced token (will fail at runtime).
Two mismatches make these assertions impossible to satisfy as-is:
- Key:
AgentTokenClaims.OrgIdis taggedjson:"org_id"inagent-manager-service/services/agent_token_manager.go(line 67), so the claim is serialized asorg_id— notou_id.require.Contains(t, claims, "ou_id")will fail. - Value: The mock middleware sets
TokenClaims.OuId = "mock-org-id"inagent-manager-service/middleware/jwtassertion/test_utils.go(line 34). That value flows throughcallerClaims.OuId → req.OrgId → claims.OrgId, so even with the right key the value will be"mock-org-id", not"mock-ou-id".
Given the PR's intent ("Add org uuid to the trace publishing token") and the chosen field/tag names, aligning the test with the implementation is the simpler fix.
🐛 Suggested fix to align the test with the implementation
require.Contains(t, claims, "component_uid")
require.Contains(t, claims, "environment_uid")
- require.Contains(t, claims, "ou_id")
+ require.Contains(t, claims, "org_id")
require.Contains(t, claims, "iss")
require.Contains(t, claims, "exp")
require.Contains(t, claims, "iat")
// Validate the component_uid matches what we expect
require.Equal(t, tokenComponentUid, claims["component_uid"])
require.Equal(t, tokenEnvUid, claims["environment_uid"])
- require.Equal(t, "mock-ou-id", claims["ou_id"])
+ require.Equal(t, "mock-org-id", claims["org_id"])Alternatively, if the on-the-wire claim name was intended to be ou_id (mirroring the upstream OuId from caller claims), update the JSON tag in agent_token_manager.go to json:"ou_id" and the mock value in test_utils.go to "mock-ou-id" instead.
📝 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.
| require.Contains(t, claims, "ou_id") | |
| require.Contains(t, claims, "iss") | |
| require.Contains(t, claims, "exp") | |
| require.Contains(t, claims, "iat") | |
| // Validate the component_uid matches what we expect | |
| require.Equal(t, tokenComponentUid, claims["component_uid"]) | |
| require.Equal(t, tokenEnvUid, claims["environment_uid"]) | |
| require.Equal(t, "mock-ou-id", claims["ou_id"]) | |
| require.Contains(t, claims, "org_id") | |
| require.Contains(t, claims, "iss") | |
| require.Contains(t, claims, "exp") | |
| require.Contains(t, claims, "iat") | |
| // Validate the component_uid matches what we expect | |
| require.Equal(t, tokenComponentUid, claims["component_uid"]) | |
| require.Equal(t, tokenEnvUid, claims["environment_uid"]) | |
| require.Equal(t, "mock-org-id", claims["org_id"]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent-manager-service/tests/agent_token_test.go` around lines 141 - 149, The
test is asserting the wrong claim key and value; update the assertions in
agent_token_test.go to match the implementation: check for "org_id" (not
"ou_id") and expect the value "mock-org-id" for the org claim — this aligns with
AgentTokenClaims.OrgId (json:"org_id") in
agent-manager-service/services/agent_token_manager.go and the mock setting
TokenClaims.OuId = "mock-org-id" in
agent-manager-service/middleware/jwtassertion/test_utils.go; change
require.Contains(t, claims, "ou_id") → require.Contains(t, claims, "org_id") and
require.Equal(t, "mock-ou-id", claims["ou_id"]) → require.Equal(t,
"mock-org-id", claims["org_id"]).
Missing: Claim mapping and collector config for
|
2 participants
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Tests