Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

Having a generic authorization contract for each key #88

Open
alexvandesande opened this issue Oct 4, 2018 · 28 comments
Open

Having a generic authorization contract for each key #88

alexvandesande opened this issue Oct 4, 2018 · 28 comments
Labels
enhancement New feature or request

Comments

@alexvandesande
Copy link
Collaborator

Currently when you approve a new device to use your app, it is by default a "management key" which means that it has unlimited powers to do anything they want to the contract. The only limits on keys are given by the purpose and the requiredApprovals number set by ERC725 standard. This is insufficient.

Consider the case in which you might want to add a phone that is able to add and approve new devices on the contract, but you want to set limits on how many new keys they can add in one day to prevent abuse. Or maybe you want to be able to add access to a very limited scope, maybe a key that can only interact with a single contract. To do that we need to either replace or improve the key purpose concept with an "authorization contract" that authorizes a key to do things according to a given set of rules.

We want to keep the types of authorization contracts open, so for the purpose of this issue we would simplify with 2 example authorization contracts:

  • a contract that limits which contracts that key can interact with. It can be set to no contracts, only one specific contract, or any contract
  • a contract that limits the amount of "addKey" operations that each key can do, on the identity contract, per day. By default the logic would be: after adding N new keys, no new "addKeys" operations are accepted for N days

A proposed layout for how this would look for the user would be similar to the authorization screens of android or facebook apps, to be added on the "pendingAuthorization" screen:

screen shot 2018-10-04 at 10 57 14 am

@gitcoinbot

This comment has been minimized.

@gitcoinbot

This comment has been minimized.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 350.0 DAI (350.0 USD @ $1.0/DAI) attached to it as part of the Ethereum Community Fund via ECF Web 3.0 Infrastructure Fund fund__.__

@gitcoinbot
Copy link

gitcoinbot commented Oct 11, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 6 months, 4 weeks from now.
Please review their action plans below:

1) shine2lay has been approved to start work.

  • Create the General Authorization Contract interface (already have a version of it that I've been working on with Alex Van de Sande)
  • Create the two General Authorization Contract mentioned (each one using the interface + any specific functions to modify the authorization parameter)
  • Modify the SignedExecutionScheme.sol so that ACTION_KEY will always check authorization contract to see if the action provided is authorized before execution

Learn more on the Gitcoin Issue Details page.

@shine2lay
Copy link

@alexvandesande Here is my implementation plan, would love some feedback on it!

Additions:

  • GeneralAuthorizationInterface.sol
    • isActionAuthorized() (param list for each will be updated later)
    • initAuthorization()
    • addAuthorizedAccount()
    • revokeAuthorizedAccount()
    • actionExecuted()
  • AuthorizationWhiteListedAddress.sol (Checks if address is interacting with a whiteListed address)
  • AuthorizationKeyAdditionLimit.sol (Checks and keeps track of how many keys have been added by a certain address)
  • SignedExecutionScheme.sol:
    ** Modifications **
    • add a new parameter bytes extraData in executeSigned() similar to what is mentioned in https://eips.ethereum.org/EIPS/eip-1077 for future compatibility. This extraData parameter will include an address which points to authorizationContract to check.
    • before any action is executed, the contract will check the authorizationContract, with the approvers and action provided in the call,
    • if action is successfully executed, the actionExecuted() function in the authorization contract which will update the state in authorization contract.

Rationale:
This solution avoid any conflicts with the existing requiredApprovals construct already implement in the code base and since each key is not tied to a specific authorization contract, each key can be given many different authorized actions (using different authorization contracts)

@mkosowsk
Copy link

@shine2lay you have been approved to start work on this bounty :)

@alexvandesande please provide feedback on @shine2lay's action plan at your earliest convenience. Thanks!

@gitcoinbot
Copy link

gitcoinbot commented Oct 18, 2018

@shine2lay Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@shine2lay Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@alexvandesande
Copy link
Collaborator Author

alexvandesande commented Oct 19, 2018

I support Shine's action plan and we've talked previously about it. I would like to share a diagram description of the functions he describes:

screen shot 2018-10-19 at 9 08 34 am

In this case, the identity is the **executor** and an external chosen contract would be the **authorizationContract** (sometimes also called "terms of use")

@mkosowsk
Copy link

@alexvandesande Diagrams look awesome! Great way to share info 👍

@alexvandesande
Copy link
Collaborator Author

Something that we need to think about, which @mkosowsk raised, is how to handle multiple authorization contracts. If key A needs to call authorization contract A1 and key B calls authorization contract B1, then how does the identity handle when user A and B try to do action C? Does the contract call each authorization contract with both users on it? What if A1 doesn't know anything about user B? What if they are both authorized to do it in A1 but not in B1?

We could only allow one authorization contract per identity, but I feel it would be less flexible. Another option would be to make actionExecuted a function that is only "write only" meaning that the authorization contract is never supposed to "throw" it, only to acknowledge.

screen shot 2018-10-19 at 9 14 24 am

The idea here being that auth contracts act as advisors, it's ultimately the identity's job to decide either to do something or not, so each identity contract could decide how to deal with that. In this case, they could simply ask all auth contracts for all users and if a single auth contract authorizes it, then it proceeds with the action, and lets the other know that the action was performed anyway

@shine2lay
Copy link

shine2lay commented Oct 19, 2018

@alexvandesande that is an interesting use case, I agree with your point that it is identity's job to decide on the final verdict. actionExecuted should just return true or false instead of throwing.

When I thought about implementing this, I was thinking about adding a new field called authorizationContract to struct Execution and right before execution, we'll call this authorizationContract with all the approvers list and check if the action is authorized.

If key A needs to call authorization contract A1 and key B calls authorization contract B1, then how does the identity handle when user A and B try to do action C?

Maybe I'll need to rethink how to handle the scenario you mentioned

@gitcoinbot
Copy link

@shine2lay Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@shine2lay Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@shine2lay due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@shine2lay due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@rmshea
Copy link

rmshea commented Nov 8, 2018

Hey @shine2lay, Ryan from Gitcoin here. How is this coming along? I saw your PR, #119, but haven't seen much activity on this thread or the PR in a week or so.

@shine2lay
Copy link

@ryan-shea I was at DevCon last week, met some folks there and discussed solution for this PR. We are not entirely sure what is the best solution yet.

@gitcoinbot
Copy link

@shine2lay Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@shine2lay Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@shine2lay due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@shine2lay due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@spm32
Copy link

spm32 commented Nov 21, 2018

Hey @shine2lay any additional updates on this? Snoozing the bot :)

@rmshea
Copy link

rmshea commented Dec 10, 2018

Hey @shine2lay and @alexvandesande, any news here? Want to check in 😄

@shine2lay
Copy link

@ryan-shea no updates, can't progress until marek and i have a chat.
@ryan-shea could you remove me from the task? I feel bad that i can't put much time on it now. I don't want to hold up if anyone else wants to take on this task.

@spm32
Copy link

spm32 commented Dec 11, 2018

@shine2lay do you have a rough estimate of how long it would take you if you were to stay on?

@shine2lay
Copy link

@ceresstation unknown. like i mentioned the main reason is that i am blocked. We are attempting to unblock it but communication has been very slow.

@rmshea
Copy link

rmshea commented Jan 30, 2019

hey @shine2lay any updates here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants