Skip to content

Commit

Permalink
[feature] #2228: Add Unauthorized variant to smartcontracts query err…
Browse files Browse the repository at this point in the history
…or (#2428)

* [feature] #2228: Add Unauthorized query error variant

Signed-off-by: Ales Tsurko <[email protected]>

* [fix] #2228: Fix torii tests

Signed-off-by: Ales Tsurko <[email protected]>

* [fix] #2228: Clean up

Signed-off-by: Ales Tsurko <[email protected]>

* [fix] #2228: Simplify code

Signed-off-by: Ales Tsurko <[email protected]>

* [fix] #2228: Reformat

Signed-off-by: Ales Tsurko <[email protected]>

* [feature] #2228: Clean up

Signed-off-by: Ales Tsurko <[email protected]>

* [feature] #2228: Apply suggested changes

Signed-off-by: Ales Tsurko <[email protected]>
  • Loading branch information
ales-tsurko authored Jul 5, 2022
1 parent 9b3f1e9 commit 9bfa772
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 53 deletions.
2 changes: 1 addition & 1 deletion cli/src/torii/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ pub(crate) const fn query_status_code(query_error: &query::Error) -> StatusCode
use query::Error::*;
match query_error {
Decode(_) | Evaluate(_) | Conversion(_) => StatusCode::BAD_REQUEST,
Signature(_) => StatusCode::UNAUTHORIZED,
Signature(_) | Unauthorized => StatusCode::UNAUTHORIZED,
Permission(_) => StatusCode::FORBIDDEN,
Find(_) => StatusCode::NOT_FOUND,
}
Expand Down
24 changes: 6 additions & 18 deletions cli/src/torii/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,14 +398,8 @@ async fn find_asset_with_no_account() {
asset_id("rose", "alice"),
)))
.await
.status(StatusCode::NOT_FOUND)
.body_matches_err(|body| {
if let query::Error::Find(err) = body {
matches!(**err, FindError::Account(_))
} else {
false
}
})
.status(StatusCode::UNAUTHORIZED)
.body_matches_err(|body| matches!(body, query::Error::Unauthorized))
.assert()
}

Expand Down Expand Up @@ -515,14 +509,8 @@ async fn find_account_with_no_account() {
AccountId::new("alice".parse().expect("Valid"), DOMAIN.parse().expect("Valid")),
)))
.await
.status(StatusCode::NOT_FOUND)
.body_matches_err(|body| {
if let query::Error::Find(err) = body {
matches!(**err, FindError::Account(_))
} else {
false
}
})
.status(StatusCode::UNAUTHORIZED)
.body_matches_err(|body| matches!(body, query::Error::Unauthorized))
.assert()
}

Expand Down Expand Up @@ -671,8 +659,8 @@ async fn query_with_no_find() {
// .deny_all()
.query(query())
.await
.status(StatusCode::NOT_FOUND)
.body_matches_err(|body| matches!(*body, query::Error::Find(_)))
.status(StatusCode::UNAUTHORIZED)
.body_matches_err(|body| matches!(body, query::Error::Unauthorized))
.assert()
}

Expand Down
21 changes: 13 additions & 8 deletions core/src/smartcontracts/isi/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ use crate::{ValidQuery, WorldStateView};
/// - grant permissions and roles
/// - Revoke permissions or roles
pub mod isi {
use super::{super::prelude::*, *};
use super::{
super::{prelude::*, query::Error as QueryError},
*,
};

#[allow(clippy::expect_used, clippy::unwrap_in_result)]
impl Execute for Register<Asset> {
Expand All @@ -28,13 +31,15 @@ pub mod isi {
let asset_id = self.object.id();

match wsv.asset(asset_id) {
Err(FindError::Asset(_)) => {
assert_can_register(&asset_id.definition_id, wsv, self.object.value())?;
wsv.asset_or_insert(asset_id, self.object.value().clone())
.expect("Account exists");
Ok(())
}
Err(e) => Err(Error::Find(Box::new(e))),
Err(err) => match err {
QueryError::Find(find_err) if matches!(*find_err, FindError::Asset(_)) => {
assert_can_register(&asset_id.definition_id, wsv, self.object.value())?;
wsv.asset_or_insert(asset_id, self.object.value().clone())
.expect("Account exists");
Ok(())
}
_ => Err(err.into()),
},
Ok(_) => Err(Error::Repetition(
InstructionType::Register,
IdBox::AssetId(asset_id.clone()),
Expand Down
35 changes: 18 additions & 17 deletions core/src/smartcontracts/isi/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,14 +397,13 @@ pub mod query {
.wrap_err("Failed to get asset id")
.map_err(|e| Error::Evaluate(e.to_string()))?;
iroha_logger::trace!(%id);
wsv.asset(&id)
.map_err(
|asset_err| match wsv.asset_definition_entry(&id.definition_id) {
Ok(_) => asset_err,
Err(definition_err) => definition_err,
},
)
.map_err(Into::into)
wsv.asset(&id).map_err(|asset_err| {
if let Err(definition_err) = wsv.asset_definition_entry(&id.definition_id) {
definition_err.into()
} else {
asset_err
}
})
}
}

Expand Down Expand Up @@ -543,12 +542,13 @@ pub mod query {
.map_err(|e| Error::Evaluate(e.to_string()))?;
iroha_logger::trace!(%id);
wsv.asset(&id)
.map_err(
|asset_err| match wsv.asset_definition_entry(&id.definition_id) {
Ok(_) => Error::Find(Box::new(asset_err)),
Err(definition_err) => Error::Find(Box::new(definition_err)),
},
)?
.map_err(|asset_err| {
if let Err(definition_err) = wsv.asset_definition_entry(&id.definition_id) {
Error::Find(Box::new(definition_err))
} else {
asset_err
}
})?
.value()
.try_as_ref()
.map_err(eyre::Error::from)
Expand All @@ -571,9 +571,10 @@ pub mod query {
.wrap_err("Failed to get key")
.map_err(|e| Error::Evaluate(e.to_string()))?;
let asset = wsv.asset(&id).map_err(|asset_err| {
match wsv.asset_definition_entry(&id.definition_id) {
Ok(_) => asset_err,
Err(definition_err) => definition_err,
if let Err(definition_err) = wsv.asset_definition_entry(&id.definition_id) {
Error::Find(Box::new(definition_err))
} else {
asset_err
}
})?;
iroha_logger::trace!(%id, %key);
Expand Down
3 changes: 3 additions & 0 deletions core/src/smartcontracts/isi/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ pub enum Error {
/// Query found wrong type of asset.
#[error("Query found wrong type of asset: {0}")]
Conversion(String),
/// Query without account.
#[error("Unauthorized query: account not provided")]
Unauthorized,
}

impl From<FindError> for Error {
Expand Down
19 changes: 10 additions & 9 deletions core/src/wsv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ use tokio::{sync::broadcast, task};
use crate::{
block::Chain,
prelude::*,
smartcontracts::{isi::Error, wasm, Execute, FindError},
smartcontracts::{
isi::{query::Error as QueryError, Error},
wasm, Execute, FindError,
},
DomainsMap, EventsSender, PeersIds,
};

Expand Down Expand Up @@ -126,7 +129,7 @@ impl WorldStateView {
///
/// # Errors
/// Fails if there is no domain or account
pub fn account_assets(&self, id: &AccountId) -> Result<Vec<Asset>, FindError> {
pub fn account_assets(&self, id: &AccountId) -> Result<Vec<Asset>, QueryError> {
self.map_account(id, |account| account.assets().cloned().collect())
}

Expand Down Expand Up @@ -261,11 +264,11 @@ impl WorldStateView {
/// - No such [`Asset`]
/// - The [`Account`] with which the [`Asset`] is associated doesn't exist.
/// - The [`Domain`] with which the [`Account`] is associated doesn't exist.
pub fn asset(&self, id: &<Asset as Identifiable>::Id) -> Result<Asset, FindError> {
self.map_account(&id.account_id, |account| -> Result<Asset, FindError> {
pub fn asset(&self, id: &<Asset as Identifiable>::Id) -> Result<Asset, QueryError> {
self.map_account(&id.account_id, |account| -> Result<Asset, QueryError> {
account
.asset(id)
.ok_or_else(|| FindError::Asset(id.clone()))
.ok_or_else(|| QueryError::Find(Box::new(FindError::Asset(id.clone()))))
.map(Clone::clone)
})?
}
Expand Down Expand Up @@ -552,11 +555,9 @@ impl WorldStateView {
&self,
id: &AccountId,
f: impl FnOnce(&Account) -> T,
) -> Result<T, FindError> {
) -> Result<T, QueryError> {
let domain = self.domain(&id.domain_id)?;
let account = domain
.account(id)
.ok_or_else(|| FindError::Account(id.clone()))?;
let account = domain.account(id).ok_or(QueryError::Unauthorized)?;
Ok(f(account))
}

Expand Down

0 comments on commit 9bfa772

Please sign in to comment.