uport-project / ethr-did

Create ethr DIDs
Apache License 2.0
259 stars 53 forks source link

DID method creates new Ethr-DID without passing singer or private key #35

Closed m-yahya closed 4 years ago

m-yahya commented 4 years ago

Description In the documentation, it is mentioned that 'either signer or privateKey' is required to create a new Ethr-DID. However, a new Ethr-Did can be created just passing the Ethereum address.

To Reproduce

  1. init npm project
  2. paste the following code into inde.js
  3. configure the provider
  4. install the dependencies ethjs-provider-http@0.1.6 and ethr-did@1.1.0
  5. run node index.js

Sample Code

const HttpProvider = require('ethjs-provider-http')
const provider = new HttpProvider('http://127.0.0.1:8545')

const EthrDID = require('ethr-did');

let createDid = async () => {

    const address = '0xCF402F0891f9551eA0e2cEE7A7e491C4e83Fc079';
    const ethrDid = new EthrDID({ address });
    console.log(ethrDid);

}

createDid()

Expected behavior It should throw an error stating 'either signer or private key is required'.

Actual behavior It successfully creates a new did.

Screenshots image

Dependencies:

mirceanis commented 4 years ago

Thank you for the report. This is intended behavior since the instance you create using only the address is still usable to encapsulate an external ethr-did (for which you do not possess the private key).

An instance created using only an address(const instance = new EthrDID({ address });) can:

Other methods that require signing should throw an error when called. If that is not the case, then it is a bug.

m-yahya commented 4 years ago

Thank you very much for your clarification.

An instance created using only an address(const instance = new EthrDID({ address });) can:

  1. be resolved using resolve(eth.did)
  2. unable to sign JWT (Error: No signer configured)
m-yahya commented 4 years ago
  • lookup its owner await instance.lookupOwner()

I have created a new instance: const ethrDid = new EthrDID({ provider, address, registry }); Here is the resolved did:

{
    '@context': 'https://w3id.org/did/v1',
        id: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c',
            publicKey:
    [{
        id: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c#owner',
        type: 'Secp256k1VerificationKey2018',
        owner: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c',
        ethereumAddress: '0xab8696a3214d09e3797affa6d4fe3a52568a0d3c'
    }],
        authentication:
    [{
        type: 'Secp256k1SignatureAuthentication2018',
        publicKey: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c#owner'
    }]
}

after calling the const changeOwnerTx = await ethrDid.changeOwner('0xde3eb6a3ab023f1d5d7e658c69161609fb5b6a94'), the resolved did is:

{
    '@context': 'https://w3id.org/did/v1',
        id: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c',
            publicKey:
    [{
        id: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c#owner',
        type: 'Secp256k1VerificationKey2018',
        owner: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c',
        ethereumAddress: '0xde3eb6a3ab023f1d5d7e658c69161609fb5b6a94'
    }],
        authentication:
    [{
        type: 'Secp256k1SignatureAuthentication2018',
        publicKey: 'did:ethr:0xab8696a3214d09e3797affa6d4fe3a52568a0d3c#owner'
    }]
}

Here I'm expecting owner as updated owner. But only the ethereumAddress is updated. However, ethrDid.lookupOwner() is returning updated owner. is this also intended behavior?

mirceanis commented 4 years ago

It was at the time of writing the resolver. However, the W3C spec regarding the public keys has changed. The owner is no longer a recognized property of a public key. The spec describes a controller property but that may have slightly different semantics.

I suggest not using the owner property of the resolved document at this time/version. If your use case requires that you find the owner as you expect, it is probably safer to call the registry contract directly instead of relying on the property that may disappear in a future version.

Thanks for spotting this

m-yahya commented 4 years ago

Thank you very much for your explanation.