Skip to content

feat(cardinal): authenticate game tokens scoped to organization/project - #1004

Open
ryanditjia wants to merge 9 commits into
mainfrom
ryandi/game-player-auth
Open

ryanditjia wants to merge 9 commits into
mainfrom
ryandi/game-player-auth

Conversation

@ryanditjia

@ryanditjia ryanditjia commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Cardinal authenticates commands and event subscriptions with the stable player ID in a game token's sub. Saving guest progress to a registered account therefore keeps the identity used by game systems.

Validation checks the signature, issuer, expiration, and audience against the shard's existing organization/project configuration. Tokens for another game and ordinary account tokens are rejected. Development clients use X-Player-ID. Game systems continue receiving the player ID through cmd.Persona; wire-format persona cleanup is separate. Expiration is checked when requests and streams start.

Deploy Auth #778 before this engine update.

Validation:

  • go test ./pkg/cardinal passed.
  • Targeted Cardinal lint passed with zero issues.
  • Tests cover invalid game-token claims, audience isolation, and development identities.

@claude

claude Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Code Review in progress

  • Gather context (diff, CLAUDE.md, related files)
  • Review auth changes in pkg/cardinal/service.go
  • Review tests
  • Post findings

View job run · ryandi/game-player-auth

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed against the latest diff

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread pkg/cardinal/auth_internal_test.go
Comment thread pkg/cardinal/service.go

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread pkg/cardinal/auth_internal_test.go
Comment thread pkg/cardinal/auth_internal_test.go
Comment thread pkg/cardinal/service.go

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 2 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread pkg/cardinal/service.go
@ryanditjia ryanditjia changed the title feat(cardinal): authenticate stable game player identities feat(cardinal): authenticate players with game-scoped access tokens Sep 24, 2026
@ryanditjia
ryanditjia force-pushed the ryandi/game-player-auth branch from eafce1b to 949b9ec Compare September 29, 2026 16:13
@ryanditjia ryanditjia changed the title feat(cardinal): authenticate players with game-scoped access tokens feat(auth): add game player tokens and renewable CLI sessions Sep 29, 2026
@ryanditjia ryanditjia changed the title feat(auth): add game player tokens and renewable CLI sessions feat(cardinal): authenticate game tokens scoped to organization/project Oct 1, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 4 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. Auto-approval skipped because cubic reviewed only this push, not the earlier force-push. Comment @cubic review to review the whole pull request.

Re-trigger cubic

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@ryanditjia
ryanditjia force-pushed the ryandi/game-player-auth branch from 9ac3b9a to e2cb5e4 Compare October 1, 2026 16:52
ryanditjia and others added 3 commits October 2, 2026 20:52
Shards reach Auth through an internal URL that differs from the public
issuer. Signature, audience, and expiry still bind the token to Auth and
this project.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 2, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1665 1 1664 7
View the top 1 failed test(s) by shortest run time
github.com/argus-labs/world-engine/pkg/cardinal::TestAuthenticatorArgusAcceptsGamePlayerToken
Stack Traces | 0s run time
=== RUN   TestAuthenticatorArgusAcceptsGamePlayerToken
    auth_internal_test.go:23: 
        	Error Trace:	.../pkg/cardinal/auth_internal_test.go:23
        	Error:      	Received unexpected error:
        	            	HTTP error: 404 - 404 Not Found
        	            		cardinal.TestAuthenticatorArgusAcceptsGamePlayerToken:.../pkg/cardinal/auth_internal_test.go:22
        	            		cardinal.newAuthenticatorArgus:.../pkg/cardinal/service.go:758
        	Test:       	TestAuthenticatorArgusAcceptsGamePlayerToken
