chore: add authorization logic to attestation#1226
Conversation
- Added permission validation in the request_attestation action to ensure only wallets with the 'system:network_writer' role can request attestations. - Enhanced tests to cover scenarios for both authorized and unauthorized users, ensuring proper error handling for permission violations. - Updated the AttestationTestHelper to grant the network_writer role to the deployer by default, facilitating smoother test setups. These changes improve the security and integrity of the attestation request process by enforcing role-based access control.
WalkthroughAdded a permission check requiring the Changes
Sequence DiagramsequenceDiagram
actor User
participant Engine
participant DB as Database
participant RoleCheck as Role Validator
User->>Engine: request_attestation(args)
activate Engine
Engine->>Engine: Capture transaction ID
rect rgb(220, 240, 255)
Note over Engine,RoleCheck: NEW: Permission Check
Engine->>RoleCheck: Check if caller has<br/>system:network_writer role
RoleCheck->>DB: Query are_members_of()
DB-->>RoleCheck: Member status
alt Role Not Found / Inactive
RoleCheck-->>Engine: Authorization Error
Engine-->>User: ❌ Missing Role Error
else Role Verified & Active
RoleCheck-->>Engine: ✓ Authorized
end
end
rect rgb(240, 255, 240)
Note over Engine,DB: Existing Flow
Engine->>Engine: Validate encryption flag
Engine->>Engine: Check action allowlist
Engine->>DB: Execute & attest
DB-->>Engine: ✓ Success
end
Engine-->>User: ✓ Attestation Created
deactivate Engine
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Changes are focused and follow a consistent pattern across files. The permission check logic is straightforward validation applied early in the request flow. Test additions clearly mirror the implementation pattern (authorized + unauthorized paths). Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/migrations/024-attestation-actions.sql (1)
25-37: Permission check is correctly placed and secure.The authorization logic correctly:
- Executes before validation and expensive operations (good fail-fast placement)
- Normalizes the caller address for case-insensitive comparison
- Provides a clear error message without leaking sensitive information
The loop-and-break pattern is functional, though it could optionally be simplified. Consider this alternative for slightly cleaner code:
- -- Permission Check: Ensure caller has the 'system:network_writer' role. - $lower_caller TEXT := LOWER(@caller); - $has_permission BOOL := false; - for $row in are_members_of('system', 'network_writer', ARRAY[$lower_caller]) { - if $row.wallet = $lower_caller AND $row.is_member { - $has_permission := true; - break; - } - } - if NOT $has_permission { - ERROR('Caller does not have the required system:network_writer role to request attestation.'); - } + -- Permission Check: Ensure caller has the 'system:network_writer' role. + $lower_caller TEXT := LOWER(@caller); + $has_permission BOOL := EXISTS( + SELECT 1 FROM are_members_of('system', 'network_writer', ARRAY[$lower_caller]) + WHERE wallet = $lower_caller AND is_member + ); + if NOT $has_permission { + ERROR('Caller does not have the required system:network_writer role to request attestation.'); + }tests/streams/attestation/attestation_request_test.go (1)
135-171: LGTM! Thorough authorization test.The unauthorized user test correctly:
- Creates a user without the
network_writerrole- Attempts to request attestation with otherwise valid parameters
- Verifies the error is returned at the action level (not engine level)
- Checks for the specific authorization error message
The test provides good coverage of the negative authorization path. Consider optionally adding a database assertion to verify no attestation row was created, though the early error in the SQL function makes this implicit:
// Optional: Verify no attestation was created var count int err = platform.Engine.Execute(helper.NewEngineContext(), platform.DB, `SELECT COUNT(*) FROM attestations WHERE requester = $req`, map[string]any{"req": unauthorizedAddr.Bytes()}, func(row *common.Row) error { count = int(row.Values[0].(int64)) return nil }) require.NoError(t, err) require.Equal(t, 0, count, "no attestation should be created for unauthorized user")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/migrations/024-attestation-actions.sql(2 hunks)tests/streams/attestation/attestation_request_test.go(2 hunks)tests/streams/attestation/test_helpers.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/streams/attestation/attestation_request_test.go (2)
tests/streams/attestation/test_helpers.go (4)
TestActionIDRequest(25-25)AttestationTestHelper(69-73)TestDataProviderHex(29-29)TestStreamID(28-28)extensions/tn_utils/serialization.go (1)
EncodeActionArgs(45-76)
tests/streams/attestation/test_helpers.go (1)
tests/streams/utils/setup/roles.go (1)
AddMemberToRoleBypass(73-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (5)
internal/migrations/024-attestation-actions.sql (1)
8-10: LGTM!The permission documentation is clear and accurately describes the new authorization requirement.
tests/streams/attestation/test_helpers.go (3)
77-88: LGTM! Smart default for test setup.Automatically granting the
network_writerrole to the deployer inNewAttestationTestHelperis a good design choice that:
- Simplifies the happy path for most tests
- Reduces boilerplate in existing tests
- Keeps unauthorized test scenarios explicit (requiring manual setup)
This aligns well with the PR's goal of adding authorization as an interim protection measure.
215-219: LGTM!The
GrantNetworkWriterRolemethod provides a clean interface for test setup. UsingAddMemberToRoleBypasswith authorization override is appropriate for test helpers.
223-224: LGTM! Good defensive test design.Granting the
network_writerrole before requesting attestation inCreateAttestationForRequesterensures that tests using this helper don't fail due to authorization issues unless they're specifically testing authorization.tests/streams/attestation/attestation_request_test.go (1)
37-43: LGTM! Clean test organization.The subtest structure clearly separates the happy path from the authorization test case, making test intent explicit and improving test output readability.
Time Submission Status
|
2 participants
Description
Related Problem
How Has This Been Tested?
Summary by CodeRabbit
Security
Tests