diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a29be997d..97806eef5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ All notable changes to this project will be documented in this file. ### Changes - +- Serviceability + - Ten new instructions remove an access pass or a user for one specific `AccessPassType`: `ClosePrepaidAccessPass`, `CloseSolanaValidatorAccessPass`, `CloseSolanaRPCAccessPass`, `CloseOthersAccessPass`, `CloseEdgeSeatAccessPass`, and `DeletePrepaidUser`, `DeleteSolanaValidatorUser`, `DeleteSolanaRPCUser`, `DeleteOthersUser`, `DeleteEdgeSeatUser`. Each reads the access pass and refuses with `AccessPassTypeMismatch` unless the pass matches the instruction. `CloseAccessPass` and `DeleteUser` keep working unchanged; a follow-up change deprecates them and moves every caller. (#2470) - SDK - The TypeScript and Python `GlobalState` deserializers expose `ip_verifier_authority_pk`, the RFC-27 trust root the Go SDK and the Rust state already carried, so those consumers can read which key signs IP ownership proofs. The field is appended, so an account written before the upgrade decodes it as the default pubkey rather than failing. (#4231) - CI diff --git a/docs/superpowers/specs/2026-09-01-per-pass-type-removal-guard-design.md b/docs/superpowers/specs/2026-09-01-per-pass-type-removal-guard-design.md new file mode 100644 index 0000000000..acba4013d8 --- /dev/null +++ b/docs/superpowers/specs/2026-09-01-per-pass-type-removal-guard-design.md @@ -0,0 +1,234 @@ +# Design: one close and delete instruction per access pass type + +Date: 2026-09-01 +Issue: [malbeclabs/infra#2470](https://github.com/malbeclabs/infra/issues/2470) +Parent: [malbeclabs/infra#2385](https://github.com/malbeclabs/infra/issues/2385) + +## Problem + +Serviceability has one `CloseAccessPass` instruction (variant 69) and one `DeleteUser` +instruction (variant 42). Neither handler looks at `AccessPassType`. Both destroy state. A +caller that means to remove a prepaid pass can remove an EdgeSeat pass by mistake, and the +program accepts it. + +Issue #2470 asks for one instruction per pass type, and states that the pass type must not be +an instruction argument. An earlier version of this document proposed a declared type in the +args instead. Martin rejected that in the issue. This document follows the issue. + +## Shape + +`AccessPassType` has 5 variants, so there are 10 new instructions: + +| Variant | Name | Replaces | +| --- | --- | --- | +| 119 | `ClosePrepaidAccessPass` | `CloseAccessPass` | +| 120 | `CloseSolanaValidatorAccessPass` | `CloseAccessPass` | +| 121 | `CloseSolanaRPCAccessPass` | `CloseAccessPass` | +| 122 | `CloseOthersAccessPass` | `CloseAccessPass` | +| 123 | `CloseEdgeSeatAccessPass` | `CloseAccessPass` | +| 124 | `DeletePrepaidUser` | `DeleteUser` | +| 125 | `DeleteSolanaValidatorUser` | `DeleteUser` | +| 126 | `DeleteSolanaRPCUser` | `DeleteUser` | +| 127 | `DeleteOthersUser` | `DeleteUser` | +| 128 | `DeleteEdgeSeatUser` | `DeleteUser` | + +The highest variant in use today is 118. + +## Components + +### 1. `AccessPassKind` + +A tag enum in `smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs`, +next to `AccessPassType`. + +```rust +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum AccessPassKind { + Prepaid, + SolanaValidator, + SolanaRPC, + Others, + EdgeSeat, +} + +impl From<&AccessPassType> for AccessPassKind { /* one arm per variant */ } +impl fmt::Display for AccessPassKind { /* for the error message */ } +``` + +This type never reaches the wire. It is not part of any instruction argument, and it needs no +Borsh derive. It exists so the shared handler body can take the kind it must accept, and so the +CLI and the SDKs have one name for the choice they make. + +`AccessPassType` carries payloads (a `Pubkey`, a `String` pair, a `Vec`). +`AccessPassKind` carries none, so it is `Copy` and cheap to compare. + +The kind check compares the variant only. `AccessPassType::Others(type_name, key)` maps to +`Others` whatever `type_name` holds, so `CloseOthersAccessPass` closes any `Others` pass. +Pinning `type_name` would tie the instruction set to catalog data, so it is left out. + +### 2. The handler bodies stay single + +Ten instructions, two bodies. Close and delete take the same accounts for every kind, so the +work does not differ by kind. Only the accepted kind differs. + +`processors/accesspass/close.rs`: + +```rust +pub fn process_close_access_pass( + program_id: &Pubkey, + accounts: &[AccountInfo], + value: &CloseAccessPassArgs, + expected: AccessPassKind, +) -> ProgramResult +``` + +`processors/user/delete.rs` gains the same trailing `expected: AccessPassKind` parameter. + +Each body checks the kind right after it reads the `AccessPass`: + +```rust +let actual = AccessPassKind::from(&accesspass.accesspass_type); +if actual != expected { + msg!("instruction is for {expected} but the pass is {actual}"); + return Err(DoubleZeroError::AccessPassTypeMismatch.into()); +} +``` + +`DoubleZeroError::AccessPassTypeMismatch` is a new variant, number 119. The highest number in +use today is 118. + +The kind is a Rust parameter chosen by the dispatch arm, not an instruction argument. Ten +copies of a 250 line body would be ten places to fix the next bug in it. + +### 3. Dispatch + +`entrypoint.rs` gets 10 arms. Each names its kind: + +```rust +DoubleZeroInstruction::ClosePrepaidAccessPass(value) => { + process_close_access_pass(program_id, accounts, &value, AccessPassKind::Prepaid)? +} +DoubleZeroInstruction::CloseSolanaValidatorAccessPass(value) => { + process_close_access_pass(program_id, accounts, &value, AccessPassKind::SolanaValidator)? +} +// ... 8 more +``` + +### 4. The arguments do not change + +All 5 close instructions carry the existing `CloseAccessPassArgs`, which is empty. All 5 delete +instructions carry the existing `UserDeleteArgs`, which holds `dz_prefix_count` and +`multicast_publisher_count`. No new argument types, and no new incremental defaults to get +wrong. + +No account layout changes, so the SDK fixtures do not need regeneration. + +### 5. Variants 69 and 42 are replaced + +They follow the pattern already used for variants 72, 75, 77 and 78. Each loses its payload and +joins the deprecated arm in `entrypoint.rs`: + +```rust +CloseAccessPass(), // variant 69, deprecated: use CloseAccessPass. See #2470. +DeleteUser(), // variant 42, deprecated: use DeleteUser. See #2470. +``` + +Both return `DoubleZeroError::Deprecated`. The discriminants stay reserved, and a caller that +does not upgrade gets a named error rather than a silent removal. + +### 6. Callers + +| Caller | Change | +| --- | --- | +| `doublezero access-pass close` | new required flag `--type ` | +| `doublezero user delete` | new required flag `--access-pass-type`, same values | +| `CloseAccessPassCommand`, `DeleteUserCommand` (Rust SDK) | new required `kind: AccessPassKind` field; picks the instruction | +| `doublezero-serviceability-instruction` | `close_access_pass` and `delete_user` take the kind and build the matching variant | +| `doublezero disconnect` (client daemon) | fills the kind from the pass it already reads | +| `Executor.DeleteUser` (Go SDK) | new `AccessPassKind` parameter; picks the instruction number | + +**Open question, asked on the issue.** The CLI takes only `--pubkey` today. For the SDK command +to pick an instruction it needs the kind from somewhere. If it reads the pass and picks from +what it read, then the on-chain refusal can never fire, because the instruction was chosen from +the same byte it checks. The guard only holds if the operator states the kind. This design +therefore adds a required flag. If Martin wants no flag, the CLI path stays unguarded and the +flag comes out. + +Two callers read the kind rather than declaring it, and both are unavoidable: + +- `doublezero disconnect` in the client daemon. It is a self delete, and the handler already + checks the owner and the client IP, so the kind adds nothing there. Its `delete_users` loop + does not hold the pass, but the `LedgerClient` trait already exposes `get_accesspass`, and the + loop already has `client_ip` and `user.owner`, so the lookup is one line. +- `DeleteTenantCommand` with `allow_delete_users`. It sweeps every user under a tenant, and those + users can hold passes of different kinds, so no single declared kind exists. Forcing one would + turn "delete every user under this tenant" into "delete only users of one kind", which strands + the tenant record, because the command then waits for `reference_count` to reach 0. + +On both paths the program's refusal cannot fire: the value asserted and the value checked come +from the same account. The comment at each call site has to say so, or someone will copy the +pattern into a path where an operator could have declared the kind. + +### 7. A bug to fix in the code being touched + +`close.rs` wraps the account type check and the `connection_count` check in +`if let Ok(data) = accesspass_account.try_borrow_data()`. When the borrow fails, the handler +logs `Failed to borrow account data, cannot close` and then closes the pass anyway. Both checks +are skipped. + +The new kind check must not sit inside that block. The fix is to read the `AccessPass` once, +before the checks, and let a failed read return an error. This is a small change in a file the +work already edits. + +## Error handling + +| Case | Result | +| --- | --- | +| the instruction matches the stored pass | the removal proceeds as it does today | +| the instruction is for another kind | `AccessPassTypeMismatch`, nothing is written | +| variant 69 or 42 | `Deprecated`, nothing is written | +| the account data cannot be read | an error, and the pass is not closed | + +## Testing + +Program tests: + +- for each of the 5 kinds, one accepted close and one accepted delete; +- for each of the 5 kinds, one rejected close and one rejected delete against a pass of a + different kind; +- variant 69 and variant 42 both return `Deprecated`. + +Existing call sites move to the new instructions: `tests/accesspass_test.rs`, +`tests/user_tests.rs`, `tests/delete_user_dynamic_accesspass.rs`, `tests/user_old_test.rs`, +`tests/accesspass_allow_multiple_ip.rs`, `tests/create_subscribe_user_test.rs`, +`tests/multicastgroup_subscribe_test.rs`, `tests/user_onchain_allocation_test.rs`. + +CLI tests: the new flag is required, and an unknown value is rejected. + +Go SDK: `user_crud_test.go` passes the new parameter. + +## Rollout + +This breaks callers that do not upgrade. They get `Deprecated`. + +The program, the instruction crate, the Rust SDK, the CLI and the Go SDK land together in this +repository. + +The oracle lives in `doublezero-shreds`. Parent issue #2385 lists five places there that remove +users: `cleanup_orphaned_users`, the lapsed seat branch of `reconcile_validator_owned_users`, +`converge_retransmit_only_seats`, `process_instant_withdrawal_requests`, and +`access_pass_expiry`. Each one knows which class of user it cranks, so each one has a real kind +to name. That is where the risk in #2385 sits. It needs its own change, shipped with this +program deploy. + +## PR size + +The 10 enum variants touch 5 places each in `instructions.rs` (the enum, the decoder, the name, +the debug format, and the round trip test), plus 10 arms in `entrypoint.rs`. That is mechanical +but wide. If the total goes past the 500 line guideline in `CLAUDE.md`, the work splits into two +PRs: the program and the instruction crate first, then the CLI and the SDKs. + +## Out of scope + +Feed subscription has no access pass or user instructions. Issue #2470 records the rule for +future instructions there. No code changes. diff --git a/smartcontract/programs/doublezero-serviceability/src/entrypoint.rs b/smartcontract/programs/doublezero-serviceability/src/entrypoint.rs index c5dbfc6c5b..084045ccff 100644 --- a/smartcontract/programs/doublezero-serviceability/src/entrypoint.rs +++ b/smartcontract/programs/doublezero-serviceability/src/entrypoint.rs @@ -105,6 +105,7 @@ use crate::{ update::process_update_user, }, }, + state::accesspass::AccessPassKind, }; use solana_program::{ @@ -178,7 +179,28 @@ pub fn process_instruction( return Err(DoubleZeroError::Deprecated.into()); } DoubleZeroInstruction::DeleteUser(value) => { - process_delete_user(program_id, accounts, &value)? + process_delete_user(program_id, accounts, &value, None)? + } + DoubleZeroInstruction::DeletePrepaidUser(value) => { + process_delete_user(program_id, accounts, &value, Some(AccessPassKind::Prepaid))? + } + DoubleZeroInstruction::DeleteSolanaValidatorUser(value) => process_delete_user( + program_id, + accounts, + &value, + Some(AccessPassKind::SolanaValidator), + )?, + DoubleZeroInstruction::DeleteSolanaRPCUser(value) => process_delete_user( + program_id, + accounts, + &value, + Some(AccessPassKind::SolanaRPC), + )?, + DoubleZeroInstruction::DeleteOthersUser(value) => { + process_delete_user(program_id, accounts, &value, Some(AccessPassKind::Others))? + } + DoubleZeroInstruction::DeleteEdgeSeatUser(value) => { + process_delete_user(program_id, accounts, &value, Some(AccessPassKind::EdgeSeat))? } DoubleZeroInstruction::DeleteDevice(value) => { process_delete_device(program_id, accounts, &value)? @@ -306,7 +328,28 @@ pub fn process_instruction( process_set_access_pass(program_id, accounts, &value)? } DoubleZeroInstruction::CloseAccessPass(value) => { - process_close_access_pass(program_id, accounts, &value)? + process_close_access_pass(program_id, accounts, &value, None)? + } + DoubleZeroInstruction::ClosePrepaidAccessPass(value) => { + process_close_access_pass(program_id, accounts, &value, Some(AccessPassKind::Prepaid))? + } + DoubleZeroInstruction::CloseSolanaValidatorAccessPass(value) => process_close_access_pass( + program_id, + accounts, + &value, + Some(AccessPassKind::SolanaValidator), + )?, + DoubleZeroInstruction::CloseSolanaRPCAccessPass(value) => process_close_access_pass( + program_id, + accounts, + &value, + Some(AccessPassKind::SolanaRPC), + )?, + DoubleZeroInstruction::CloseOthersAccessPass(value) => { + process_close_access_pass(program_id, accounts, &value, Some(AccessPassKind::Others))? + } + DoubleZeroInstruction::CloseEdgeSeatAccessPass(value) => { + process_close_access_pass(program_id, accounts, &value, Some(AccessPassKind::EdgeSeat))? } DoubleZeroInstruction::CheckStatusAccessPass(value) => { process_check_status_access_pass(program_id, accounts, &value)? diff --git a/smartcontract/programs/doublezero-serviceability/src/error.rs b/smartcontract/programs/doublezero-serviceability/src/error.rs index b5b5ae01a9..c203c4b974 100644 --- a/smartcontract/programs/doublezero-serviceability/src/error.rs +++ b/smartcontract/programs/doublezero-serviceability/src/error.rs @@ -247,6 +247,8 @@ pub enum DoubleZeroError { IpProofMessageMismatch, // variant 117 #[error("IP ownership proof carries an unsupported layout version")] IpProofVersionUnsupported, // variant 118 + #[error("This instruction is for a different access pass type")] + AccessPassTypeMismatch, // variant 119 } impl From for ProgramError { @@ -371,6 +373,7 @@ impl From for ProgramError { DoubleZeroError::IpProofSignatureMismatch => ProgramError::Custom(116), DoubleZeroError::IpProofMessageMismatch => ProgramError::Custom(117), DoubleZeroError::IpProofVersionUnsupported => ProgramError::Custom(118), + DoubleZeroError::AccessPassTypeMismatch => ProgramError::Custom(119), } } } @@ -496,6 +499,7 @@ impl From for DoubleZeroError { 116 => DoubleZeroError::IpProofSignatureMismatch, 117 => DoubleZeroError::IpProofMessageMismatch, 118 => DoubleZeroError::IpProofVersionUnsupported, + 119 => DoubleZeroError::AccessPassTypeMismatch, _ => DoubleZeroError::Custom(e), } } @@ -519,6 +523,15 @@ mod tests { use super::*; use strum::IntoEnumIterator; + #[test] + fn test_access_pass_type_mismatch_roundtrip() { + let err = DoubleZeroError::AccessPassTypeMismatch; + assert_eq!(ProgramError::from(err.clone()), ProgramError::Custom(119)); + let pe: ProgramError = ProgramError::Custom(119); + let err2: DoubleZeroError = pe.into(); + assert_eq!(err2, err); + } + #[test] fn test_error_enum_conversions() { // Using EnumIter ensures all variants are tested - if a new variant is added diff --git a/smartcontract/programs/doublezero-serviceability/src/instructions.rs b/smartcontract/programs/doublezero-serviceability/src/instructions.rs index a68995bbaa..df6c14b13f 100644 --- a/smartcontract/programs/doublezero-serviceability/src/instructions.rs +++ b/smartcontract/programs/doublezero-serviceability/src/instructions.rs @@ -258,6 +258,23 @@ pub enum DoubleZeroInstruction { SubscribeFeed(SubscribeFeedArgs), // variant 117 UnsubscribeFeed(UnsubscribeFeedArgs), // variant 118 + + /// One close instruction per `AccessPassType`. Each refuses a pass of any other + /// kind with `AccessPassTypeMismatch`. See malbeclabs/infra#2470. + ClosePrepaidAccessPass(CloseAccessPassArgs), // variant 119 + CloseSolanaValidatorAccessPass(CloseAccessPassArgs), // variant 120 + CloseSolanaRPCAccessPass(CloseAccessPassArgs), // variant 121 + CloseOthersAccessPass(CloseAccessPassArgs), // variant 122 + CloseEdgeSeatAccessPass(CloseAccessPassArgs), // variant 123 + + /// One delete instruction per `AccessPassType`, keyed on the kind of pass the user + /// holds. Each refuses a user on a pass of any other kind with + /// `AccessPassTypeMismatch`. See malbeclabs/infra#2470. + DeletePrepaidUser(UserDeleteArgs), // variant 124 + DeleteSolanaValidatorUser(UserDeleteArgs), // variant 125 + DeleteSolanaRPCUser(UserDeleteArgs), // variant 126 + DeleteOthersUser(UserDeleteArgs), // variant 127 + DeleteEdgeSeatUser(UserDeleteArgs), // variant 128 } impl DoubleZeroInstruction { @@ -409,6 +426,18 @@ impl DoubleZeroInstruction { 117 => Ok(Self::SubscribeFeed(SubscribeFeedArgs::try_from(rest).unwrap())), 118 => Ok(Self::UnsubscribeFeed(UnsubscribeFeedArgs::try_from(rest).unwrap())), + 119 => Ok(Self::ClosePrepaidAccessPass(CloseAccessPassArgs::try_from(rest).unwrap())), + 120 => Ok(Self::CloseSolanaValidatorAccessPass(CloseAccessPassArgs::try_from(rest).unwrap())), + 121 => Ok(Self::CloseSolanaRPCAccessPass(CloseAccessPassArgs::try_from(rest).unwrap())), + 122 => Ok(Self::CloseOthersAccessPass(CloseAccessPassArgs::try_from(rest).unwrap())), + 123 => Ok(Self::CloseEdgeSeatAccessPass(CloseAccessPassArgs::try_from(rest).unwrap())), + + 124 => Ok(Self::DeletePrepaidUser(UserDeleteArgs::try_from(rest).unwrap())), + 125 => Ok(Self::DeleteSolanaValidatorUser(UserDeleteArgs::try_from(rest).unwrap())), + 126 => Ok(Self::DeleteSolanaRPCUser(UserDeleteArgs::try_from(rest).unwrap())), + 127 => Ok(Self::DeleteOthersUser(UserDeleteArgs::try_from(rest).unwrap())), + 128 => Ok(Self::DeleteEdgeSeatUser(UserDeleteArgs::try_from(rest).unwrap())), + _ => Err(ProgramError::InvalidInstructionData), } } @@ -560,6 +589,18 @@ impl DoubleZeroInstruction { Self::SetAccessPassFlags(_) => "SetAccessPassFlags".to_string(), // variant 116 Self::SubscribeFeed(_) => "SubscribeFeed".to_string(), // variant 117 Self::UnsubscribeFeed(_) => "UnsubscribeFeed".to_string(), // variant 118 + + Self::ClosePrepaidAccessPass(_) => "ClosePrepaidAccessPass".to_string(), // variant 119 + Self::CloseSolanaValidatorAccessPass(_) => "CloseSolanaValidatorAccessPass".to_string(), // variant 120 + Self::CloseSolanaRPCAccessPass(_) => "CloseSolanaRPCAccessPass".to_string(), // variant 121 + Self::CloseOthersAccessPass(_) => "CloseOthersAccessPass".to_string(), // variant 122 + Self::CloseEdgeSeatAccessPass(_) => "CloseEdgeSeatAccessPass".to_string(), // variant 123 + + Self::DeletePrepaidUser(_) => "DeletePrepaidUser".to_string(), // variant 124 + Self::DeleteSolanaValidatorUser(_) => "DeleteSolanaValidatorUser".to_string(), // variant 125 + Self::DeleteSolanaRPCUser(_) => "DeleteSolanaRPCUser".to_string(), // variant 126 + Self::DeleteOthersUser(_) => "DeleteOthersUser".to_string(), // variant 127 + Self::DeleteEdgeSeatUser(_) => "DeleteEdgeSeatUser".to_string(), // variant 128 } } @@ -704,6 +745,18 @@ impl DoubleZeroInstruction { Self::SetAccessPassFlags(args) => format!("{args:?}"), // variant 116 Self::SubscribeFeed(args) => format!("{args:?}"), // variant 117 Self::UnsubscribeFeed(args) => format!("{args:?}"), // variant 118 + + Self::ClosePrepaidAccessPass(args) => format!("{args:?}"), // variant 119 + Self::CloseSolanaValidatorAccessPass(args) => format!("{args:?}"), // variant 120 + Self::CloseSolanaRPCAccessPass(args) => format!("{args:?}"), // variant 121 + Self::CloseOthersAccessPass(args) => format!("{args:?}"), // variant 122 + Self::CloseEdgeSeatAccessPass(args) => format!("{args:?}"), // variant 123 + + Self::DeletePrepaidUser(args) => format!("{args:?}"), // variant 124 + Self::DeleteSolanaValidatorUser(args) => format!("{args:?}"), // variant 125 + Self::DeleteSolanaRPCUser(args) => format!("{args:?}"), // variant 126 + Self::DeleteOthersUser(args) => format!("{args:?}"), // variant 127 + Self::DeleteEdgeSeatUser(args) => format!("{args:?}"), // variant 128 } } } @@ -1183,6 +1236,61 @@ mod tests { DoubleZeroInstruction::CloseAccessPass(CloseAccessPassArgs {}), "CloseAccessPass", ); + test_instruction( + DoubleZeroInstruction::ClosePrepaidAccessPass(CloseAccessPassArgs {}), + "ClosePrepaidAccessPass", + ); + test_instruction( + DoubleZeroInstruction::CloseSolanaValidatorAccessPass(CloseAccessPassArgs {}), + "CloseSolanaValidatorAccessPass", + ); + test_instruction( + DoubleZeroInstruction::CloseSolanaRPCAccessPass(CloseAccessPassArgs {}), + "CloseSolanaRPCAccessPass", + ); + test_instruction( + DoubleZeroInstruction::CloseOthersAccessPass(CloseAccessPassArgs {}), + "CloseOthersAccessPass", + ); + test_instruction( + DoubleZeroInstruction::CloseEdgeSeatAccessPass(CloseAccessPassArgs {}), + "CloseEdgeSeatAccessPass", + ); + test_instruction( + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { + dz_prefix_count: 0, + multicast_publisher_count: 0, + }), + "DeletePrepaidUser", + ); + test_instruction( + DoubleZeroInstruction::DeleteSolanaValidatorUser(UserDeleteArgs { + dz_prefix_count: 0, + multicast_publisher_count: 0, + }), + "DeleteSolanaValidatorUser", + ); + test_instruction( + DoubleZeroInstruction::DeleteSolanaRPCUser(UserDeleteArgs { + dz_prefix_count: 0, + multicast_publisher_count: 0, + }), + "DeleteSolanaRPCUser", + ); + test_instruction( + DoubleZeroInstruction::DeleteOthersUser(UserDeleteArgs { + dz_prefix_count: 0, + multicast_publisher_count: 0, + }), + "DeleteOthersUser", + ); + test_instruction( + DoubleZeroInstruction::DeleteEdgeSeatUser(UserDeleteArgs { + dz_prefix_count: 0, + multicast_publisher_count: 0, + }), + "DeleteEdgeSeatUser", + ); test_instruction( DoubleZeroInstruction::CheckStatusAccessPass(CheckStatusAccessPassArgs {}), "CheckStatusAccessPass", diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/accesspass/close.rs b/smartcontract/programs/doublezero-serviceability/src/processors/accesspass/close.rs index e8e7d7a922..ac9d63f6e5 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/accesspass/close.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/accesspass/close.rs @@ -3,7 +3,9 @@ use crate::{ error::DoubleZeroError, serializer::try_acc_close, state::{ - accesspass::AccessPass, accounttype::AccountType, globalstate::GlobalState, + accesspass::{AccessPass, AccessPassKind}, + accounttype::AccountType, + globalstate::GlobalState, permission::permission_flags, }, }; @@ -30,6 +32,7 @@ pub fn process_close_access_pass( program_id: &Pubkey, accounts: &[AccountInfo], _value: &CloseAccessPassArgs, + expected: Option, ) -> ProgramResult { let accounts_iter = &mut accounts.iter(); @@ -84,35 +87,45 @@ pub fn process_close_access_pass( permission_flags::ACCESS_PASS_ADMIN, )?; - if let Ok(data) = accesspass_account.try_borrow_data() { - let account_type: AccountType = data[0].into(); - if account_type != AccountType::AccessPass { - msg!("AccountType is not AccessPass, cannot close"); - return Err(DoubleZeroError::InvalidAccountType.into()); - } - let accesspass = AccessPass::try_from(accesspass_account)?; - - // Feed authority can only close access passes they own - if globalstate.feed_authority_pk == *payer_account.key - && accesspass.owner != *payer_account.key - { - msg!("Feed authority can only close access passes they own"); - return Err(DoubleZeroError::NotAllowed.into()); + // These checks used to sit inside `if let Ok(data) = accesspass_account.try_borrow_data()`, + // with an `else` that logged a warning and fell through to the close. A failed borrow + // therefore closed the pass with neither the account-type check nor the connection check + // applied. Read the account once here and let a failed read stop the instruction. + let account_type: AccountType = accesspass_account.try_borrow_data()?[0].into(); + if account_type != AccountType::AccessPass { + msg!("AccountType is not AccessPass, cannot close"); + return Err(DoubleZeroError::InvalidAccountType.into()); + } + let accesspass = AccessPass::try_from(accesspass_account)?; + + // `None` is the deprecated `CloseAccessPass` (variant 69), which predates the + // per-pass-type split and performs no kind check. It is removed, along with this + // `Option`, in the follow-up that moves every caller. See malbeclabs/infra#2470. + if let Some(expected) = expected { + let actual = AccessPassKind::from(&accesspass.accesspass_type); + if actual != expected { + msg!("this instruction closes a {expected} pass, but the pass is {actual}"); + return Err(DoubleZeroError::AccessPassTypeMismatch.into()); } + } - if accesspass.connection_count != 0 { - msg!( - "AccessPass has {} active connections, cannot close", - accesspass.connection_count - ); - return Err(DoubleZeroError::AccessPassInUse.into()); - } + // Feed authority can only close access passes they own + if globalstate.feed_authority_pk == *payer_account.key && accesspass.owner != *payer_account.key + { + msg!("Feed authority can only close access passes they own"); + return Err(DoubleZeroError::NotAllowed.into()); + } - msg!("AccountType is AccessPass and there are no active connections, proceeding to close"); - } else { - msg!("Failed to borrow account data, cannot close"); + if accesspass.connection_count != 0 { + msg!( + "AccessPass has {} active connections, cannot close", + accesspass.connection_count + ); + return Err(DoubleZeroError::AccessPassInUse.into()); } + msg!("AccountType is AccessPass and there are no active connections, proceeding to close"); + try_acc_close(accesspass_account, payer_account)?; msg!("Access pass closed"); diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs b/smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs index a1bd01bafe..935af73fd1 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs @@ -281,8 +281,8 @@ pub fn process_update_multicastgroup_roles( // another owner's pass with the right permission, and the two operations require different // grants: // - Removal-only cleanup (stripping roles as a prerequisite to delete/request-ban) is a - // USER_ADMIN operation, as DeleteUserCommand / RequestBanUserCommand authorize the - // final instruction with the same flag. + // USER_ADMIN operation, as the DeleteUser instructions / RequestBanUserCommand + // authorize the final instruction with the same flag. // - Granting roles (subscribe/publish) on behalf of another owner manages the pass's // entitlements, so it is an ACCESS_PASS_ADMIN operation. This is the path the oracle // uses to subscribe validator-owned users (accesspass.user_payer = validator) once it diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/user/delete.rs b/smartcontract/programs/doublezero-serviceability/src/processors/user/delete.rs index 46312f0090..e18e054439 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/user/delete.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/user/delete.rs @@ -5,7 +5,7 @@ use crate::{ processors::validation::validate_program_account, serializer::{try_acc_close, try_acc_write}, state::{ - accesspass::{AccessPass, AccessPassStatus}, + accesspass::{AccessPass, AccessPassKind, AccessPassStatus}, device::Device, globalstate::GlobalState, permission::permission_flags, @@ -52,6 +52,7 @@ pub fn process_delete_user( program_id: &Pubkey, accounts: &[AccountInfo], value: &UserDeleteArgs, + expected: Option, ) -> ProgramResult { if value.dz_prefix_count == 0 { msg!("dz_prefix_count must be > 0; DeleteUser requires on-chain deallocation"); @@ -151,51 +152,59 @@ pub fn process_delete_user( "Invalid AccessPass PDA", ); - if !accesspass_account.data_is_empty() { - // Read Access Pass - let mut accesspass = AccessPass::try_from(accesspass_account)?; - if accesspass.user_payer != user.owner { - msg!( - "Invalid user_payer accesspass.user_payer: {} = user_payer: {} ", - accesspass.user_payer, - user.owner - ); - return Err(DoubleZeroError::Unauthorized.into()); - } - // Skip IP validation when the pass is stored at the UNSPECIFIED PDA (0.0.0.0): it is - // a dynamic pass valid for any IP by construction. This includes legacy passes which - // have client_ip=0.0.0.0. - if accesspass.client_ip != Ipv4Addr::UNSPECIFIED - && accesspass.client_ip != user.client_ip - && !accesspass.allow_multiple_ip() - { - msg!( - "Invalid client_ip accesspass.{{client_ip: {}}} = {{ client_ip: {} }}", - accesspass.client_ip, - user.client_ip - ); - return Err(DoubleZeroError::Unauthorized.into()); - } - - accesspass.connection_count = accesspass.connection_count.saturating_sub(1); - // Release the per-category seat (EdgeSeat only; no-op otherwise). - accesspass.remove_user(user.user_type); - // Release every feed-scoped seat this user consumed at connect. The feeds are read from the - // User (recorded when the seats were ticked), so each release is bound to exactly that seat - // and cannot be misdirected to another metro's feed by a caller-supplied account. - user.release_feed_seats(&mut accesspass); - accesspass.status = if accesspass.connection_count > 0 { - AccessPassStatus::Connected - } else { - AccessPassStatus::Disconnected - }; - if accesspass.connection_count == 0 && accesspass.allow_multiple_ip() { - accesspass.client_ip = Ipv4Addr::UNSPECIFIED; // reset to allow multiple IPs + // Read Access Pass + let mut accesspass = AccessPass::try_from(accesspass_account)?; + // `None` is the deprecated `DeleteUser` (variant 42), which predates the + // per-pass-type split and performs no kind check. It is removed, along with this + // `Option`, in the follow-up that moves every caller. See malbeclabs/infra#2470. + if let Some(expected) = expected { + let actual = AccessPassKind::from(&accesspass.accesspass_type); + if actual != expected { + msg!("this instruction deletes a user on a {expected} pass, but the pass is {actual}"); + return Err(DoubleZeroError::AccessPassTypeMismatch.into()); } + } + if accesspass.user_payer != user.owner { + msg!( + "Invalid user_payer accesspass.user_payer: {} = user_payer: {} ", + accesspass.user_payer, + user.owner + ); + return Err(DoubleZeroError::Unauthorized.into()); + } + // Skip IP validation when the pass is stored at the UNSPECIFIED PDA (0.0.0.0): it is + // a dynamic pass valid for any IP by construction. This includes legacy passes which + // have client_ip=0.0.0.0. + if accesspass.client_ip != Ipv4Addr::UNSPECIFIED + && accesspass.client_ip != user.client_ip + && !accesspass.allow_multiple_ip() + { + msg!( + "Invalid client_ip accesspass.{{client_ip: {}}} = {{ client_ip: {} }}", + accesspass.client_ip, + user.client_ip + ); + return Err(DoubleZeroError::Unauthorized.into()); + } - try_acc_write(&accesspass, accesspass_account, payer_account, accounts)?; + accesspass.connection_count = accesspass.connection_count.saturating_sub(1); + // Release the per-category seat (EdgeSeat only; no-op otherwise). + accesspass.remove_user(user.user_type); + // Release every feed-scoped seat this user consumed at connect. The feeds are read from the + // User (recorded when the seats were ticked), so each release is bound to exactly that seat + // and cannot be misdirected to another metro's feed by a caller-supplied account. + user.release_feed_seats(&mut accesspass); + accesspass.status = if accesspass.connection_count > 0 { + AccessPassStatus::Connected + } else { + AccessPassStatus::Disconnected + }; + if accesspass.connection_count == 0 && accesspass.allow_multiple_ip() { + accesspass.client_ip = Ipv4Addr::UNSPECIFIED; // reset to allow multiple IPs } + try_acc_write(&accesspass, accesspass_account, payer_account, accounts)?; + if !user.publishers.is_empty() || !user.subscribers.is_empty() { msg!("{:?}", user); return Err(DoubleZeroError::ReferenceCountNotZero.into()); diff --git a/smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs b/smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs index e3ca3491da..7d8c504d81 100644 --- a/smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs +++ b/smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs @@ -86,6 +86,47 @@ impl AccessPassType { } } +/// The `AccessPassType` variant with no payload attached. +/// +/// `AccessPassType` carries a `Pubkey`, a `String` pair or a `Vec` depending on the +/// variant. The close and delete instructions only need to know which variant a pass is, so +/// they use this type: it is `Copy`, it compares in one instruction, and it never reaches the +/// wire. There is deliberately no `Borsh` derive — the kind is chosen by the instruction +/// variant, not sent as an argument (see malbeclabs/infra#2470). +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum AccessPassKind { + Prepaid, + SolanaValidator, + SolanaRPC, + Others, + EdgeSeat, +} + +impl From<&AccessPassType> for AccessPassKind { + fn from(value: &AccessPassType) -> Self { + match value { + AccessPassType::Prepaid => AccessPassKind::Prepaid, + AccessPassType::SolanaValidator(_) => AccessPassKind::SolanaValidator, + AccessPassType::SolanaRPC(_) => AccessPassKind::SolanaRPC, + AccessPassType::Others(_, _) => AccessPassKind::Others, + AccessPassType::EdgeSeat(_) => AccessPassKind::EdgeSeat, + } + } +} + +impl fmt::Display for AccessPassKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let name = match self { + AccessPassKind::Prepaid => "prepaid", + AccessPassKind::SolanaValidator => "solana_validator", + AccessPassKind::SolanaRPC => "solana_rpc", + AccessPassKind::Others => "others", + AccessPassKind::EdgeSeat => "edge_seat", + }; + f.write_str(name) + } +} + impl fmt::Display for AccessPassType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { @@ -398,6 +439,52 @@ mod tests { use super::*; + #[test] + fn test_access_pass_kind_from_type() { + let cases = [ + (AccessPassType::Prepaid, AccessPassKind::Prepaid), + ( + AccessPassType::SolanaValidator(Pubkey::new_unique()), + AccessPassKind::SolanaValidator, + ), + ( + AccessPassType::SolanaRPC(Pubkey::new_unique()), + AccessPassKind::SolanaRPC, + ), + ( + AccessPassType::Others("thing".to_string(), "key".to_string()), + AccessPassKind::Others, + ), + (AccessPassType::EdgeSeat(vec![]), AccessPassKind::EdgeSeat), + ]; + + for (pass_type, want) in cases { + assert_eq!(AccessPassKind::from(&pass_type), want, "{pass_type}"); + } + } + + #[test] + fn test_access_pass_kind_display_matches_discriminant_string() { + // The kind and the pass type must print the same name, so an error message + // and a CLI flag value cannot drift apart. + let cases = [ + AccessPassType::Prepaid, + AccessPassType::SolanaValidator(Pubkey::new_unique()), + AccessPassType::SolanaRPC(Pubkey::new_unique()), + AccessPassType::EdgeSeat(vec![]), + ]; + + for pass_type in cases { + assert_eq!( + AccessPassKind::from(&pass_type).to_string(), + pass_type.to_discriminant_string(), + ); + } + // `Others` carries its own type_name in `to_discriminant_string`, so the kind + // prints the fixed name instead. + assert_eq!(AccessPassKind::Others.to_string(), "others"); + } + #[test] fn test_state_compatibility_accesspass() { /* To generate the base64 strings, use the following commands after deploying the program and creating accounts: diff --git a/smartcontract/programs/doublezero-serviceability/tests/accesspass_allow_multiple_ip.rs b/smartcontract/programs/doublezero-serviceability/tests/accesspass_allow_multiple_ip.rs index 0a7a683cf3..707711683d 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/accesspass_allow_multiple_ip.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/accesspass_allow_multiple_ip.rs @@ -403,7 +403,7 @@ async fn test_accesspass_allow_multiple_ip() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), diff --git a/smartcontract/programs/doublezero-serviceability/tests/accesspass_test.rs b/smartcontract/programs/doublezero-serviceability/tests/accesspass_test.rs index 6f67969151..d53ff8d337 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/accesspass_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/accesspass_test.rs @@ -142,7 +142,7 @@ async fn test_accesspass() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::CloseAccessPass(CloseAccessPassArgs {}), + DoubleZeroInstruction::CloseSolanaValidatorAccessPass(CloseAccessPassArgs {}), vec![ AccountMeta::new(accesspass_pubkey, false), AccountMeta::new(globalstate_pubkey, false), @@ -728,7 +728,7 @@ async fn test_close_accesspass_rejects_nonzero_connection_count() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::CloseAccessPass(CloseAccessPassArgs {}), + DoubleZeroInstruction::ClosePrepaidAccessPass(CloseAccessPassArgs {}), vec![ AccountMeta::new(accesspass_pubkey, false), AccountMeta::new(globalstate_pubkey, false), diff --git a/smartcontract/programs/doublezero-serviceability/tests/close_access_pass_kind_test.rs b/smartcontract/programs/doublezero-serviceability/tests/close_access_pass_kind_test.rs new file mode 100644 index 0000000000..1791e4dd51 --- /dev/null +++ b/smartcontract/programs/doublezero-serviceability/tests/close_access_pass_kind_test.rs @@ -0,0 +1,172 @@ +//! Issue #2470: each `CloseAccessPass` instruction must close a pass of its own kind and +//! refuse a pass of any other kind with `AccessPassTypeMismatch`. + +mod test_helpers; + +use doublezero_serviceability::{ + error::DoubleZeroError, + instructions::DoubleZeroInstruction, + pda::{get_accesspass_pda, get_globalstate_pda, get_program_config_pda}, + processors::accesspass::{close::CloseAccessPassArgs, set::SetAccessPassArgs}, + state::accesspass::AccessPassType, +}; +use solana_program_test::*; +use solana_sdk::{instruction::AccountMeta, pubkey::Pubkey, signature::Keypair}; +use std::net::Ipv4Addr; +use test_helpers::*; + +/// The close instruction that matches each pass type, and one that does not. +fn close_instructions( + pass_type: &AccessPassType, +) -> (DoubleZeroInstruction, DoubleZeroInstruction) { + let args = CloseAccessPassArgs {}; + match pass_type { + AccessPassType::Prepaid => ( + DoubleZeroInstruction::ClosePrepaidAccessPass(args.clone()), + DoubleZeroInstruction::CloseEdgeSeatAccessPass(args), + ), + AccessPassType::SolanaValidator(_) => ( + DoubleZeroInstruction::CloseSolanaValidatorAccessPass(args.clone()), + DoubleZeroInstruction::ClosePrepaidAccessPass(args), + ), + AccessPassType::SolanaRPC(_) => ( + DoubleZeroInstruction::CloseSolanaRPCAccessPass(args.clone()), + DoubleZeroInstruction::ClosePrepaidAccessPass(args), + ), + AccessPassType::Others(_, _) => ( + DoubleZeroInstruction::CloseOthersAccessPass(args.clone()), + DoubleZeroInstruction::ClosePrepaidAccessPass(args), + ), + AccessPassType::EdgeSeat(_) => ( + DoubleZeroInstruction::CloseEdgeSeatAccessPass(args.clone()), + DoubleZeroInstruction::ClosePrepaidAccessPass(args), + ), + } +} + +/// Create an access pass of `pass_type` at `client_ip` via `SetAccessPass`, the real +/// instruction a caller uses, rather than hand-writing an `AccessPass` and inserting it into +/// the test bank. `SetAccessPass` always creates a fresh pass with `connection_count: 0`, +/// which is what the close path requires. Returns its pubkey. +async fn create_access_pass( + banks_client: &mut BanksClient, + recent_blockhash: solana_program::hash::Hash, + program_id: Pubkey, + globalstate_pubkey: Pubkey, + payer: &Keypair, + client_ip: Ipv4Addr, + pass_type: AccessPassType, +) -> Pubkey { + let user_payer = Pubkey::new_unique(); + let (accesspass_pubkey, _) = get_accesspass_pda(&program_id, &client_ip, &user_payer); + + execute_transaction( + banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::SetAccessPass(SetAccessPassArgs { + accesspass_type: pass_type, + client_ip, + last_access_epoch: 9999, + allow_multiple_ip: false, + max_unicast_users: 1, + max_multicast_users: 1, + }), + vec![ + AccountMeta::new(accesspass_pubkey, false), + AccountMeta::new(globalstate_pubkey, false), + AccountMeta::new(user_payer, false), + ], + payer, + ) + .await; + + accesspass_pubkey +} + +#[tokio::test] +async fn close_refuses_a_pass_of_another_kind() { + let (mut banks_client, program_id, payer, recent_blockhash) = init_test().await; + + let (program_config_pubkey, _) = get_program_config_pda(&program_id); + let (globalstate_pubkey, _) = get_globalstate_pda(&program_id); + + // Makes `payer` the sole entry in the foundation allowlist, so it holds ACCESS_PASS_ADMIN. + execute_transaction( + &mut banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::InitGlobalState(), + vec![ + AccountMeta::new(program_config_pubkey, false), + AccountMeta::new(globalstate_pubkey, false), + ], + &payer, + ) + .await; + + for (i, pass_type) in [ + AccessPassType::Prepaid, + AccessPassType::SolanaValidator(Pubkey::new_unique()), + AccessPassType::SolanaRPC(Pubkey::new_unique()), + AccessPassType::Others("thing".to_string(), "key".to_string()), + AccessPassType::EdgeSeat(vec![]), + ] + .into_iter() + .enumerate() + { + let client_ip: Ipv4Addr = [101, 0, 0, 1 + i as u8].into(); + let accesspass_pubkey = create_access_pass( + &mut banks_client, + recent_blockhash, + program_id, + globalstate_pubkey, + &payer, + client_ip, + pass_type.clone(), + ) + .await; + + let accounts = vec![ + AccountMeta::new(accesspass_pubkey, false), + AccountMeta::new(globalstate_pubkey, false), + ]; + let (matching, other) = close_instructions(&pass_type); + + let err = try_execute_transaction( + &mut banks_client, + recent_blockhash, + program_id, + other, + accounts.clone(), + &payer, + ) + .await + .expect_err("a close for another kind must fail"); + assert_custom_error(&err, DoubleZeroError::AccessPassTypeMismatch); + + assert!( + get_account_data(&mut banks_client, accesspass_pubkey) + .await + .is_some(), + "the pass must survive a refused close: {pass_type}" + ); + + execute_transaction( + &mut banks_client, + recent_blockhash, + program_id, + matching, + accounts, + &payer, + ) + .await; + + assert!( + get_account_data(&mut banks_client, accesspass_pubkey) + .await + .is_none(), + "the matching close must remove the pass: {pass_type}" + ); + } +} diff --git a/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs b/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs index c60f5c626c..61bc125ade 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs @@ -2544,7 +2544,7 @@ async fn test_publisher_disconnect_delete_decrements_publishers_count() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), diff --git a/smartcontract/programs/doublezero-serviceability/tests/delete_user_dynamic_accesspass.rs b/smartcontract/programs/doublezero-serviceability/tests/delete_user_dynamic_accesspass.rs index 6a294c2659..bfb2a9eff9 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/delete_user_dynamic_accesspass.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/delete_user_dynamic_accesspass.rs @@ -357,7 +357,7 @@ async fn test_delete_user_is_dynamic_pass() { &mut env.banks_client, recent_blockhash, env.program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -444,7 +444,7 @@ async fn test_delete_user_allow_multiple_ip_resets_client_ip() { &mut env.banks_client, recent_blockhash, env.program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -515,7 +515,7 @@ async fn test_delete_user_specific_ip_pass() { &mut env.banks_client, recent_blockhash, env.program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -600,7 +600,7 @@ async fn test_delete_multicast_user_dynamic_pass() { &mut env.banks_client, recent_blockhash, env.program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), diff --git a/smartcontract/programs/doublezero-serviceability/tests/delete_user_kind_test.rs b/smartcontract/programs/doublezero-serviceability/tests/delete_user_kind_test.rs new file mode 100644 index 0000000000..c738b8fdb9 --- /dev/null +++ b/smartcontract/programs/doublezero-serviceability/tests/delete_user_kind_test.rs @@ -0,0 +1,402 @@ +//! Issue #2470: each `DeleteUser` instruction must delete a user whose access pass is of +//! its own kind, and refuse a user on a pass of any other kind with `AccessPassTypeMismatch`. + +mod test_helpers; + +use doublezero_serviceability::{ + error::DoubleZeroError, + instructions::DoubleZeroInstruction, + pda::{get_accesspass_pda, get_device_pda, get_resource_extension_pda, get_user_pda}, + processors::{ + accesspass::set::SetAccessPassArgs, + device::{create::DeviceCreateArgs, update::DeviceUpdateArgs}, + user::{create::UserCreateArgs, delete::UserDeleteArgs}, + }, + resource::ResourceType, + state::{ + accesspass::{AccessPass, AccessPassType, FeedSeat}, + device::{DeviceDesiredStatus, DeviceType}, + user::{User, UserCYOA, UserType}, + }, +}; +use solana_program_test::*; +use solana_sdk::{ + account::AccountSharedData, instruction::AccountMeta, pubkey::Pubkey, signature::Keypair, + signer::Signer, +}; +use std::net::Ipv4Addr; +use test_helpers::*; + +fn delete_args() -> UserDeleteArgs { + UserDeleteArgs { + dz_prefix_count: 1, + multicast_publisher_count: 1, + } +} + +/// The delete instruction that matches each pass type, and one that does not. +fn delete_instructions( + pass_type: &AccessPassType, +) -> (DoubleZeroInstruction, DoubleZeroInstruction) { + match pass_type { + AccessPassType::Prepaid => ( + DoubleZeroInstruction::DeletePrepaidUser(delete_args()), + DoubleZeroInstruction::DeleteEdgeSeatUser(delete_args()), + ), + AccessPassType::SolanaValidator(_) => ( + DoubleZeroInstruction::DeleteSolanaValidatorUser(delete_args()), + DoubleZeroInstruction::DeletePrepaidUser(delete_args()), + ), + AccessPassType::SolanaRPC(_) => ( + DoubleZeroInstruction::DeleteSolanaRPCUser(delete_args()), + DoubleZeroInstruction::DeletePrepaidUser(delete_args()), + ), + AccessPassType::Others(_, _) => ( + DoubleZeroInstruction::DeleteOthersUser(delete_args()), + DoubleZeroInstruction::DeletePrepaidUser(delete_args()), + ), + AccessPassType::EdgeSeat(_) => ( + DoubleZeroInstruction::DeleteEdgeSeatUser(delete_args()), + DoubleZeroInstruction::DeletePrepaidUser(delete_args()), + ), + } +} + +struct TestEnv { + context: ProgramTestContext, + payer: Keypair, + program_id: Pubkey, + globalstate_pubkey: Pubkey, + device_pubkey: Pubkey, +} + +/// GlobalState/Config, Location, Exchange, Contributor and an Activated Device, ready to host +/// users under any access-pass kind. +async fn setup_test_env() -> TestEnv { + let (mut context, program_id, globalstate_pubkey, globalconfig_pubkey) = + setup_program_with_globalconfig_context().await; + let payer = context.payer.insecure_clone(); + let recent_blockhash = context.last_blockhash; + + let (location_pubkey, exchange_pubkey, contributor_pubkey) = setup_device_prerequisites( + &mut context.banks_client, + recent_blockhash, + program_id, + globalstate_pubkey, + globalconfig_pubkey, + &payer, + ) + .await; + + let gs = get_globalstate(&mut context.banks_client, globalstate_pubkey).await; + let (device_pubkey, _) = get_device_pda(&program_id, gs.account_index + 1); + let (tunnel_ids_pda, _, _) = + get_resource_extension_pda(&program_id, ResourceType::TunnelIds(device_pubkey, 0)); + let (dz_prefix_pda, _, _) = + get_resource_extension_pda(&program_id, ResourceType::DzPrefixBlock(device_pubkey, 0)); + + execute_transaction( + &mut context.banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::CreateDevice(DeviceCreateArgs { + code: "dev".to_string(), + device_type: DeviceType::Hybrid, + public_ip: [100, 0, 0, 1].into(), + dz_prefixes: "100.1.0.0/23".parse().unwrap(), + metrics_publisher_pk: Pubkey::default(), + mgmt_vrf: "mgmt".to_string(), + desired_status: Some(DeviceDesiredStatus::Activated), + resource_count: 2, + }), + vec![ + AccountMeta::new(device_pubkey, false), + AccountMeta::new(contributor_pubkey, false), + AccountMeta::new(location_pubkey, false), + AccountMeta::new(exchange_pubkey, false), + AccountMeta::new(globalstate_pubkey, false), + AccountMeta::new(globalconfig_pubkey, false), + AccountMeta::new(tunnel_ids_pda, false), + AccountMeta::new(dz_prefix_pda, false), + ], + &payer, + ) + .await; + + execute_transaction( + &mut context.banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::UpdateDevice(DeviceUpdateArgs { + max_users: Some(128), + ..DeviceUpdateArgs::default() + }), + vec![ + AccountMeta::new(device_pubkey, false), + AccountMeta::new(contributor_pubkey, false), + AccountMeta::new(location_pubkey, false), + AccountMeta::new(location_pubkey, false), + AccountMeta::new(globalstate_pubkey, false), + ], + &payer, + ) + .await; + + TestEnv { + context, + payer, + program_id, + globalstate_pubkey, + device_pubkey, + } +} + +/// Set an access pass of `pass_type` at `client_ip` and create a user of `user_type` under it. +/// Returns (accesspass_pubkey, user_pubkey). +async fn create_and_activate_user( + env: &mut TestEnv, + client_ip: Ipv4Addr, + user_type: UserType, + pass_type: AccessPassType, +) -> (Pubkey, Pubkey) { + let recent_blockhash = env.context.last_blockhash; + let payer_pk = env.payer.pubkey(); + + let (accesspass_pubkey, _) = get_accesspass_pda(&env.program_id, &client_ip, &payer_pk); + + execute_transaction( + &mut env.context.banks_client, + recent_blockhash, + env.program_id, + DoubleZeroInstruction::SetAccessPass(SetAccessPassArgs { + accesspass_type: pass_type, + client_ip, + last_access_epoch: 9999, + allow_multiple_ip: false, + max_unicast_users: 1, + max_multicast_users: 1, + }), + vec![ + AccountMeta::new(accesspass_pubkey, false), + AccountMeta::new(env.globalstate_pubkey, false), + AccountMeta::new(payer_pk, false), + ], + &env.payer, + ) + .await; + + let (user_pubkey, _) = get_user_pda(&env.program_id, &client_ip, user_type); + let (user_tunnel_block_pda, _, _) = + get_resource_extension_pda(&env.program_id, ResourceType::UserTunnelBlock); + let (multicast_publisher_block_pda, _, _) = + get_resource_extension_pda(&env.program_id, ResourceType::MulticastPublisherBlock); + let (device_tunnel_ids_pda, _, _) = get_resource_extension_pda( + &env.program_id, + ResourceType::TunnelIds(env.device_pubkey, 0), + ); + let (dz_prefix_block_pda, _, _) = get_resource_extension_pda( + &env.program_id, + ResourceType::DzPrefixBlock(env.device_pubkey, 0), + ); + + execute_transaction( + &mut env.context.banks_client, + recent_blockhash, + env.program_id, + DoubleZeroInstruction::CreateUser(UserCreateArgs { + client_ip, + user_type, + cyoa_type: UserCYOA::GREOverDIA, + tunnel_endpoint: Ipv4Addr::UNSPECIFIED, + dz_prefix_count: 1, + ip_proof: None, + }), + vec![ + AccountMeta::new(user_pubkey, false), + AccountMeta::new(env.device_pubkey, false), + AccountMeta::new(accesspass_pubkey, false), + AccountMeta::new(env.globalstate_pubkey, false), + AccountMeta::new(user_tunnel_block_pda, false), + AccountMeta::new(multicast_publisher_block_pda, false), + AccountMeta::new(device_tunnel_ids_pda, false), + AccountMeta::new(dz_prefix_block_pda, false), + ], + &env.payer, + ) + .await; + + (accesspass_pubkey, user_pubkey) +} + +/// Rewrite the EdgeSeat pass to carry one feed seat with a user on it, and give the user that +/// same feed in `feed_pks`, bypassing the real provisioning path. That real path is +/// `SetAccessPassFeeds` (see `set_access_pass_feeds_test.rs`), but it only puts the seat on the +/// pass — it needs a caller with a permissioned authority (foundation allowlist or +/// `ACCESS_PASS_ADMIN`) and does not tick `current_users` or touch a user. Recording the feed on +/// a user, ticked, is done by `CreateSubscribeUser` or `SubscribeFeed`, and both require a real +/// `MulticastGroup` (its own create instruction, `ResourceExtension` accounts, and onchain +/// allocation), which this suite does not otherwise set up. Standing that up here to seed one +/// feed seat would roughly double this file for no gain in what the delete path itself is +/// tested against, so the seat is seeded directly instead. +/// This is the only way to put a real seat in front of `process_delete_user`'s +/// `release_feed_seats` call without that extra machinery; a feedless EdgeSeat pass makes that +/// call a no-op and never exercises the release path a `DeleteEdgeSeatUser` must perform. +async fn seed_feed_seat( + env: &mut TestEnv, + accesspass_pubkey: Pubkey, + user_pubkey: Pubkey, +) -> Pubkey { + let feed_key = Pubkey::new_unique(); + + let mut accesspass_account = env + .context + .banks_client + .get_account(accesspass_pubkey) + .await + .unwrap() + .expect("access pass must exist"); + let mut accesspass = AccessPass::try_from(&accesspass_account.data[..]).unwrap(); + accesspass.accesspass_type = AccessPassType::EdgeSeat(vec![FeedSeat { + feed_key, + max_users: 1, + max_future_users: 1, + current_users: 1, + anniversary_day: 1, + window_end: 4_000_000_000, + terminates_at: 4_100_000_000, + }]); + accesspass_account.data = borsh::to_vec(&accesspass).unwrap(); + env.context.set_account( + &accesspass_pubkey, + &AccountSharedData::from(accesspass_account), + ); + + let mut user_account = env + .context + .banks_client + .get_account(user_pubkey) + .await + .unwrap() + .expect("user must exist"); + let mut user = User::try_from(&user_account.data[..]).unwrap(); + user.feed_pks = vec![feed_key]; + user_account.data = borsh::to_vec(&user).unwrap(); + env.context + .set_account(&user_pubkey, &AccountSharedData::from(user_account)); + + feed_key +} + +#[tokio::test] +async fn delete_refuses_a_user_of_another_kind() { + for (i, pass_type) in [ + AccessPassType::Prepaid, + AccessPassType::SolanaValidator(Pubkey::new_unique()), + AccessPassType::SolanaRPC(Pubkey::new_unique()), + AccessPassType::Others("thing".to_string(), "key".to_string()), + AccessPassType::EdgeSeat(vec![]), + ] + .into_iter() + .enumerate() + { + let mut env = setup_test_env().await; + let client_ip: Ipv4Addr = [100, 0, 0, 10 + i as u8].into(); + let user_type = if matches!(pass_type, AccessPassType::EdgeSeat(_)) { + UserType::Multicast + } else { + UserType::IBRL + }; + + let (accesspass_pubkey, user_pubkey) = + create_and_activate_user(&mut env, client_ip, user_type, pass_type.clone()).await; + + if matches!(pass_type, AccessPassType::EdgeSeat(_)) { + seed_feed_seat(&mut env, accesspass_pubkey, user_pubkey).await; + } + + let (matching, other) = delete_instructions(&pass_type); + + let (user_tunnel_block_pda, _, _) = + get_resource_extension_pda(&env.program_id, ResourceType::UserTunnelBlock); + let (multicast_publisher_block_pda, _, _) = + get_resource_extension_pda(&env.program_id, ResourceType::MulticastPublisherBlock); + let (device_tunnel_ids_pda, _, _) = get_resource_extension_pda( + &env.program_id, + ResourceType::TunnelIds(env.device_pubkey, 0), + ); + let (dz_prefix_pda, _, _) = get_resource_extension_pda( + &env.program_id, + ResourceType::DzPrefixBlock(env.device_pubkey, 0), + ); + let owner = env.payer.pubkey(); + + let accounts = vec![ + AccountMeta::new(user_pubkey, false), + AccountMeta::new(accesspass_pubkey, false), + AccountMeta::new(env.globalstate_pubkey, false), + AccountMeta::new(env.device_pubkey, false), + AccountMeta::new(user_tunnel_block_pda, false), + AccountMeta::new(multicast_publisher_block_pda, false), + AccountMeta::new(device_tunnel_ids_pda, false), + AccountMeta::new(dz_prefix_pda, false), + AccountMeta::new(owner, false), + ]; + + let err = try_execute_transaction( + &mut env.context.banks_client, + env.context.last_blockhash, + env.program_id, + other, + accounts.clone(), + &env.payer, + ) + .await + .expect_err("a delete for another kind must fail"); + assert_custom_error(&err, DoubleZeroError::AccessPassTypeMismatch); + + assert!( + get_account_data(&mut env.context.banks_client, user_pubkey) + .await + .is_some(), + "the user must survive a refused delete: {pass_type}" + ); + + execute_transaction( + &mut env.context.banks_client, + env.context.last_blockhash, + env.program_id, + matching, + accounts, + &env.payer, + ) + .await; + + assert!( + get_account_data(&mut env.context.banks_client, user_pubkey) + .await + .is_none(), + "the matching delete must remove the user: {pass_type}" + ); + + // The EdgeSeat case seeded a real feed seat (see `seed_feed_seat`); confirm the matching + // delete actually released it rather than `release_feed_seats` silently no-op'ing. + if matches!(pass_type, AccessPassType::EdgeSeat(_)) { + let accesspass_account = env + .context + .banks_client + .get_account(accesspass_pubkey) + .await + .unwrap() + .expect("access pass survives a user delete"); + let accesspass = AccessPass::try_from(&accesspass_account.data[..]).unwrap(); + assert_eq!( + accesspass.feed_seats(), + [FeedSeat { + current_users: 0, + ..accesspass.feed_seats()[0].clone() + }], + "the matching EdgeSeat delete must release the seat" + ); + } + } +} diff --git a/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs b/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs index 1fbef24df7..4e69eb9b5a 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs @@ -1,6 +1,7 @@ use borsh::to_vec; use doublezero_serviceability::{ entrypoint::process_instruction, + error::DoubleZeroError, instructions::*, pda::{ get_contributor_pda, get_exchange_pda, get_globalconfig_pda, get_globalstate_pda, @@ -18,6 +19,7 @@ use doublezero_serviceability::{ topology::TopologyConstraint, }, }; +use solana_program::program_error::ProgramError; use solana_program_test::*; use solana_sdk::{ instruction::{AccountMeta, Instruction}, @@ -987,3 +989,18 @@ pub fn custom_error_code(err: &BanksClientError) -> Option { _ => None, } } + +/// Asserts a failed transaction was rejected with the `ProgramError::Custom` code that +/// `expected` maps to. Takes the expected error as a parameter rather than hardcoding one, so +/// any test in this crate can use it to check any `DoubleZeroError`. +#[allow(dead_code)] +pub fn assert_custom_error(err: &BanksClientError, expected: DoubleZeroError) { + let ProgramError::Custom(want) = ProgramError::from(expected.clone()) else { + panic!("{expected:?} must map to ProgramError::Custom"); + }; + assert_eq!( + custom_error_code(err), + Some(want), + "expected Custom({want}), got {err:?}" + ); +} diff --git a/smartcontract/programs/doublezero-serviceability/tests/user_old_test.rs b/smartcontract/programs/doublezero-serviceability/tests/user_old_test.rs index cc09b14722..100177dcd0 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/user_old_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/user_old_test.rs @@ -402,7 +402,7 @@ async fn test_old_user() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), diff --git a/smartcontract/programs/doublezero-serviceability/tests/user_onchain_allocation_test.rs b/smartcontract/programs/doublezero-serviceability/tests/user_onchain_allocation_test.rs index f874a26e8f..ed287978d2 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/user_onchain_allocation_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/user_onchain_allocation_test.rs @@ -760,7 +760,7 @@ async fn test_delete_user_atomic_with_deallocation() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -1467,7 +1467,7 @@ async fn test_delete_user_atomic_decrements_multicast_subscribers_count() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -1672,7 +1672,7 @@ async fn test_multicast_publisher_block_deallocation_and_reuse() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -1968,7 +1968,7 @@ async fn test_delete_user_atomic_decrements_subscribers_count_for_non_publisher( &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), diff --git a/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs b/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs index 4e9b85f28a..2324a17098 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs @@ -685,7 +685,7 @@ async fn test_user() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -1362,7 +1362,7 @@ async fn test_user_delete_from_banned() { &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }), @@ -1461,7 +1461,7 @@ async fn test_user_check_access_pass_expired_epoch_stays_activated_and_delete() &mut banks_client, recent_blockhash, program_id, - DoubleZeroInstruction::DeleteUser(UserDeleteArgs { + DoubleZeroInstruction::DeletePrepaidUser(UserDeleteArgs { dz_prefix_count: 1, multicast_publisher_count: 0, }),