--- FAIL: TestAuthenticatorArgusAcceptsGamePlayerToken (0.00s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="docs/cardinal/client-integration.mdx">

<violation number="1" location="docs/cardinal/client-integration.mdx:57">
P2: This snippet only handles `AuthNoSavedSessionException`, so a restore failure for any other reason (expired/revoked session, network error) or a failed `SignInAsync` leaves `result.Player` empty and the example proceeds to `client.Player` with no log and no handling. The previous version guarded on `result.Error == null`; check `result.Error` after the restore/sign-in chain and bail out before using `client.Player`.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread docs/cardinal/client-integration.mdx Outdated
if (result.Error is AuthNoSavedSessionException)
result = await client.SignInAsync(); // or SignInAsGuestAsync()

if (result.Player is PlayerState.Registered player)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: This snippet only handles AuthNoSavedSessionException, so a restore failure for any other reason (expired/revoked session, network error) or a failed SignInAsync leaves result.Player empty and the example proceeds to client.Player with no log and no handling. The previous version guarded on result.Error == null; check result.Error after the restore/sign-in chain and bail out before using client.Player.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/cardinal/client-integration.mdx, line 57:

<comment>This snippet only handles `AuthNoSavedSessionException`, so a restore failure for any other reason (expired/revoked session, network error) or a failed `SignInAsync` leaves `result.Player` empty and the example proceeds to `client.Player` with no log and no handling. The previous version guarded on `result.Error == null`; check `result.Error` after the restore/sign-in chain and bail out before using `client.Player`.</comment>

<file context>
@@ -45,20 +45,20 @@ Create a client with a configuration object that specifies the auth URL and regi
+    if (result.Error is AuthNoSavedSessionException)
+        result = await client.SignInAsync(); // or SignInAsGuestAsync()
+
+    if (result.Player is PlayerState.Registered player)
+        Debug.Log($"Signed in as {player.Email} (Player: {player.Id})");
 
</file context>

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="pkg/cardinal/service.go">

<violation number="1" location="pkg/cardinal/service.go:741">
P1: This concatenation breaks valid auth URLs with a trailing slash by requesting `//auth/jwks`, so the shard can fail during authenticator initialization. Preserve trailing-slash normalization before appending the JWKS path.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread pkg/cardinal/service.go
func newAuthenticatorArgus(argusAuthURL, organization, project string) (*authenticatorArgus, error) {
assert.That(argusAuthURL != "", "Should've validated the URL")

jwksURL := argusAuthURL + "/auth/jwks"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: This concatenation breaks valid auth URLs with a trailing slash by requesting //auth/jwks, so the shard can fail during authenticator initialization. Preserve trailing-slash normalization before appending the JWKS path.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At pkg/cardinal/service.go, line 741:

<comment>This concatenation breaks valid auth URLs with a trailing slash by requesting `//auth/jwks`, so the shard can fail during authenticator initialization. Preserve trailing-slash normalization before appending the JWKS path.</comment>

<file context>
@@ -738,7 +738,7 @@ type authenticatorArgus struct {
 	assert.That(argusAuthURL != "", "Should've validated the URL")
 
-	jwksURL := strings.TrimRight(argusAuthURL, "/") + "/auth/jwks"
+	jwksURL := argusAuthURL + "/auth/jwks"
 	client := &http.Client{
 		Timeout: 3 * time.Second,
</file context>
Suggested change
jwksURL := argusAuthURL + "/auth/jwks"
jwksURL := strings.TrimRight(argusAuthURL, "/") + "/auth/jwks"

…DK API

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="docs/cardinal/client-integration.mdx">

<violation number="1" location="docs/cardinal/client-integration.mdx:53">
P3: The startup sample never handles the `SignedOut` case, even though the paragraph above says `AuthStatus` tells the game what it can do. A fresh install (no saved login) follows this snippet and gets no render and no direction to sign in; a failed `RefreshAuthAsync` is likewise silently swallowed. Render the `SignedOut` state and make refresh failure visible so readers following the sample end up authenticated or with a clear next step.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment on lines +53 to +56
client.AuthChanged += Render;
Render();
if (client.AuthStatus == AuthStatus.NeedsRefresh)
await client.RefreshAuthAsync();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: The startup sample never handles the SignedOut case, even though the paragraph above says AuthStatus tells the game what it can do. A fresh install (no saved login) follows this snippet and gets no render and no direction to sign in; a failed RefreshAuthAsync is likewise silently swallowed. Render the SignedOut state and make refresh failure visible so readers following the sample end up authenticated or with a clear next step.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/cardinal/client-integration.mdx, line 53:

<comment>The startup sample never handles the `SignedOut` case, even though the paragraph above says `AuthStatus` tells the game what it can do. A fresh install (no saved login) follows this snippet and gets no render and no direction to sign in; a failed `RefreshAuthAsync` is likewise silently swallowed. Render the `SignedOut` state and make refresh failure visible so readers following the sample end up authenticated or with a clear next step.</comment>

<file context>
@@ -45,20 +45,26 @@ Create a client with a configuration object that specifies the auth URL and regi
-    var result = await client.RestoreSessionAsync();
-    if (result.Error is AuthNoSavedSessionException)
-        result = await client.SignInAsync(); // or SignInAsGuestAsync()
+    client.AuthChanged += Render;
+    Render();
+    if (client.AuthStatus == AuthStatus.NeedsRefresh)
</file context>
Suggested change
client.AuthChanged += Render;
Render();
if (client.AuthStatus == AuthStatus.NeedsRefresh)
await client.RefreshAuthAsync();
client.AuthChanged += Render;
Render();
if (client.AuthStatus == AuthStatus.SignedOut)
Debug.Log("No saved login — sign in to continue.");
else if (client.AuthStatus == AuthStatus.NeedsRefresh)
await client.RefreshAuthAsync();

This branch has not been deployed

No deployments
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.

1 participant