Add permission check to account #14535

runtian-zhou · 2024-09-05T00:41:31Z

Description

Adds permission checks to account operations in the Aptos Framework to restrict key rotation and signer capability management. This introduces a new AccountPermission type and associated functions to control access to privileged account operations.

Key changes:

Added check_signer_permission function to verify permissions
Added grant_permission function to authorize signers
Added permission checks to all account rotation and capability management functions
Introduced new error code ENO_ACCOUNT_PERMISSION

Type of Change

New feature
Breaking change

Which Components or Systems Does This Change Impact?

Move/Aptos Virtual Machine
Aptos Framework

How Has This Been Tested?

TBA

Key Areas to Review

Permission check implementation in check_signer_permission
Integration of permission checks across account operations
Error handling for unauthorized operations
Permission granting mechanism in grant_permission

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-09-05T00:41:34Z

⏱️ 3h 5m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-unit-coverage	20m	🟩
rust-move-unit-coverage	19m	🟩
rust-move-unit-coverage	16m	🟩
rust-move-unit-coverage	15m	🟩
rust-move-unit-coverage	14m	🟩
general-lints	13m	🟩 🟩 🟩 🟩 🟩 (+2 more)
rust-cargo-deny	12m	🟩 🟩 🟩 🟩 🟩 (+2 more)
rust-move-unit-coverage	11m	🟩
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-move-tests	9m	🟥
rust-move-tests	8m	🟥
rust-move-tests	8m	🟥
rust-move-tests	8m	🟥
semgrep/ci	3m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-move-unit-coverage	3m	⬜

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

runtian-zhou · 2024-09-05T00:41:38Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

codecov · 2024-09-09T07:25:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (asset_permission@444ac11). Learn more about missing BASE report.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             asset_permission   #14535   +/-   ##
===================================================
  Coverage                    ?    59.4%           
===================================================
  Files                       ?      857           
  Lines                       ?   210762           
  Branches                    ?        0           
===================================================
  Hits                        ?   125197           
  Misses                      ?    85565           
  Partials                    ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aptos-move/framework/aptos-framework/sources/aptos_account.move

igor-aptos · 2024-12-13T23:59:19Z

aptos-move/framework/aptos-framework/sources/account.move

@@ -187,6 +190,23 @@ module aptos_framework::account {
    /// Create signer for testing, independently of an Aptos-style `Account`.
    public fun create_signer_for_test(addr: address): signer { create_signer(addr) }

+    struct AccountPermission has copy, drop, store {}


I don't follow fully what all of the function below do - but not all are related to key rotation, right?

If so, can we organize them, and have AccountKeyRotationPermission as a separate category, and see for the rest if we need 1 or more.

If all below are related to key rotation - then rename to AccountKeyRotationPermission - as that sounds much scarier (as it should), than AccountPermission

graphite-app · 2024-12-14T18:18:35Z

aptos-move/framework/aptos-framework/sources/account.move

+    /// Permissions
+    inline fun check_signer_permission(s: &signer) {
+        assert!(
+            permissioned_signer::check_permission_exists(s, AccountPermission {}),
+            error::permission_denied(ENO_ACCOUNT_PERMISSION),


Add a test-only flag to optionally skip permission checks in tests by modifying check_signer_permission() to check this flag first

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

runtian-zhou force-pushed the 09-04-implement_rust_logics_for_permissioned_signer branch from 703921b to 48991ff Compare September 9, 2024 07:08

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 05cd155 to 19324d8 Compare September 9, 2024 07:08

runtian-zhou force-pushed the 09-04-implement_rust_logics_for_permissioned_signer branch from 48991ff to 906c7c7 Compare September 9, 2024 07:47

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 19324d8 to 0a93383 Compare September 9, 2024 07:47

runtian-zhou force-pushed the 09-04-implement_rust_logics_for_permissioned_signer branch from 906c7c7 to 350db4c Compare September 9, 2024 17:09

runtian-zhou mentioned this pull request Sep 9, 2024

add permissions for fungible assets operation #14567

Open

21 tasks

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 0a93383 to bde486a Compare September 9, 2024 17:09

runtian-zhou changed the base branch from 09-04-implement_rust_logics_for_permissioned_signer to asset_permission September 9, 2024 17:09

runtian-zhou force-pushed the asset_permission branch from dec4a79 to 6cd5fd0 Compare September 9, 2024 17:55

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from bde486a to 5fc08ad Compare September 9, 2024 17:55

runtian-zhou force-pushed the asset_permission branch from 6cd5fd0 to 88ad9a1 Compare September 10, 2024 17:25

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 5fc08ad to da28781 Compare September 10, 2024 17:25

runtian-zhou mentioned this pull request Sep 10, 2024

add perimission checks to object #14582

Open

10 tasks

runtian-zhou force-pushed the asset_permission branch from 88ad9a1 to 5f90e3a Compare September 10, 2024 17:29

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from da28781 to 006ad4c Compare September 10, 2024 17:29

runtian-zhou force-pushed the asset_permission branch from 5f90e3a to ef7be0f Compare September 12, 2024 01:50

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 006ad4c to ef7abd3 Compare September 12, 2024 01:50

This was referenced Sep 12, 2024

create permissioned signer example #14469

Open

[feature] Add permissioned signer functionality #14521

Open

permission for framework #14605

Open

runtian-zhou force-pushed the asset_permission branch from ef7be0f to cd6090b Compare September 17, 2024 05:09

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from ef7abd3 to c38c2dd Compare September 17, 2024 05:09

runtian-zhou force-pushed the asset_permission branch from cd6090b to 0df1c5a Compare September 17, 2024 21:23

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from c38c2dd to 95b6704 Compare September 17, 2024 21:23

runtian-zhou force-pushed the asset_permission branch from 0df1c5a to 1bd468b Compare September 18, 2024 06:23

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 95b6704 to 7394967 Compare September 18, 2024 06:23

runtian-zhou force-pushed the asset_permission branch from 1bd468b to 40796a4 Compare September 18, 2024 17:52

runtian-zhou force-pushed the asset_permission branch from c557d1d to 1212bb1 Compare December 2, 2024 02:43

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from da2d989 to c124e6c Compare December 2, 2024 02:43

runtian-zhou force-pushed the asset_permission branch from 1212bb1 to 57713ac Compare December 2, 2024 10:06

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from c124e6c to 304e1e6 Compare December 2, 2024 10:07

runtian-zhou force-pushed the asset_permission branch from 57713ac to 289ab4d Compare December 4, 2024 06:14

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 304e1e6 to 3cd3d85 Compare December 4, 2024 06:14

runtian-zhou force-pushed the asset_permission branch from 289ab4d to 75ffedc Compare December 5, 2024 02:22

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 3cd3d85 to 5aae08b Compare December 5, 2024 02:22

graphite-app bot reviewed Dec 13, 2024

View reviewed changes

aptos-move/framework/aptos-framework/sources/aptos_account.move Outdated Show resolved Hide resolved

igor-aptos reviewed Dec 14, 2024

View reviewed changes

runtian-zhou marked this pull request as ready for review December 14, 2024 15:26

runtian-zhou requested review from junkil-park, alinush, davidiw, movekevin and wrwg as code owners December 14, 2024 15:26

runtian-zhou force-pushed the asset_permission branch from 75ffedc to d09b7ba Compare December 14, 2024 18:00

runtian-zhou requested a review from a team as a code owner December 14, 2024 18:00

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 5aae08b to f15f9e1 Compare December 14, 2024 18:00

graphite-app bot reviewed Dec 14, 2024

View reviewed changes

runtian-zhou force-pushed the asset_permission branch from d09b7ba to 7d7eed9 Compare December 16, 2024 09:16

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from f15f9e1 to 5ed6a9c Compare December 16, 2024 09:16

runtian-zhou mentioned this pull request Dec 16, 2024

Add another flavor of permission api #15609

Draft

22 tasks

runtian-zhou force-pushed the asset_permission branch 2 times, most recently from 4db0036 to d932531 Compare December 17, 2024 02:27

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 5ed6a9c to 895df9e Compare December 17, 2024 02:27

TMP

65c5737

runtian-zhou force-pushed the asset_permission branch from d932531 to 15dfa6e Compare December 17, 2024 12:01

runtian-zhou force-pushed the 09-04-add_permission_check_to_account branch from 895df9e to 65c5737 Compare December 17, 2024 12:01

runtian-zhou mentioned this pull request Dec 17, 2024

Feature gate permissioned signer #15622

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add permission check to account #14535

Add permission check to account #14535

runtian-zhou commented Sep 5, 2024 •

edited

Loading

trunk-io bot commented Sep 5, 2024 •

edited

Loading

runtian-zhou commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

igor-aptos Dec 13, 2024

graphite-app bot Dec 14, 2024

Add permission check to account #14535

Are you sure you want to change the base?

Add permission check to account #14535

Conversation

runtian-zhou commented Sep 5, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Sep 5, 2024 • edited Loading

runtian-zhou commented Sep 5, 2024 • edited Loading

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

igor-aptos Dec 13, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 14, 2024

Choose a reason for hiding this comment

runtian-zhou commented Sep 5, 2024 •

edited

Loading

trunk-io bot commented Sep 5, 2024 •

edited

Loading

runtian-zhou commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading