-
Notifications
You must be signed in to change notification settings - Fork 54
feat: implement session key authorization on execute #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
011e967
e6e5dac
621f666
4a66692
344938d
8cc411c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -126,20 +126,45 @@ impl AncoreAccount { | |||||||||||||||||
|
|
||||||||||||||||||
| /// Get the current nonce | ||||||||||||||||||
| pub fn get_nonce(env: Env) -> Result<u64, ContractError> { | ||||||||||||||||||
| Ok(env.storage() | ||||||||||||||||||
| Ok(env | ||||||||||||||||||
| .storage() | ||||||||||||||||||
| .instance() | ||||||||||||||||||
| .get(&DataKey::Nonce) | ||||||||||||||||||
| .unwrap_or(0)) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Execute a transaction: validate nonce, perform cross-contract call, increment nonce. | ||||||||||||||||||
| /// Execute a transaction with nonce replay-protection and dual auth paths. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Security | ||||||||||||||||||
| /// - Caller must be owner (session key auth not yet wired) | ||||||||||||||||||
| /// - `expected_nonce` must match current nonce (replay protection) | ||||||||||||||||||
| /// - Nonce is incremented only after a successful invocation | ||||||||||||||||||
| /// # Auth paths | ||||||||||||||||||
| /// - **Owner path**: `caller` == stored owner → `caller.require_auth()` is sufficient. | ||||||||||||||||||
| /// - **Session-key path**: `caller` != owner → | ||||||||||||||||||
| /// 1. `session_key` must be `Some(public_key)` that was previously registered. | ||||||||||||||||||
| /// 2. The session key must not be expired (`expires_at > current ledger timestamp`). | ||||||||||||||||||
| /// 3. The session key's `permissions` list must contain `required_permission`. | ||||||||||||||||||
| /// 4. `caller.require_auth()` is still called so Soroban validates the caller's | ||||||||||||||||||
| /// signature over this invocation. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Nonce | ||||||||||||||||||
| /// `expected_nonce` must equal the current nonce stored in the contract. | ||||||||||||||||||
| /// The nonce is incremented **after** all checks pass, preventing replays. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Parameters | ||||||||||||||||||
| /// - `caller` – Address of the entity authorising the call (owner or session-key holder). | ||||||||||||||||||
| /// - `to` – Target contract address. | ||||||||||||||||||
| /// - `function` – Name of the function to invoke on `to`. | ||||||||||||||||||
| /// - `_args` – Arguments forwarded to the cross-contract call (reserved; not yet executed). | ||||||||||||||||||
| /// - `expected_nonce` – Caller's view of the current nonce; must match exactly. | ||||||||||||||||||
| /// - `required_permission` – Permission code the session key must carry (ignored for owner). | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Errors | ||||||||||||||||||
| /// - [`ContractError::NotInitialized`] – Contract has no owner yet. | ||||||||||||||||||
| /// - [`ContractError::InvalidNonce`] – `expected_nonce` does not match stored nonce. | ||||||||||||||||||
| /// - [`ContractError::SessionKeyNotFound`] – No session key exists for the supplied public key. | ||||||||||||||||||
| /// - [`ContractError::SessionKeyExpired`] – Session key's `expires_at` ≤ current ledger timestamp. | ||||||||||||||||||
| /// - [`ContractError::InsufficientPermission`] – Session key does not carry `required_permission`. | ||||||||||||||||||
| pub fn execute( | ||||||||||||||||||
| env: Env, | ||||||||||||||||||
| caller: Address, | ||||||||||||||||||
| to: Address, | ||||||||||||||||||
| function: soroban_sdk::Symbol, | ||||||||||||||||||
| _args: Vec<soroban_sdk::Val>, | ||||||||||||||||||
|
|
@@ -149,18 +174,19 @@ impl AncoreAccount { | |||||||||||||||||
| // TODO: Execute call | ||||||||||||||||||
|
|
||||||||||||||||||
| let owner = Self::get_owner(env.clone())?; | ||||||||||||||||||
| owner.require_auth(); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Get nonce before incrementing | ||||||||||||||||||
| // ── Nonce check (before any auth so we fail fast on replays) ───────── | ||||||||||||||||||
| let current_nonce: u64 = Self::get_nonce(env.clone())?; | ||||||||||||||||||
|
|
||||||||||||||||||
| if expected_nonce != current_nonce { | ||||||||||||||||||
| panic!("Invalid nonce"); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| // Increment nonce | ||||||||||||||||||
| env.storage().instance().set(&DataKey::Nonce, &(current_nonce + 1)); | ||||||||||||||||||
| // ── Increment nonce ─────────────────────────────────────────────────── | ||||||||||||||||||
| env.storage() | ||||||||||||||||||
| .instance() | ||||||||||||||||||
| .set(&DataKey::Nonce, &(current_nonce + 1)); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Extend instance TTL to keep contract alive | ||||||||||||||||||
| env.storage().instance().extend_ttl(INSTANCE_BUMP_THRESHOLD, INSTANCE_BUMP_AMOUNT); | ||||||||||||||||||
|
|
@@ -265,7 +291,33 @@ impl AncoreAccount { | |||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||
| mod test { | ||||||||||||||||||
| use super::*; | ||||||||||||||||||
| use soroban_sdk::{testutils::{Address as _, Events}, Address, Env}; | ||||||||||||||||||
| use soroban_sdk::{ | ||||||||||||||||||
| testutils::{Address as _, Events, Ledger}, | ||||||||||||||||||
| Address, Env, | ||||||||||||||||||
| }; | ||||||||||||||||||
|
Comment on lines
+401
to
+405
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unused import Pipeline reports: 🧹 Proposed fix use soroban_sdk::{
- testutils::{Address as _, Events, Ledger},
+ testutils::{Address as _, Events},
Address, Env,
};📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: CI[warning] 295-295: Rust warning: unused import 🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| // ── Helpers ─────────────────────────────────────────────────────────────── | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Convenience: deploy + initialize the contract and return (client, owner). | ||||||||||||||||||
| fn setup() -> (Env, AncoreAccountClient<'static>, Address) { | ||||||||||||||||||
| let env = Env::default(); | ||||||||||||||||||
| let contract_id = env.register_contract(None, AncoreAccount); | ||||||||||||||||||
| // SAFETY: the client borrows from `env`; we keep both alive by returning them together. | ||||||||||||||||||
| // Using a raw pointer cast is the standard pattern for Soroban test helpers when the | ||||||||||||||||||
| // lifetime would otherwise force splitting setup across every test. | ||||||||||||||||||
| let client = AncoreAccountClient::new(&env, &contract_id); | ||||||||||||||||||
| // We need 'static lifetime for the client in our return type. | ||||||||||||||||||
| // This is safe because `env` is returned alongside and kept alive. | ||||||||||||||||||
| // Standard soroban test pattern: use Box::leak or simply re-create per test. | ||||||||||||||||||
| // Since Soroban clients are cheap to create we just rebuild in each helper instead. | ||||||||||||||||||
| let owner = Address::generate(&env); | ||||||||||||||||||
| client.initialize(&owner); | ||||||||||||||||||
| (env, client, owner) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // ───────────────────────────────────────────────────────────────────────── | ||||||||||||||||||
| // Existing tests (updated call sites for new execute() signature) | ||||||||||||||||||
| // ───────────────────────────────────────────────────────────────────────── | ||||||||||||||||||
|
|
||||||||||||||||||
| #[test] | ||||||||||||||||||
| fn test_initialize() { | ||||||||||||||||||
|
|
@@ -289,17 +341,15 @@ mod test { | |||||||||||||||||
| let owner = Address::generate(&env); | ||||||||||||||||||
| client.initialize(&owner); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Check that an event was published | ||||||||||||||||||
| let events_list = env.events().all(); | ||||||||||||||||||
| assert_eq!(events_list.len(), 1); | ||||||||||||||||||
| let (_contract, topics, data) = events_list.get_unchecked(0).clone(); | ||||||||||||||||||
| assert_eq!(topics.len(), 1); | ||||||||||||||||||
| // Convert topic to Symbol for comparison | ||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
|
|
||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = | ||||||||||||||||||
| soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
| assert_eq!(topic_symbol, events::initialized(&env)); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Verify owner address is in the event data | ||||||||||||||||||
|
|
||||||||||||||||||
| let event_owner: Address = soroban_sdk::FromVal::from_val(&env, &data); | ||||||||||||||||||
| assert_eq!(event_owner, owner); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -342,17 +392,15 @@ mod test { | |||||||||||||||||
|
|
||||||||||||||||||
| client.add_session_key(&session_pk, &expires_at, &permissions); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Find the session_key_added event (skip the initialize event) | ||||||||||||||||||
| let events_list = env.events().all(); | ||||||||||||||||||
| assert!(events_list.len() >= 2); | ||||||||||||||||||
| let (_contract, topics, data) = events_list.get_unchecked(1).clone(); | ||||||||||||||||||
| assert_eq!(topics.len(), 1); | ||||||||||||||||||
| // Convert topic to Symbol for comparison | ||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
|
|
||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = | ||||||||||||||||||
| soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
| assert_eq!(topic_symbol, events::session_key_added(&env)); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Verify data is (public_key, expires_at) | ||||||||||||||||||
|
|
||||||||||||||||||
| let data_tuple: (BytesN<32>, u64) = soroban_sdk::FromVal::from_val(&env, &data); | ||||||||||||||||||
| assert_eq!(data_tuple.0, session_pk); | ||||||||||||||||||
| assert_eq!(data_tuple.1, expires_at); | ||||||||||||||||||
|
|
@@ -376,21 +424,20 @@ mod test { | |||||||||||||||||
| client.add_session_key(&session_pk, &expires_at, &permissions); | ||||||||||||||||||
| client.revoke_session_key(&session_pk); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Find the session_key_revoked event (should be the last event) | ||||||||||||||||||
| let events_list = env.events().all(); | ||||||||||||||||||
| assert!(events_list.len() >= 3); | ||||||||||||||||||
| let (_contract, topics, data) = events_list.get_unchecked(2).clone(); | ||||||||||||||||||
| assert_eq!(topics.len(), 1); | ||||||||||||||||||
| // Convert topic to Symbol for comparison | ||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
|
|
||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = | ||||||||||||||||||
| soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
| assert_eq!(topic_symbol, events::session_key_revoked(&env)); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Verify public_key is in the event data | ||||||||||||||||||
|
|
||||||||||||||||||
| let event_pk: BytesN<32> = soroban_sdk::FromVal::from_val(&env, &data); | ||||||||||||||||||
| assert_eq!(event_pk, session_pk); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Owner can execute; event is emitted with correct (to, function, nonce=0). | ||||||||||||||||||
| #[test] | ||||||||||||||||||
| fn test_execute_emits_event() { | ||||||||||||||||||
| let env = Env::default(); | ||||||||||||||||||
|
|
@@ -404,26 +451,24 @@ mod test { | |||||||||||||||||
|
|
||||||||||||||||||
| let to = Address::generate(&env); | ||||||||||||||||||
| let function = soroban_sdk::Symbol::new(&env, "transfer"); | ||||||||||||||||||
| let args = Vec::new(&env); | ||||||||||||||||||
| let args: Vec<soroban_sdk::Val> = Vec::new(&env); | ||||||||||||||||||
|
|
||||||||||||||||||
| client.execute(&to, &function, &args, &0u64); | ||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
|
|
||||||||||||||||||
| // Find the executed event (skip the initialize event) | ||||||||||||||||||
| let events_list = env.events().all(); | ||||||||||||||||||
| assert!(events_list.len() >= 2); | ||||||||||||||||||
| let (_contract, topics, data) = events_list.get_unchecked(1).clone(); | ||||||||||||||||||
| assert_eq!(topics.len(), 1); | ||||||||||||||||||
| // Convert topic to Symbol for comparison | ||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
|
|
||||||||||||||||||
| let topic_symbol: soroban_sdk::Symbol = | ||||||||||||||||||
| soroban_sdk::FromVal::from_val(&env, &topics.get_unchecked(0)); | ||||||||||||||||||
| assert_eq!(topic_symbol, events::executed(&env)); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Verify data is (to, function, nonce) | ||||||||||||||||||
|
|
||||||||||||||||||
| let data_tuple: (Address, soroban_sdk::Symbol, u64) = | ||||||||||||||||||
| soroban_sdk::FromVal::from_val(&env, &data); | ||||||||||||||||||
| assert_eq!(data_tuple.0, to); | ||||||||||||||||||
| assert_eq!(data_tuple.1, function); | ||||||||||||||||||
| assert_eq!(data_tuple.2, 0); // First execution, nonce should be 0 | ||||||||||||||||||
| assert_eq!(data_tuple.2, 0); // nonce before increment | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| #[test] | ||||||||||||||||||
|
|
@@ -438,8 +483,9 @@ mod test { | |||||||||||||||||
| client.initialize(&owner); // Should panic with contract error #1 | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Passing expected_nonce = 1 when current nonce is 0 must be rejected. | ||||||||||||||||||
| #[test] | ||||||||||||||||||
| #[should_panic(expected = "Invalid nonce")] | ||||||||||||||||||
| #[should_panic(expected = "Error(Contract, #4)")] | ||||||||||||||||||
| fn test_execute_rejects_invalid_nonce() { | ||||||||||||||||||
| let env = Env::default(); | ||||||||||||||||||
| let contract_id = env.register_contract(None, AncoreAccount); | ||||||||||||||||||
|
|
@@ -452,12 +498,13 @@ mod test { | |||||||||||||||||
|
|
||||||||||||||||||
| let to = Address::generate(&env); | ||||||||||||||||||
| let function = soroban_sdk::symbol_short!("transfer"); | ||||||||||||||||||
| let args = Vec::new(&env); | ||||||||||||||||||
| let args: Vec<soroban_sdk::Val> = Vec::new(&env); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Current nonce is 0; passing expected_nonce = 1 should panic Invalid nonce | ||||||||||||||||||
| client.execute(&to, &function, &args, &1u64); | ||||||||||||||||||
| // Nonce is 0; passing 1 must panic with InvalidNonce (#4). | ||||||||||||||||||
| client.execute(&owner, &to, &function, &args, &None, &1u64, &0u32); | ||||||||||||||||||
| } | ||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
|
|
||||||||||||||||||
| /// Correct nonce is accepted and incremented to 1 afterward. | ||||||||||||||||||
| #[test] | ||||||||||||||||||
| fn test_execute_validates_nonce_then_increments() { | ||||||||||||||||||
| let env = Env::default(); | ||||||||||||||||||
|
|
@@ -471,14 +518,11 @@ mod test { | |||||||||||||||||
|
|
||||||||||||||||||
| env.mock_all_auths(); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Deploy a trivial contract that returns a Val so we can invoke it | ||||||||||||||||||
| let callee_id = env.register_contract(None, AncoreAccount); | ||||||||||||||||||
| let to = callee_id; | ||||||||||||||||||
| let to = Address::generate(&env); | ||||||||||||||||||
| let function = soroban_sdk::symbol_short!("get_nonce"); | ||||||||||||||||||
| let args = Vec::new(&env); | ||||||||||||||||||
| let args: Vec<soroban_sdk::Val> = Vec::new(&env); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Execute with expected_nonce = 0 (matches current); invokes get_nonce on callee | ||||||||||||||||||
| let _result = client.execute(&to, &function, &args, &0u64); | ||||||||||||||||||
| client.execute(&owner, &to, &function, &args, &None, &0u64, &0u32); | ||||||||||||||||||
|
|
||||||||||||||||||
| assert_eq!(client.get_nonce(), 1); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.