Skip to content
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

test: PriorityRpc, RegisterProvider, and ManageCanister caller authorizations #122

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

rvanasa
Copy link
Collaborator

@rvanasa rvanasa commented Jan 8, 2024

No description provided.

@@ -30,7 +30,7 @@ pub fn require_register_provider() -> Result<(), String> {
}

pub fn is_rpc_allowed(caller: &Principal) -> bool {
METADATA.with(|m| m.borrow().get().open_rpc_access) || is_authorized(caller, Auth::Rpc)
METADATA.with(|m| m.borrow().get().open_rpc_access) || is_authorized(caller, Auth::PriorityRpc)
Copy link
Collaborator Author

@rvanasa rvanasa Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was supposed to be named PriorityRpc instead of Rpc, but it seems like this change was lost in some unmerged PR a while ago.

@@ -14,7 +14,7 @@ pub fn is_authorized(principal: &Principal, auth: Auth) -> bool {

pub fn require_admin_or_controller() -> Result<(), String> {
let caller = ic_cdk::caller();
if is_authorized(&caller, Auth::ManageService) || ic_cdk::api::is_controller(&caller) {
if is_authorized(&caller, Auth::ManageCanister) || ic_cdk::api::is_controller(&caller) {
Copy link
Collaborator Author

@rvanasa rvanasa Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming ManageService to ManageCanister for clarity. In all other cases, the term "service" refers to third-party RPC services rather than the EVM RPC canister itself. Updated the design document accordingly.

Copy link
Contributor

@THLO THLO Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ManageCanister could be confused with the management canister (or a management canister function), so this name is also not idea...
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I see what you're saying. Maybe just Manage then? I'll do this refactor in a separate PR so we can try a few different options.

@rvanasa rvanasa changed the title test: PriorityRpc and RegisterProvider principal authorizations test: PriorityRpc, RegisterProvider, and ManageCanister caller authorizations Jan 8, 2024
Comment on lines 176 to +186
self.call_update("registerProvider", Encode!(&args).unwrap())
.wait()
}

pub fn unregister_provider(&self, provider_id: u64) -> bool {
self.call_update("unregisterProvider", Encode!(&provider_id).unwrap())
.wait()
}

pub fn update_provider(&self, args: UpdateProviderArgs) {
self.call_update("updateProvider", Encode!(&args).unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume panicking in these functions is okay, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep; these functions are only used by us to manage the canister.

@rvanasa rvanasa merged commit dd1ac9e into main Jan 9, 2024
3 checks passed
@rvanasa rvanasa deleted the more-tests branch January 9, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants