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

refactor/permissions: refactor permissions handling in Mock Vault to the new granularized AppPermissions #1034

Merged
merged 1 commit into from Sep 26, 2019

Conversation

@Yoga07
Copy link
Member

commented Sep 12, 2019

Resolves #989 and #990

@Yoga07 Yoga07 requested review from nbaksalyar, m-cat and lionel1704 Sep 12, 2019
@Yoga07 Yoga07 force-pushed the Yoga07:perms branch 3 times, most recently from f5cca4e to 44ba3ea Sep 12, 2019
safe_app/src/tests/coins.rs Outdated Show resolved Hide resolved
safe_app/src/tests/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Added a comment.

It might be worth discussing how the granularity of permissions work. According to this PR if an app has permissions to mutate but doesn't have permissions to read the balance then the mutation will fail.
Let's discuss this further.

match self.get_coin_balance(coins_balance_id) {
Some(balance) => Ok(balance.balance()),
None => Err(SndError::NoSuchBalance),
fn get_balance(&self, coins_balance_id: &XorName, requester_pk: PublicKey) -> SndResult<Coins> {

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 20, 2019

Member

This function shares some logic with the authorise_mutation.
It might be worth adding something like an Operations enum with GetBalance, TransferCoins and Mutation and a common function to authorise the operation depending on the request

@Yoga07 Yoga07 force-pushed the Yoga07:perms branch 2 times, most recently from 23ba0fd to 34fefb2 Sep 24, 2019
}

// Authorises coin transfers, mutations and get balance operations.
pub fn authorise_operation(

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 24, 2019

Member

This can be improved a bit.
Will discuss offline.

@Yoga07 Yoga07 force-pushed the Yoga07:perms branch 3 times, most recently from f1a1007 to fa01235 Sep 24, 2019
@Yoga07 Yoga07 requested review from lionel1704 and m-cat Sep 24, 2019
@Yoga07 Yoga07 force-pushed the Yoga07:perms branch from fa01235 to a4597fe Sep 25, 2019
Copy link
Member

left a comment

A few more comments

// Fetches permissions granted to the application
let perms = account.auth_keys().get(&requester_pk).map_or_else(
|| {
debug!("Account not found for {:?}", owner);

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 25, 2019

Member

This debug! trace should say something like "App not authorised"

safe_core/src/client/mock/vault.rs Outdated Show resolved Hide resolved
return Err(SndError::AccessDenied);
}
let unlimited_mut = unlimited_muts(&self.config);

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 25, 2019

Member

I've an idea.
Can we add an unlimited_coins: bool to the dev config? It would be useful for app devs.
This way we can also add a has_sufficient_balance(op: Operation, balance: Coins) -> bool function and use it whenever this check is required.

self.authorise_operation(
&[
Operation::Mutation,
Operation::GetBalance,

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 25, 2019

Member

A couple of changes required here:

  • An app does not need GetBalance permissions to create a balance.
  • TransferCoins permissions is required only if amount > 0
safe_core/src/client/mock/vault.rs Outdated Show resolved Hide resolved
safe_core/src/client/mock/vault.rs Outdated Show resolved Hide resolved
safe_core/src/client/mock/vault.rs Outdated Show resolved Hide resolved
safe_core/src/client/mock/vault.rs Outdated Show resolved Hide resolved
@Yoga07 Yoga07 force-pushed the Yoga07:perms branch 2 times, most recently from 4391a6c to 372ca4c Sep 25, 2019
@Yoga07 Yoga07 requested review from lionel1704 and m-cat Sep 26, 2019
Copy link
Member

left a comment

Last comment I promise 😄

safe_core/src/client/mock/vault.rs Show resolved Hide resolved
} else {
} else if let Err(e) =
// Check if the requester is authorized to perform coin transactions, mutate, and read balance.
if amount == unwrap!(Coins::from_str("0")) {

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 26, 2019

Member

Same comment as above

if let Err(e) = self.authorise_coin_operation(&source, requester_pk) {
Response::Mutation(Err(e))
} else if self.get_login_packet(account_data.destination()).is_some() {
if self.get_login_packet(account_data.destination()).is_some() {

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Sep 26, 2019

Member

This needs to be coherent with safe_vault.
In the real network, the operation will be authorised first (at my client handlers) and then passed on to a different section which is responsible for the login packet storage.
So depending on which check is performed first a different error would be returned in tests. So we should test this against a vault too.

@Yoga07 Yoga07 force-pushed the Yoga07:perms branch 2 times, most recently from 977c4c9 to b499e74 Sep 26, 2019
@Yoga07 Yoga07 force-pushed the Yoga07:perms branch from b499e74 to 75e175e Sep 26, 2019
@Yoga07 Yoga07 force-pushed the Yoga07:perms branch from 75e175e to 61e830c Sep 26, 2019
@Yoga07 Yoga07 requested a review from lionel1704 Sep 26, 2019
@@ -360,8 +358,13 @@ impl Vault {
amount: Coins,
transaction_id: u64,
) -> SndResult<Transaction> {
let ul = unlimited_coins(&self.config);

This comment has been minimized.

Copy link
@m-cat

m-cat Sep 26, 2019

Contributor

Why store this in a variable if it's only used once? Other than that, looks good!

This comment has been minimized.

Copy link
@Yoga07

Yoga07 Sep 26, 2019

Author Member

self is being borrowed mutably in the next line(L362) at self.get_coin_balance_mut(&source), and it doesn't allow us to use self.configwhile still being borrowed. Hence storing the result before hand.

@Yoga07 Yoga07 force-pushed the Yoga07:perms branch from 61e830c to 72ef4b3 Sep 26, 2019
@Yoga07 Yoga07 requested a review from m-cat Sep 26, 2019
Copy link
Member

left a comment

Nice work @Yoga07 👍

Copy link
Contributor

left a comment

Great job!

Copy link
Member

left a comment

Great work! Just a small comment which you can ignore or implement it in one of the next PRs.

@@ -44,6 +44,8 @@ impl AuthReq {
app: app.into_repr_c()?,
app_container,
app_permission_transfer_coins: app_permissions.transfer_coins,
app_permission_perform_mutations: app_permissions.perform_mutations,

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Sep 26, 2019

Member

I'm thinking that it would probably be better to have a separate FFI struct for AppPermissions instead of relying on prefixes? I.e. we can replace

app_permission_transfer_coins: app_permissions.transfer_coins,
app_permission_perform_mutations: app_permissions.perform_mutations,
...

with

app_permissions: app_permissions.into_repr_c(),

And because AppPermissions itself is just a primitive struct with some booleans, perhaps it can be just made #[repr(C)] right in safe-nd?

This comment has been minimized.

Copy link
@Yoga07

Yoga07 Sep 26, 2019

Author Member

Sure @nbaksalyar ! Sounds like a good idea. We shall do this in the upcoming PRs.

&[
Operation::Mutation,
Operation::GetBalance,
Operation::TransferCoins,

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Sep 26, 2019

Member

Adding as a comment here too: as was discussed on Slack, this is not what is expected for the CreateLoginPacket operation, it should require only the Operation::Mutation permissions.

…the new granularized AppPermissions

- refactors and integrates authorization functions with Operation Enum
- refactors mock_unlimited_muts gate to mock_unlimited_coins in dev config as we use coins for mutation operations
- adds test to verify PUTs by unregistered clients to fail
@Yoga07 Yoga07 dismissed stale reviews from m-cat and lionel1704 via 096c4b7 Sep 26, 2019
@Yoga07 Yoga07 force-pushed the Yoga07:perms branch from 72ef4b3 to 096c4b7 Sep 26, 2019
@m-cat
m-cat approved these changes Sep 26, 2019
@Yoga07 Yoga07 requested a review from nbaksalyar Sep 26, 2019
Copy link
Member

left a comment

LGTM 👍 Just a minor comment to be addressed in later PRs.

Response::Mutation(Err(e))
} else if self.get_login_packet(account_data.destination()).is_some() {
Response::Mutation(Err(SndError::LoginPacketExists))
} else {
let result = self
.get_balance(&source)
.and_then(|source_balance| {
if source_balance.checked_sub(*COST_OF_PUT).is_none() {
if !self.has_sufficient_balance(source_balance, *COST_OF_PUT) {

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Sep 26, 2019

Member

We don't need this check here: it's already performed by self.authorise_operations.
It's OK to address this in later PRs though. :)

let debit_amt = amount
.checked_add(*COST_OF_PUT)
.ok_or(SndError::ExcessiveValue)?;
if !self.has_sufficient_balance(source_balance, debit_amt) {

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Sep 26, 2019

Member

Same here: this check is already performed by authorise_operations.

@nbaksalyar nbaksalyar merged commit b138d49 into maidsafe:master Sep 26, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.