-
Notifications
You must be signed in to change notification settings - Fork 317
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
feat(boundary): setup of rate-limit canister #1961
base: master
Are you sure you want to change the base?
Changes from 13 commits
bc2e2bf
13da21b
5127eb5
ecf0379
0e6d348
51f7d8e
c251521
32f32d9
ce21765
851f8d8
7ed0807
5c554f2
47efd9b
d134f46
2dc3dfc
fbd6d94
3ac629f
a4eab8b
3406440
306fc08
3cacb9f
e02f328
4b99933
6478c6d
cca546a
039499d
b67780f
0d98d46
5c2d972
cc53441
ba8103b
381074d
2c54dce
4dee538
226cfaa
6e1906c
cacb6ee
7402063
0f92fcf
67cb123
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
[package] | ||
name = "rate_limits" | ||
version.workspace = true | ||
authors.workspace = true | ||
edition.workspace = true | ||
description.workspace = true | ||
documentation.workspace = true | ||
|
||
[dependencies] | ||
anyhow = { workspace = true } | ||
bincode = { workspace = true } | ||
candid = { workspace = true } | ||
hex = { workspace = true } | ||
ic-cdk = { workspace = true } | ||
ic-cdk-macros = { workspace = true } | ||
ic-cdk-timers = { workspace = true } | ||
ic-stable-structures = { workspace = true } | ||
rate-limits-api = { path = "./api" } | ||
serde = { workspace = true } | ||
serde_json = { workspace = true } | ||
sha2 = { workspace = true } | ||
thiserror = { workspace = true } | ||
|
||
[lib] | ||
crate-type = ["cdylib"] | ||
path = "canister/canister.rs" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
[package] | ||
name = "rate-limits-api" | ||
version.workspace = true | ||
authors.workspace = true | ||
edition.workspace = true | ||
description.workspace = true | ||
documentation.workspace = true | ||
|
||
[dependencies] | ||
candid = {workspace = true} | ||
serde = {workspace = true} | ||
|
||
[lib] | ||
path = "src/lib.rs" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
use candid::CandidType; | ||
use candid::Principal; | ||
use serde::{Deserialize, Serialize}; | ||
pub type Version = u64; | ||
pub type Timestamp = u64; | ||
pub type RuleId = String; | ||
pub type IncidentId = String; | ||
pub type SchemaVersion = u64; | ||
|
||
pub type GetConfigResponse = Result<ConfigResponse, String>; | ||
pub type AddConfigResponse = Result<(), String>; | ||
pub type GetRuleByIdResponse = Result<OutputRuleMetadata, String>; | ||
pub type DiscloseRulesResponse = Result<(), String>; | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub enum DiscloseRulesArg { | ||
RuleIds(Vec<RuleId>), | ||
IncidentIds(Vec<IncidentId>), | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct ConfigResponse { | ||
pub version: Version, | ||
pub active_since: Timestamp, | ||
pub config: OutputConfig, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct OutputConfig { | ||
pub schema_version: SchemaVersion, | ||
pub rules: Vec<OutputRule>, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct InputConfig { | ||
pub schema_version: SchemaVersion, | ||
pub rules: Vec<InputRule>, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct InputRule { | ||
pub incident_id: IncidentId, | ||
pub rule_raw: Vec<u8>, | ||
pub description: String, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct OutputRule { | ||
pub id: RuleId, | ||
pub incident_id: IncidentId, | ||
pub rule_raw: Option<Vec<u8>>, | ||
pub description: Option<String>, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct OutputRuleMetadata { | ||
pub id: RuleId, | ||
pub rule_raw: Option<Vec<u8>>, | ||
pub description: Option<String>, | ||
pub disclosed_at: Option<Timestamp>, | ||
pub added_in_version: Version, | ||
pub removed_in_version: Option<Version>, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Debug)] | ||
pub struct InitArg { | ||
pub registry_polling_period_secs: u64, | ||
} | ||
|
||
#[derive(CandidType, Deserialize, Clone, Copy, PartialEq, Eq)] | ||
pub struct GetApiBoundaryNodeIdsRequest {} | ||
|
||
#[derive(CandidType, Serialize, Deserialize, Clone, PartialEq, Debug, Eq)] | ||
pub struct ApiBoundaryNodeIdRecord { | ||
pub id: Option<Principal>, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
use candid::Principal; | ||
|
||
use crate::storage::API_BOUNDARY_NODE_PRINCIPALS; | ||
|
||
const FULL_ACCESS_ID: &str = ""; | ||
const FULL_READ_TESTING_ID: &str = ""; // TODO: remove this | ||
|
||
pub trait ResolveAccessLevel { | ||
fn get_access_level(&self) -> AccessLevel; | ||
} | ||
|
||
#[derive(Debug, thiserror::Error)] | ||
pub enum AccessLevelError {} | ||
|
||
#[derive(PartialEq, Eq)] | ||
pub enum AccessLevel { | ||
FullAccess, | ||
FullRead, | ||
RestrictedRead, | ||
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. Maybe we should call it something like 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.
|
||
} | ||
|
||
#[derive(Clone)] | ||
pub struct AccessLevelResolver { | ||
pub caller_id: Principal, | ||
} | ||
|
||
impl AccessLevelResolver { | ||
pub fn new(caller_id: Principal) -> Self { | ||
Self { caller_id } | ||
} | ||
} | ||
|
||
impl ResolveAccessLevel for AccessLevelResolver { | ||
fn get_access_level(&self) -> AccessLevel { | ||
let full_access_principal = Principal::from_text(FULL_ACCESS_ID).unwrap(); | ||
|
||
API_BOUNDARY_NODE_PRINCIPALS.with(|cell| { | ||
let mut full_read_principals = cell.borrow_mut(); | ||
// TODO: this is just for testing, remove later | ||
let full_read_id = Principal::from_text(FULL_READ_TESTING_ID).unwrap(); | ||
let _ = full_read_principals.insert(full_read_id); | ||
|
||
if self.caller_id == full_access_principal { | ||
return AccessLevel::FullAccess; | ||
} else if full_read_principals.contains(&self.caller_id) { | ||
return AccessLevel::FullRead; | ||
} | ||
|
||
AccessLevel::RestrictedRead | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
get_access_level(&self, Principal) -> AccessLevel
? Otherwise, I notice below you create a newAccessLevelResolver
every time you want to check someone's access level?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it allows me to construct objects nicely in the canister method:
otherwise i'd need to pass
caller_id
somehow else.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing the ACLs so far down the call stack? For example, for
add_config
ordisclose_rules
, we should not even instantiate theseConfigAdder
andRulesDiscloser
objects. The earlier we can reject the call, the better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO in the canister function it's better to assemble the final object (by either objects composition or using wrappers) and perform just one single call on that object, which executes the whole logic. With this approach we can ensure best testability of the canister. I think this approach is common for canisters. So I would abstain from putting access level call within the canister method. Although it is probably indeed better to introduce wrapper structs:
WithLogger
,WithAccessLevel
,WithMetrics
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to log info before/after authorization, so we need a composable and extensible approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but there's an even nicer way to achieve this:
I have some examples for doing this here, here as well as here (last one includes stable structures).
Also, we use a similar approach throughout this codebase.