feat: new custom auth/link handler using argus id #815
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “graphite/merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
WalkthroughThe changes in this pull request introduce functionality for JWT authentication via Argus ID, including validation, parsing, and error handling. A new file, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)relay/nakama/auth/custom.go (2)🔇 Additional comments (3)relay/nakama/auth/custom.go (3)
The warning message says "You won't be able to use Argus ID custom link," but since this function is used for both authentication and linking, consider updating the message to reflect both functionalities. Apply this diff to improve the log message: - logger.Warn("JWT secret isn't set. You won't be able to use Argus ID custom link")
+ logger.Warn("JWT secret isn't set. Argus ID custom authentication and linking will not be available")
Update calls to The method signatures of Run the following script to find all usages of these functions: Also applies to: 97-101 ✅ Verification successfulBased on the results, I can generate the final response as we have found all the usages of both functions in the codebase. All calls to The codebase scan shows that all calls to both functions in 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find all calls to `InitCustomAuthentication` and `InitCustomLink` that need updating.
# Search for calls to `InitCustomAuthentication` and `InitCustomLink`
rg -t go 'InitCustomAuthentication\('
rg -t go 'InitCustomLink\('
Length of output: 487
Ensure consistent OpenTelemetry tracing across handlers The addition of OpenTelemetry tracing in Run the following script to check for tracing in other authentication and linking handlers: Also applies to: 112-140 ✅ Verification successfulLet me search for other authentication handlers to ensure we have a complete picture. OpenTelemetry tracing is consistently implemented across authentication handlers The verification shows that OpenTelemetry tracing is properly implemented across all authentication and linking handlers in the codebase. Specifically:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for OpenTelemetry tracing in other authentication and linking handlers.
# Find all handler functions related to authentication and linking
rg -t go 'func handle.*Authentication' -A 15
rg -t go 'func handle.*Link' -A 15
# Search within these handlers for usage of `otel.Tracer`
rg -t go 'func handle.*Authentication' -A 15 | rg 'otel\.Tracer'
rg -t go 'func handle.*Link' -A 15 | rg 'otel\.Tracer'
Length of output: 2678 Script: #!/bin/bash
# Search for all authentication-related files and functions
fd -e go | xargs rg -l 'auth|Auth'
# Then search for OpenTelemetry usage in these files
fd -e go | xargs rg 'otel\.Tracer|trace\.WithAttributes|span\.End'
Length of output: 4757 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd the label graphite/merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #815 +/- ##
==========================================
- Coverage 58.19% 57.86% -0.34%
==========================================
Files 144 145 +1
Lines 9229 9384 +155
==========================================
+ Hits 5371 5430 +59
- Misses 3409 3504 +95
- Partials 449 450 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
relay/nakama/go.modis excluded by!**/*.modrelay/nakama/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
relay/nakama/auth/argus_id.go(1 hunks)relay/nakama/auth/argus_id_test.go(1 hunks)relay/nakama/auth/custom.go(3 hunks)relay/nakama/auth/siwe.go(6 hunks)relay/nakama/main.go(1 hunks)
🔇 Additional comments (14)
relay/nakama/auth/argus_id_test.go (3)
1-14: LGTM! Appropriate imports and test setup.
The imports cover all necessary functionality for JWT testing, and the test secret is properly isolated.
52-69: LGTM! Good error validation.
The test properly verifies JWT signature validation with an incorrect secret.
1-101: Verify test coverage against PR objectives.
The test file provides good coverage of JWT validation scenarios, aligning with the PR objectives for implementing Supabase authentication. However, consider verifying that all requirements from WORLD-1254 are covered:
✅ Verification successful
Let me gather more specific information about the JWT validation implementation and test coverage.
Test coverage is comprehensive and aligns with implementation
The test file provides thorough coverage of the JWT validation implementation:
- Happy path testing with valid JWT and hash
- Error cases for invalid hash, wrong secret, incorrect signing method
- Validation of JWT claims and subject field
- All error paths in the implementation are tested
The implementation includes proper OpenTelemetry integration and error handling as evidenced by the codebase search results. No additional test cases are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for JWT validation requirements
# Look for test coverage of error messages mentioned in the issue
rg -l "ErrInvalid.*JWT" --type go
# Check for OpenTelemetry tracing in the main implementation
rg -l "opentelemetry|otel" --type go
# Look for logging implementations
rg -l "log\." --type go
Length of output: 1533
Script:
#!/bin/bash
# Check the main JWT validation implementation
ast-grep --pattern 'func validateAndParseJWT($$$)' relay/nakama/auth/argus_id.go
# Check for error definitions
rg "var Err.*JWT.*=" relay/nakama/auth/argus_id.go
# Check test coverage of error cases
rg "Test.*JWT.*" relay/nakama/auth/argus_id_test.go
Length of output: 2436
relay/nakama/auth/siwe.go (4)
11-11: LGTM: OpenTelemetry import added for tracing support
The addition of OpenTelemetry tracing import aligns with the PR objectives for adding tracing capabilities.
27-27: LGTM: Error message corrected for accuracy
The error message now correctly indicates that the message field is missing when a signature is provided without a message.
89-89: LGTM: Tracing properly integrated into authentication flow
The span parameter is correctly propagated through the authentication chain.
Also applies to: 95-95
109-109: LGTM: Tracing implementation consistent with authentication handler
The linking handler correctly mirrors the tracing implementation from the authentication handler.
Let's verify the consistency of tracing implementation across auth handlers:
Also applies to: 115-115
✅ Verification successful
Tracing implementation verified as consistent across SIWE handlers
The verification confirms that:
- Both
authWithSIWEandlinkWithSIWEfunctions consistently include thetrace.Spanparameter - Both handlers correctly pass the span to
processSIWE - The
processSIWEfunction properly utilizes the span for tracing withAddEvent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other auth handlers to ensure consistent tracing implementation
ast-grep --pattern 'func $handler($_, $_, $_, $_, trace.Span)'
Length of output: 64
Script:
#!/bin/bash
# Let's try a more specific search for auth-related functions with span parameter
rg -l "func.*\(.*trace\.Span\)" relay/nakama/auth/
Length of output: 52
Script:
#!/bin/bash
# Let's also look for the actual function implementations
rg "func.*\(.*trace\.Span.*\)" relay/nakama/auth/ -A 1
Length of output: 56
Script:
#!/bin/bash
# Search for processSIWE calls to verify consistent span usage
rg "processSIWE\(" relay/nakama/auth/ -A 1
Length of output: 436
Script:
#!/bin/bash
# Let's look at the complete implementation of authWithSIWE and linkWithSIWE
rg "func (auth|link)WithSIWE" relay/nakama/auth/siwe.go -B 1 -A 5
Length of output: 350
Script:
#!/bin/bash
# Let's also check the complete processSIWE function signature
rg "^func processSIWE" relay/nakama/auth/siwe.go -A 10
Length of output: 449
relay/nakama/main.go (1)
129-130: LGTM! Verify initialization order dependencies.
The addition of the logger parameter aligns with the PR's objective of implementing custom authentication using Argus ID. The error handling is appropriate.
Let's verify the initialization order dependencies:
✅ Verification successful
Based on the results, I can now generate the final response:
Initialization order is correctly implemented
The code shows a clear and safe initialization sequence:
InitCustomAuthenticationandInitCustomLinkare independent handlers that only share environment variables- Both handlers are properly registered with the Nakama runtime via
RegisterBeforeAuthenticateCustomandRegisterBeforeLinkCustomrespectively - The shared JWT secret is safely accessed within each handler's scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any dependencies that might require specific initialization order
# Check for any direct dependencies between auth initialization and other components
rg -A 5 -B 5 "InitCustomAuthentication|InitCustomLink" --type go
# Look for any shared state or resources that might require specific initialization order
ast-grep --pattern 'var $name = $expr'
Length of output: 2593
relay/nakama/auth/custom.go (4)
55-59: Verify correctness of OpenTelemetry tracing implementation
The OpenTelemetry tracing seems correctly implemented, but ensure that the tracer name "nakama.auth" is consistent across the codebase and that the span attributes are meaningful for trace analysis.
Run the following script to check for consistent tracer names:
✅ Verification successful
Let me search for all OpenTelemetry tracer names to ensure consistency across the codebase.
OpenTelemetry tracer naming follows consistent domain-based pattern
The tracer name "nakama.auth" is correctly used and follows a consistent domain-based naming pattern across the codebase:
- Authentication operations use "nakama.auth"
- RPC handlers use "nakama.rpc"
- Other components use their respective domain names (e.g., "world", "router", "system", "txpool", "redis", "ecb")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the tracer name "nakama.auth" is used consistently.
# Test: Search for all tracer initializations.
# Expect: Only "nakama.auth" is used.
rg 'otel\.Tracer\("nakama\.auth"\)'
Length of output: 233
Script:
#!/bin/bash
# Search for all OpenTelemetry tracer initializations
rg 'otel\.Tracer\(' -A 1
Length of output: 1891
62-70: Ensure error statuses are set appropriately in spans
The spans record errors and set statuses based on the authentication outcome. Verify that all error paths correctly record errors and set the span status to otelcode.Error.
Run the following script to check error handling in spans:
✅ Verification successful
Let me analyze the error handling in spans by looking at the context around these error recordings. I'll check if the error paths are consistently handled.
Error handling in spans is implemented correctly
The error handling in spans follows a consistent pattern across all error paths in the authentication flow:
- Each error case properly records the error using
span.RecordError(err) - Each error is followed by an appropriate status update using
span.SetStatus(otelcode.Error, message) - The error messages are descriptive and specific to each failure case
- The error recording is always paired with returning the error to the caller
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that errors in spans are correctly handled.
# Test: Search for all occurrences where spans record errors.
# Expect: Errors are recorded and span statuses are set appropriately.
rg 'span\.RecordError\(.*\)'
rg 'span\.SetStatus\(otelcode\.Error,.*\)'
Length of output: 5434
Script:
#!/bin/bash
# Get more context around the error handling in auth/custom.go
rg -B 2 -A 2 'span\.RecordError\(' relay/nakama/auth/custom.go
Length of output: 1117
106-109:
Ensure safety when accessing GetVars() to prevent nil pointer dereference
Similar to handleCustomAuthentication, ensure that GetVars() is not nil before accessing it to prevent a potential nil pointer dereference.
Apply this diff to add nil checks:
func handleCustomLink(
ctx context.Context,
logger runtime.Logger,
_ *sql.DB,
nk runtime.NakamaModule,
in *api.AccountCustom) (*api.AccountCustom, error) {
- authType := in.GetVars()["type"]
+ vars := in.GetVars()
+ if vars == nil {
+ return nil, errors.New("account vars are nil")
+ }
+ authType := vars["type"]Likely invalid or redundant comment.
72-81: Handle missing or unsupported authType gracefully
If authType is missing or does not match known types, the function proceeds to record an error. Ensure that you handle unsupported authType values by providing informative error messages to the client.
Run the following script to check for coverage of possible authType values:
relay/nakama/auth/argus_id.go (2)
101-103: Validate presence of JWT hash and JWT string before processing.
To prevent potential null pointer exceptions or improper handling, ensure that jwtHash and jwt are not empty before proceeding with validation.
Apply this diff to add validation:
jwtHash := in.GetAccount().GetId()
jwt := in.GetAccount().GetVars()["jwt"]
+if jwtHash == "" || jwt == "" {
+ _, err := utils.LogErrorWithMessageAndCode(logger, nil, codes.InvalidArgument, "JWT hash or JWT string is missing")
+ return nil, err
+}
claims, err := validateAndParseJWT(jwtHash, jwt, globalJWTSecret)144-146: Validate presence of JWT hash and JWT string before processing.
Ensure that jwtHash and jwt are not empty to prevent errors during validation.
Apply this diff:
jwtHash := in.GetId()
jwt := in.GetVars()["jwt"]
+if jwtHash == "" || jwt == "" {
+ _, err := utils.LogErrorWithMessageAndCode(logger, nil, codes.InvalidArgument, "JWT hash or JWT string is missing")
+ return nil, err
+}
claims, err := validateAndParseJWT(jwtHash, jwt, globalJWTSecret)
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
relay/nakama/auth/argus_id.go(1 hunks)relay/nakama/auth/argus_id_test.go(1 hunks)relay/nakama/auth/custom.go(3 hunks)
🧰 Additional context used
📓 Learnings (3)
relay/nakama/auth/argus_id.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id.go:159-165
Timestamp: 2024-11-21T17:08:28.410Z
Learning: In the function `linkWithArgusID` within `relay/nakama/auth/argus_id.go`, the `Vars` field of `api.AccountCustom` (`in.Vars`) is initialized to an empty map by default. Therefore, explicit initialization of `in.Vars` is unnecessary.
relay/nakama/auth/argus_id_test.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id_test.go:34-50
Timestamp: 2024-11-21T16:47:32.434Z
Learning: In Go tests in `relay/nakama/auth/argus_id_test.go`, when testing the `validateAndParseJWT` function, it's acceptable to use a simple invalid hash string like `"invalidhashvalue"` for `wrongHash`, and it's unnecessary to use actual SHA256 hashes of different JWTs.
relay/nakama/auth/custom.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:55-55
Timestamp: 2024-11-21T17:05:28.598Z
Learning: In `handleCustomAuthentication` in `relay/nakama/auth/custom.go`, nil pointer checks for `in.GetAccount()` and `in.GetAccount().GetVars()` are handled in `validateAndParseJWT`, so explicit nil checks are not needed.
🔇 Additional comments (8)
relay/nakama/auth/argus_id_test.go (4)
1-12: LGTM! Clean package structure and imports.
The package structure is well-organized with all necessary imports and no redundant ones.
14-14: LGTM! Well-named test constant.
The test JWT secret is appropriately named to indicate its testing purpose.
34-51: LGTM! Simple and effective wrong ID test.
The test effectively verifies the invalid hash scenario. The simple invalid hash string is appropriate for this test case, as confirmed by previous learnings.
53-70: LGTM! Comprehensive wrong secret test.
The test effectively verifies JWT validation failure with an incorrect secret and properly asserts the specific error.
relay/nakama/auth/custom.go (3)
43-43: Fix Typo in Comment
The word "essentially" is misspelled.
[typo]
Apply this diff to correct the typo:
-// be shared because it is essentialy a
+// be shared because it is essentially a47-51: Verify Impact of Changing Function Signature of InitCustomAuthentication
The InitCustomAuthentication function signature now includes an additional logger parameter. Ensure that all calls to this function across the codebase are updated accordingly to prevent compile-time errors.
Run the following script to find all usages of InitCustomAuthentication and verify the parameters:
✅ Verification successful
All calls to InitCustomAuthentication are properly updated with the logger parameter
The search results show only two occurrences of InitCustomAuthentication:
- The function definition in
relay/nakama/auth/custom.gowith the correct signature(logger runtime.Logger, initializer runtime.Initializer) - The function call in
relay/nakama/main.gowhich correctly passes both parameters:auth.InitCustomAuthentication(logger, initializer)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of InitCustomAuthentication and check for correct parameters.
# Search for function calls to InitCustomAuthentication in Go files
rg 'InitCustomAuthentication\(' --glob '*.go' -A 1
Length of output: 485
95-99: Verify Impact of Changing Function Signature of InitCustomLink
The InitCustomLink function signature now includes an additional logger parameter. Ensure that all calls to this function across the codebase are updated accordingly to prevent compile-time errors.
Run the following script to find all usages of InitCustomLink and verify the parameters:
✅ Verification successful
All calls to InitCustomLink are properly updated with the logger parameter
The search results show only two occurrences of InitCustomLink:
- The function definition in
relay/nakama/auth/custom.go - The function call in
relay/nakama/main.gowhich correctly passes both required parameters:loggerandinitializer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of InitCustomLink and check for correct parameters.
# Search for function calls to InitCustomLink in Go files
rg 'InitCustomLink\(' --glob '*.go' -A 1
Length of output: 427
relay/nakama/auth/argus_id.go (1)
95-98: Ensure GlobalJWTSecret is properly initialized
The GlobalJWTSecret should be initialized before authentication attempts to prevent errors. Verify that it is set during application startup and handle any initialization errors appropriately.
Would you like assistance in verifying the initialization of GlobalJWTSecret?
rmrt1n
force-pushed
the
ryan/world-1254-nakama-argus-id
branch
from
773d21e to
4f24373
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
relay/nakama/go.modis excluded by!**/*.modrelay/nakama/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
relay/nakama/auth/argus_id.go(1 hunks)relay/nakama/auth/argus_id_test.go(1 hunks)relay/nakama/auth/custom.go(3 hunks)relay/nakama/auth/siwe.go(6 hunks)relay/nakama/main.go(1 hunks)
🧰 Additional context used
📓 Learnings (3)
relay/nakama/auth/argus_id_test.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id_test.go:34-50
Timestamp: 2024-11-21T16:47:32.434Z
Learning: In Go tests in `relay/nakama/auth/argus_id_test.go`, when testing the `validateAndParseJWT` function, it's acceptable to use a simple invalid hash string like `"invalidhashvalue"` for `wrongHash`, and it's unnecessary to use actual SHA256 hashes of different JWTs.
relay/nakama/auth/argus_id.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id.go:159-165
Timestamp: 2024-11-21T17:08:28.410Z
Learning: In the function `linkWithArgusID` within `relay/nakama/auth/argus_id.go`, the `Vars` field of `api.AccountCustom` (`in.Vars`) is initialized to an empty map by default. Therefore, explicit initialization of `in.Vars` is unnecessary.
relay/nakama/auth/custom.go (2)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:29-36
Timestamp: 2024-11-21T17:33:33.067Z
Learning: In `relay/nakama/auth/custom.go`, the `checkJWTSecret` function is not called concurrently, so accessing and setting `GlobalJWTSecret` does not cause data races in this context.
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:55-55
Timestamp: 2024-11-21T17:05:28.598Z
Learning: In `handleCustomAuthentication` in `relay/nakama/auth/custom.go`, nil pointer checks for `in.GetAccount()` and `in.GetAccount().GetVars()` are handled in `validateAndParseJWT`, so explicit nil checks are not needed.
🔇 Additional comments (5)
relay/nakama/auth/argus_id.go (1)
51-54: 🛠️ Refactor suggestion
Use constant-time comparison to prevent timing attacks
Comparing hashes using != can be vulnerable to timing attacks. Use subtle.ConstantTimeCompare for secure, constant-time comparison.
Apply this diff to enhance security:
import (
+ "crypto/subtle"
"crypto/sha256"
"encoding/hex"
)
func validateAndParseJWT(jwtHash string, jwtString string, jwtSecret string) (*SupabaseClaims, error) {
computedHash := sha256.Sum256([]byte(jwtString))
computedHashString := hex.EncodeToString(computedHash[:])
- if computedHashString != jwtHash {
+ if subtle.ConstantTimeCompare([]byte(computedHashString), []byte(jwtHash)) != 1 {
return nil, ErrInvalidIDForJWT
}Likely invalid or redundant comment.
relay/nakama/auth/argus_id_test.go (1)
31-32: 🧹 Nitpick (assertive)
Assert returned claims to ensure correctness
While the test verifies successful validation, it's beneficial to assert that the returned claims match the expected values.
Apply this diff to enhance the test:
_, err = validateAndParseJWT(jwtHash, jwtString, testJWTSecret)
assert.Nil(t, err)
+assert.Equal(t, claims.Subject, parsedClaims.Subject)
+assert.Equal(t, claims.UserMetaData, parsedClaims.UserMetaData)relay/nakama/auth/siwe.go (1)
Line range hint 68-70: Enhance tracing by recording errors in spans
When errors occur, recording them in spans improves observability and debugging.
Apply this diff to add error details to the span:
if err != nil {
+ span.RecordError(err)
+ span.SetStatus(otelcode.Error, "Validation failed")
_, err = utils.LogErrorWithMessageAndCode(logger, err, codes.InvalidArgument, "invalid vars")
return err
}Repeat similar changes at other error handling points.
relay/nakama/auth/custom.go (1)
34-34: 🧹 Nitpick (assertive)
Clarify the log message in checkJWTSecret
Since checkJWTSecret is used for both authentication and linking, update the log message to reflect both.
Apply this diff to improve the clarity:
if GlobalJWTSecret == "" {
- logger.Warn("JWT secret isn't set. You won't be able to use Argus ID custom link")
+ logger.Warn("JWT secret isn't set. Argus ID custom authentication and linking will not be available")
}relay/nakama/main.go (1)
129-134: 🧹 Nitpick (assertive)
Add tracing spans around custom authentication and linking initialization
Including spans enhances tracing and observability during initialization.
Apply this diff to incorporate tracing:
+ ctx, span := otel.Tracer("nakama.init").Start(ctx, "InitCustomAuthentication")
+ defer span.End()
if err := auth.InitCustomAuthentication(logger, initializer); err != nil {
return eris.Wrap(err, "failed to init custom authentication")
}
+ span.SetStatus(otelcode.Ok, "Custom authentication initialized successfully")
+ ctx, span = otel.Tracer("nakama.init").Start(ctx, "InitCustomLink")
+ defer span.End()
if err := auth.InitCustomLink(logger, initializer); err != nil {
return eris.Wrap(err, "failed to init custom link")
}
+ span.SetStatus(otelcode.Ok, "Custom link initialized successfully")Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
relay/nakama/auth/argus_id.go(1 hunks)relay/nakama/auth/custom.go(3 hunks)
🧰 Additional context used
📓 Learnings (2)
relay/nakama/auth/custom.go (2)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:29-36
Timestamp: 2024-11-21T17:33:33.067Z
Learning: In `relay/nakama/auth/custom.go`, the `checkJWTSecret` function is not called concurrently, so accessing and setting `GlobalJWTSecret` does not cause data races in this context.
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:55-55
Timestamp: 2024-11-21T17:05:28.598Z
Learning: In `handleCustomAuthentication` in `relay/nakama/auth/custom.go`, nil pointer checks for `in.GetAccount()` and `in.GetAccount().GetVars()` are handled in `validateAndParseJWT`, so explicit nil checks are not needed.
relay/nakama/auth/argus_id.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id.go:159-165
Timestamp: 2024-11-21T17:08:28.410Z
Learning: In the function `linkWithArgusID` within `relay/nakama/auth/argus_id.go`, the `Vars` field of `api.AccountCustom` (`in.Vars`) is initialized to an empty map by default. Therefore, explicit initialization of `in.Vars` is unnecessary.
🪛 golangci-lint (1.62.2)
relay/nakama/auth/argus_id.go
1-1: : # pkg.world.dev/world-engine/relay/nakama/auth [pkg.world.dev/world-engine/relay/nakama/auth.test]
auth/argus_id_test.go:31:51: not enough arguments in call to validateAndParseJWT
have (string, string, string)
want (context.Context, string, string, string)
auth/argus_id_test.go:49:53: not enough arguments in call to validateAndParseJWT
have (string, string, string)
want (context.Context, string, string, string)
auth/argus_id_test.go:68:51: not enough arguments in call to validateAndParseJWT
have (string, string, string)
want (context.Context, string, string, string)
auth/argus_id_test.go:88:51: not enough arguments in call to validateAndParseJWT
have (string, string, string)
want (context.Context, string, string, string)
auth/argus_id_test.go:100:51: not enough arguments in call to validateAndParseJWT
have (string, string, string)
want (context.Context, string, string, string)
(typecheck)
🪛 GitHub Check: Lint (go)
relay/nakama/auth/argus_id.go
[failure] 1-1:
: # pkg.world.dev/world-engine/relay/nakama/auth [pkg.world.dev/world-engine/relay/nakama/auth.test]
🔇 Additional comments (6)
relay/nakama/auth/custom.go (1)
34-34: 🧹 Nitpick (assertive)
Clarify Warning Message to Reflect Both Authentication and Linking
The warning message in checkJWTSecret mentions only the custom link, but since this function is used for both authentication and linking, consider updating the message to include both.
Apply this diff to update the log message:
- logger.Warn("JWT secret isn't set. You won't be able to use Argus ID custom link")
+ logger.Warn("JWT secret isn't set. Argus ID custom authentication and linking will not be available")relay/nakama/auth/argus_id.go (5)
58-59: 🛠️ Refactor suggestion
Use Constant-Time Comparison for Hash Verification
To prevent timing attacks during hash comparison, consider using subtle.ConstantTimeCompare from the crypto/subtle package instead of the != operator.
Apply this diff to improve security:
+import "crypto/subtle"
// ...
- if computedHashString != jwtHash {
+ if subtle.ConstantTimeCompare([]byte(computedHashString), []byte(jwtHash)) != 1 {
return nil, ErrInvalidIDForJWT
}119-119: 🛠️ Refactor suggestion
Avoid Logging Potentially Sensitive User Metadata Values
Logging the actual values of user metadata may expose sensitive information. Consider logging only the keys or a generic message to avoid potential leakage of sensitive data.
Apply this diff to modify the log statement:
- logger.Warn("Found non-string value in user metadata: %v", value)
+ logger.Warn("Found non-string value in user metadata for key: %s", key)106-109: 🧹 Nitpick (assertive)
Enhance Error Handling for JWT Validation
When calling claims.Valid(), you might encounter specific validation errors such as token expiration. Providing detailed error messages can help users understand why authentication failed.
Apply this diff to improve error handling:
if err = claims.Valid(); err != nil {
- _, err = utils.LogErrorWithMessageAndCode(logger, err, codes.InvalidArgument, "JWT is not valid")
+ if validationErr, ok := err.(*jwt.ValidationError); ok {
+ switch {
+ case validationErr.Errors&jwt.ValidationErrorExpired != 0:
+ _, err = utils.LogErrorWithMessageAndCode(logger, err, codes.InvalidArgument, "JWT token is expired")
+ default:
+ _, err = utils.LogErrorWithMessageAndCode(logger, err, codes.InvalidArgument, "JWT token is invalid")
+ }
+ } else {
+ _, err = utils.LogErrorWithMessageAndCode(logger, err, codes.InvalidArgument, "Failed to validate JWT claims")
+ }
return nil, err
}Also, apply similar changes in linkWithArgusID.
Likely invalid or redundant comment.
150-152:
Initialize in.Vars to Prevent Nil Map Assignment
Similarly, if in.Vars is nil, assigning values to it will cause a runtime panic. Initialize in.Vars before populating it with user metadata.
Apply this diff to ensure in.Vars is initialized:
+ if in.Vars == nil {
+ in.Vars = make(map[string]string)
+ }
for key, value := range claims.UserMetaData {
if strValue, ok := value.(string); ok {
in.Vars[key] = strValueLikely invalid or redundant comment.
115-117:
Initialize in.Account.Vars to Prevent Nil Map Assignment
If in.Account.Vars is nil, assigning values to it will cause a runtime panic. Initialize in.Account.Vars before populating it with user metadata.
Apply this diff to ensure in.Account.Vars is initialized:
+ if in.Account.Vars == nil {
+ in.Account.Vars = make(map[string]string)
+ }
for key, value := range claims.UserMetaData {
if strValue, ok := value.(string); ok {
in.Account.Vars[key] = strValueLikely invalid or redundant comment.
rmrt1n
force-pushed
the
ryan/world-1254-nakama-argus-id
branch
from
54557af to
bd8513a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
relay/nakama/go.modis excluded by!**/*.modrelay/nakama/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
relay/nakama/auth/argus_id.go(1 hunks)relay/nakama/auth/argus_id_test.go(1 hunks)relay/nakama/auth/custom.go(3 hunks)relay/nakama/auth/siwe.go(4 hunks)relay/nakama/main.go(1 hunks)
🧰 Additional context used
📓 Learnings (3)
relay/nakama/auth/argus_id_test.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id_test.go:34-50
Timestamp: 2024-11-21T16:47:32.434Z
Learning: In Go tests in `relay/nakama/auth/argus_id_test.go`, when testing the `validateAndParseJWT` function, it's acceptable to use a simple invalid hash string like `"invalidhashvalue"` for `wrongHash`, and it's unnecessary to use actual SHA256 hashes of different JWTs.
relay/nakama/auth/argus_id.go (1)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/argus_id.go:159-165
Timestamp: 2024-11-21T17:08:28.410Z
Learning: In the function `linkWithArgusID` within `relay/nakama/auth/argus_id.go`, the `Vars` field of `api.AccountCustom` (`in.Vars`) is initialized to an empty map by default. Therefore, explicit initialization of `in.Vars` is unnecessary.
relay/nakama/auth/custom.go (2)
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:29-36
Timestamp: 2024-11-21T17:33:33.067Z
Learning: In `relay/nakama/auth/custom.go`, the `checkJWTSecret` function is not called concurrently, so accessing and setting `GlobalJWTSecret` does not cause data races in this context.
Learnt from: rmrt1n
PR: Argus-Labs/world-engine#815
File: relay/nakama/auth/custom.go:55-55
Timestamp: 2024-11-21T17:05:28.598Z
Learning: In `handleCustomAuthentication` in `relay/nakama/auth/custom.go`, nil pointer checks for `in.GetAccount()` and `in.GetAccount().GetVars()` are handled in `validateAndParseJWT`, so explicit nil checks are not needed.
🔇 Additional comments (6)
relay/nakama/auth/custom.go (2)
34-34: 🧹 Nitpick (assertive)
Clarify the log message to reflect both authentication and linking
The warning message in checkJWTSecret mentions only "Argus ID custom link". Since this function is used for both authentication and linking, consider updating the message to reflect that both functionalities are affected.
Apply this diff to update the log message:
- logger.Warn("JWT secret isn't set. You won't be able to use Argus ID custom link")
+ logger.Warn("JWT secret isn't set. Argus ID custom authentication and linking will not be available")69-69: 🧹 Nitpick (assertive)
Use sub-spans instead of events for handling authentication methods
Instead of adding events to the main span for handling SIWE and Argus ID authentication, consider creating sub-spans to provide better trace granularity and insights.
You can create sub-spans as follows:
// For SIWE authentication
_, siweSpan := otel.Tracer("nakama.auth").Start(ctx, "AuthenticateWithSIWE")
defer siweSpan.End()
// ... existing SIWE authentication code ...
// For Argus ID authentication
_, argusSpan := otel.Tracer("nakama.auth").Start(ctx, "AuthenticateWithArgusID")
defer argusSpan.End()
// ... existing Argus ID authentication code ...Also applies to: 80-80
relay/nakama/auth/argus_id.go (1)
50-50:
Use constant-time comparison to prevent timing attacks
For security reasons, comparing hashes using != may be susceptible to timing attacks. Consider using subtle.ConstantTimeCompare from the crypto/subtle package for secure, constant-time comparisons.
Apply this diff to enhance security:
+import (
+ "crypto/subtle"
+ // ... other imports ...
+)
...
if subtle.ConstantTimeCompare([]byte(computedHashString), []byte(jwtHash)) != 1 {
span.RecordError(ErrInvalidIDForJWT)
span.SetStatus(otelcode.Error, "Given JWT hash does not match computed hash")
return nil, ErrInvalidIDForJWT
}Likely invalid or redundant comment.
relay/nakama/auth/argus_id_test.go (1)
1-107: Unit tests provide good coverage for JWT validation
The test cases effectively cover various scenarios, ensuring the reliability of the JWT authentication mechanisms.
relay/nakama/main.go (2)
133-134: LGTM! Consider adding trace spans.
The logger parameter addition is consistent with the authentication changes. Given the PR objectives mention OpenTelemetry tracing requirements, consider adding trace spans around the link initialization.
129-130: 🧹 Nitpick (assertive)
LGTM! Consider adding trace spans.
The logger parameter addition aligns with the authentication changes. Given the PR objectives mention OpenTelemetry tracing, consider adding trace spans around the authentication initialization.
+ ctx, span := tracer.Start(ctx, "init_custom_auth")
+ defer span.End()
if err := auth.InitCustomAuthentication(logger, initializer); err != nil {
return eris.Wrap(err, "failed to init custom authentication")
}Likely invalid or redundant comment.
Closes: WORLD-1254
Overview
This PR adds the handlers for custom authentication and linking using Argus ID. ATM, only us (Argus Labs) can use this custom authentication as it requires a symmetric JWT secret key to verify the JWTs issued by Supabase. Once asymmetric JWTs are rolled out, third parties should be able to use it too.
There's a slight change from the original design document. Instead of using the JWTs as the IDs for
AuthenticateCustom, we're using the SHA256 hash of the JWT as the ID and including the JWT in the request variables. This change is made becauseAuthenticateCustomhas an ID max length limit of 128 characters, which isn't long enough to contain the entirety of the JWT.Brief Changelog
'argus'and the handlers for authentication and linking.Testing and Verifying
Added unit tests to validate the JWT verifying and parsing logic.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests