In local testing all authorizations are considered to have succeeded. But what if I want to ensure that my contract fails with the wrong authorization?
#Auth failure testing in local testing mode
trim temple
Actual example: I have a tiny nft project with a single contract function:
pub fn xfer(env: Env, to: Address) -> Address
The first time it’s called no authorization is performed and the to address becomes the owner. In subsequent calls I perform an authorization check on the owner and then make the to address the new owner.
In my tests how do I verify that ownership is not transferred when the wrong account is authorizing?
old turret
you can verify that the authorization from the owner address is required via recorded_top_authorizations. it's equivalent to saying 'if it's not present the contract will trap`
require_auth is declarative by its nature and it makes sense to test it in declarative fashion. it's basically part of your contract interface, even though it can be computed in a more dynamic fashion.
trim temple
this way of testing authorization feels pretty unintuitive to me.
As the developer I want to write a test that says "invoke this function authorized as user1" but instead of that I need to write a test that says "invoke this function with successful authorization and then check that it was authorized by user1"
old turret
right. I see how this may be non-intuitive. the approach you are suggesting is possible as well (though it would be more intrusive to the host logic). however, I'm not sure if it's strictly beneficial to the current approach. it would require you to cover auth for every invocation, which has been problematic with the previous approach. it's somewhat easier now that you don't need to do any signing, but still pretty is annoying - chances are you'd end up writing the helpers for every contract function that fill the required auth data
trim temple
it would require you to cover auth for every invocation, which has been problematic with the previous approach
why do you say it's problematic? I'm looking at our previous invoker auth test and it's much more intuitive (to me, at least)
```that's not flexible enough
you'd need to build the payloads, similar to what we have in transactions
and if you build the payloads, then chances are you start covering things you don't need to cover, like incorrect args etc
by 'previous approach' I mean advanced-auth. it's similar in that you had to specify the arguments that need to be signed
trim temple
I definitely don't want devs to start building payloads
old turret
well, that's the thign
trim temple
but if the perceived idea of auth next was "as simple as invoker auth, as powerful as advanced auth" we're not doing well on the former
old turret
I think in more complex cases it would be indeed more complex
I don't think it's significantly more complex for the simple cases
the testing approach is a matter of preference/getting used to
even with just the invoker auth before, the testing wasn't actually that simple, if you wanted to provide comprehensive coverage. besides a positive test, you'd want to do a bunch of negative tests (what if another account invoked this? what if a non-existent account invoked this? what if a contract invoked this?). the current approach clearly says that 'owner and only owner should have authorized this call', no matter how this has been invoked.
trim temple
so after playing around with this some more I'm coming around to agree that the "auto success" authorization is indeed desirable in most cases, where you don't want to bother yourself with testing authorization itself. However, I think we can make the success case more explicit and add other modes. Consider the following examples:
//current behavior
assert_eq!(client.with_auth_success().increment(&user_2, &4), 5);
//explicit payload
let user1_auth = std::vec![(
user_1.clone(),
contract_id.clone(),
symbol!("increment"),
(user_1.clone(), 5_u32).into_val(&env)
)];
assert_eq!(client.with_auth(user1_auth), increment(&user_1, &5), 7);
//invoker-like behavior
assert_eq!(client.with_auth_invoker(user_1), increment(&user_1, &5), 7);
not sure what is the invoker semantics; I suspect you mean something like with_default_auth(Vec<Address>) where we expect every Address to have called require_auth() at the top level
I have the same concern with the explicit approach - it sends the signal that the negative scenarios may need to be covered, which isn't the case
trim temple
can you clarify what's the negative scenario that you don't think people should test but they might feel the need to? It seems to me that "the wrong authorization provided" or "no authorization provided" is a case that the contract developer would like to test
old turret
It seems to me that "the wrong authorization provided" or "no authorization provided" is a case that the contract developer would like to test
that's what I mean - the interface reflects your intent of covering negative scenarios, but there is really no need to do that. all you would be covering is the host logic, and that's not something that should be tested in contracts
it's similar to covering e.g. storage access failures. we could in theory provide a way to invoke contract with the storage footprint, but covering this in the contract tests is pointless. I realize that the auth case is a bit different, but the main difference is that unlike storage you may want to verify that your expected authorized users/payloads look correct (which you can do). but the failure scenarios are similarly non-actionable
https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/test/token.rs#L1407 - this test exercises a similar interface but in the enforcing mode. I don't really want everyone to write such tests for every contract function that needs auth
trim temple
Maybe I'm not explaining myself properly or I don't understand you. I hear you - I'm not interested in testing the implementation of auth, but I am interested in testing my contract's use of it. I want to (a) ensure that I'm actually calling auth_required() and (b) that I'm doing it on the right account.
trim temple
Maybe I'm not explaining myself properly or I don't understand you. I hear you -...
these are not tests of how authorization works, they are tests that my contract is using it as intended
trim temple
https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/test/to...
I agree. these tests look like they test the auth implementation. That's not what I'm talking about
old turret
the only difference between approaches is pre- versus post-check. checked payloads are the same. however, in case of pre-check, it may seem like it makes sense testing negative scenarios (as you need to setup your 'mock' before even running the contract). in case of the post-check negative checks are clearly not needed, as you just verify how exactly the auth looks like
the example I sent covers these negative cases. most of them may seem sensible for recording auth as well. like, 'what if they authorize decrement instead of increment?'
trim temple
you're right. getting people to not test things like "what if they authorize decrement instead of increment" is an interesting problem to solve. However, this current solution also makes it idiosyncratic to ask questions like "what if tomer authorizes this action instead of dima", which we do want people to test for
old turret
if you think about this from the interface standpoint, then this question may not be needed. it's not dissimilar to a question 'what if my increment() function is invoked with a Symbol argument instead of no arguments'
i.e. it's about trusting the host to not allow the contract to succeed in case if the auth interface is not fulfilled
different Address may seem special, but it's not actually that special in reality. like in the example above, you may want to ask 'what if user A is an admin, but they authorized foo instead of bar'. this is a fair question in e2e scope, but it's pretty meaningless in the scope of the contract testing (because really any diff in the payload, be it address, fn name or args values/order would cause the contract to trap. not to mention the possible issues that aren't even visible to the contract, like incorrect signature, nonce etc.)
if you accept that the host will ensure that the correct address has authorized the payload you have asserted on in the test, then you shouldn't worry about the exact sorts of discrepancies that may appear during on-chain execution. I'm not strictly against passing this payload before running the contract, but I feel like it really results in some wrong assumptions
plush condor
I think the class of bugs Tomer is referring to are ones when the contract calls require_auth on the wrong Address. For example, you could have a bug where the contract takes an Address as input, but then for some reason calls require_auth on an Address it had in storage. This bug has nothing to do with the host implementation.
Now this is possible to test, but is counterintuitive because you need to do something like assert_eq!(env.recorded_top_authorizations(), std::vec![..]) after the auth call. I think Tomer is just asking for some utils or changes to the testing interface to make the auth check seamless. Maybe something like addr.authorized(client.increment()) or client.verify_addr_auth(addr).increment() (these aren't great because the other parameters are not verified but gets the point across)? @trim temple please correct me if I misunderstood.
old turret
I understand the sort of errors this tries to cover. however, args should be a part of the check as well (we could add a helper symmetric with require_auth). we also need to verify the contract id as the check may not happen in the top level.
in any case, I don't see how the check can be 'seamless'. what has been proposed so far is actually more verbose than what we have now (as you need to add something each contract call, one way or another)
I would be happy to introduce simplified helpers for the common cases. but I think the main argument is about whether the auth check should happen before or after the contract call. I'm not too keen on building a mocking framework on top of auth, just to allow users to test negative scenarios that actually don't need to be tested.
plush condor
Maybe "seamless" was the wrong word. I think the goal here is for the verification to be done on the same line.
old turret
I'm not sure if that should be a requirement...
I mean, it's not like we can't rewrite this into a one-liner with e.g. a lambda... not sure if we should though, but we could
plush condor
I'm not sure either. I'm just saying that's what I think Tomer wants. Maybe I'm wrong
old turret
the checks even can happen before the contract call (like client.expect_auth([...]).increment(...)). the key aspect to me is whether we do the post-check for equality (hence the expect name) vs setting up a mock-style fixture that should cause the contract to fail in case of a mismatch (as implied by with_ names). my understanding was that this is the main issue. but if we are not in disagreement here, then we can just iterate on the exact API
trim temple
I don't nesscarily agree with the "mock" classification since mocking usually implies some stateful setup, which I don't think is the case here
But ok - good discussion. I'll go back to my tiny nft contract, complete it and write some meaningful tests in the current format - maybe this will change mind.
old turret
hmm, so you don't suggest to do a stateful setup? (like setup the simplified auth entries before hand, then expect that the contract behavior depends on them)
trim temple
@old turret is this along the lines of what you would expect an auth test to look like? https://github.com/tomerweller/tinynft/blob/master/src/test.rs#L26
old turret
seems fine by me. could use the full tuple as well, but that's probably not necessary for the simpler contracts
I agree that the 'last invocation' part may be counter-intuitive. I think we could adjust the interface to explicitly tie the check to the invocation
trim temple
I think we could adjust the interface to explicitly tie the check to the invocation
how would you do that?
old turret
the simplest solution is with lambda, but maybe we can do without it, not sure
like what I proposed above - client.expect_auth(...).xfer(). this is less flexible though, so you won't be able to do shorthand check like in your example
old turret
lambda variant is something like env.record_auth(|| client.xfer(..)) that returns what is returned now from 'recorded_top_authorizations`
trim temple
@old turret do you mind filing an sdk issue that outlines your potential solution? After using the existing this for a bit I still feel like it's weird but not end-of-the-world-fix-asap weird
old turret
sure. both options I provided are trivial and can be implemented within a couple of hours (the most work would be to update the examples)
trim temple
thanks 🙏 . I also added my proposal from above
old turret
I don't think we've reached any conclusion on value of the mocking behavior and failure testing... I'm still not convinced there is a case for covering the negative scenarios, but there are definitely some substantial downsides (variety of possible negative cases that don't need to be covered; implementation complexity with introducing a third auth mode)
also more complex testing due to contract traps
(if you don't return a Result)
I just want to emphasize that require_auth failures aren't recoverable now
trim temple
yeah we definitely didn't reach a conclusion. I just wanted to surface my proposal as it's deep inside this thread (but not religious about it)
I just want to emphasize that require_auth failures aren't recoverable now
why is that a problem? are we against people writing#[should_panic]tests?
old turret
sure
I mean, not 'against'
but that's just a terrible ux for anything beyond trivial contracts
but anyway
my main point is that this will not cover anything substantial, compared to what is already covered
naive bolt
Just a few thoughts while writing tests:, for me this is super confusing, that in test mode everything succeeds.
I am used to specify the source account on the client representing the calling contract.
What speaks against doing it similar to hardhat in solidity with connect():
contractClient.with_source_account(address).foo();
it would require you to cover auth for every invocation, which has been problematic with the previous approach.
We could simply do:
contractClient.set_default_source_account(address);
Also: is there a technical reason why the env variable doesn't have the source account (or did I miss it)? Very often the auth mechanics are bloat the function interface and the source account should be metadata and not function signature (similar to Authorization in http).
As a developer I'd love to have something like:
let require_auth = true;
let source = env.source_account(require_auth);
The source_account function should even include a require_auth optional (or even required param).
Also, the recorded_top_authorizations assertion construction in the test is too bloated.
Great would be:
assert_eq!(client.foo(), Err(Ok(Error::Auth)));
old turret
I am used to specify the source account on the client representing the calling contract.
it's probably worth reading about soroban auth first - it's not based on the source accounts at all
mortal sonnet
for me this is super confusing, that in test mode everything succeeds.
This is my experience also. @naive bolt You may find this proposal of interest: https://github.com/stellar/rs-soroban-sdk/issues/875#issuecomment-1511780119
feral lynx
Just a few thoughts while writing tests:, for me this is super confusing, that i...
assert_eq!(client.foo(), Err(Ok(Error::Auth)));
I have to say this for sure feels a lot more Rust idiomatic 🙂 but I think @old turret's point is that an auth "error" would just cause a panic? (and that you should treat as such?)
I'm getting the message being: "if auth succeeds then every other scenario is just not possible" ? (and why waste time thinking about them or testing them?)
naive bolt
> I am used to specify the source account on the client representing the calling...
Ok, then I'm confused about the docs 🙂
... I'll read the docs again, but if it's not about the source account, then why not doing the same thing but just .set_default_account(...)?
I now read the invocation docs and i think it would help (at least me) insanely if there was a full client side example on how to build the ContractAuth xdr bit, e.g. for the JS library.
I still have problems processing this.
This is my understanding now:
- an account can authorize a contract to do specific actions.
add.require_auth()checks that this was explicitly given - I do not understand how I would configure this e.g. in javascript
- checking if the user is allowed to do things from the contract perspective is done using storage, i understand this from https://github.com/stellar/soroban-examples/blob/v0.7.0/token/src/admin.rs
naive bolt
```rust
assert_eq!(client.foo(), Err(Ok(Error::Auth)));
```
I have to say this ...
Hm. Is panic not the 500 Server equivalent of rust? I don't want my code to panic. I want clean, readable errors to surface to my dApp.
feral lynx
Hm. Is panic not the 500 Server equivalent of rust? I don't want my code to pani...
well... not really 🙂 it all depends on context. I'll just leave this here for ref:
https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html#to-panic-or-not-to-panic
(having said that, again, I do like the more idiomatic way of using an Err 🙂 )
naive bolt
I think i now have a solid understanding from the soroban side of things, but am still lacking how I would construct this in a dApp using the js SDK and freighter.
I'll move that to a different thread.
sand wyvern
I think i now have a solid understanding from the soroban side of things, but am...
FYI, the js side of that would be something like https://github.com/stellar/js-soroban-client/blob/main/src/server.ts#L374-L441, or just using the server.prepareTransaction method there. However, currently that doesn't support contract functions that take multiple Address arguments that need authorization.
