contract auth #598
contract auth #598
Conversation
| auto& sessions = sc.sessions(); | ||
| for (auto& session : sessions) | ||
| if (session.expiration <= now) | ||
| push_event(session_del_event{sc.eden_account(), session.key}, contract); |
There was a problem hiding this comment.
I think this excessively exposes implementation details. The contract behaves (or should behave) as if the session key is automatically removed when the expiration passes.
There was a problem hiding this comment.
I'm split. It makes sense for the history side to be aware of that, but we were bitten recently by the history side knowing those things.
| eosio::public_key key; | ||
| eosio::block_timestamp expiration; | ||
| std::string description; | ||
| std::vector<eosio::varuint32> sequences; |
There was a problem hiding this comment.
I think we need to document clearly that it is unsafe to reuse session keys. The contract does not prevent reuse.
contracts/eden/include/eden.hpp
Outdated
| void execsession(const eosio::signature& signature, | ||
| eosio::ignore<eosio::name> eden_account, | ||
| eosio::ignore<eosio::varuint32> sequence, | ||
| eosio::ignore<std::vector<action>> actions); |
There was a problem hiding this comment.
I don't like this interface much
- The requirement of assigning unique sequential integers to the actions is ugly
- It doesn't allow authorization actions on a different contract, which could become a problem if we do need to put functionality into a separate contract. I'd prefer to see
std::vector<eosio::action>or eveneosio::transaction - The fact that there is one action that can behave the same as many other actions makes filtering harder. Ideally, I'd like to see this run the other actions as inline actions.
There was a problem hiding this comment.
If we come up with an alternative, I don't want it to sacrifice the ability of existing history tools to create a human-readable decoding of the original request. Right now they can use ABI decoding to show what's in the action vector.
There was a problem hiding this comment.
For history purposes,I don't see why it's important that the original request is human readable vs. having a human readable decoding in the trace. Having the original request human readable is normally critical for signing, but that's not an issue here.
There was a problem hiding this comment.
a human readable decoding in the trace
I like this too, starting to look good... but still hard to digest in block explorer (needs to open the transactions first and then look for traces)
Add contract auth support
Phase 1 (this PR): Only actions which support session keys are dispatchable through the
runaction. All actions support the normal dispatch.Phase 2 (future): All actions will be dispatchable through the
runaction. Ones which don't support session keys will requireaccount_authorno_auth; they won't supportsignature_auth. TBD if this really happens.Phase 3 (future): Normal dispatch will be disabled for actions other than
newsessionandrun. TBD if this really happens.