-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve did:web #24
Resolve did:web #24
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
pub mod jwk; | ||
pub mod web; | ||
|
||
use crate::did::Did; | ||
use crate::resolver::DidResolutionResult; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
use crate::did::Did; | ||
use crate::method::{DidMethod, DidMethodError, DidResolutionResult}; | ||
use async_trait::async_trait; | ||
use crypto::key_manager::KeyManager; | ||
use did_web::DIDWeb as SpruceDidWebMethod; | ||
use ssi_dids::did_resolve::{DIDResolver, ResolutionInputMetadata}; | ||
use std::sync::Arc; | ||
|
||
/// Concrete implementation for a did:web DID | ||
pub struct DidWeb { | ||
uri: String, | ||
key_manager: Arc<dyn KeyManager>, | ||
} | ||
|
||
impl Did for DidWeb { | ||
fn uri(&self) -> &str { | ||
&self.uri | ||
} | ||
|
||
fn key_manager(&self) -> &Arc<dyn KeyManager> { | ||
&self.key_manager | ||
} | ||
} | ||
|
||
/// Options that can be used to create a did:web DID. | ||
/// This is currently a unit struct because did:web does not support key creation. | ||
pub struct DidWebCreateOptions; | ||
|
||
#[async_trait] | ||
impl DidMethod<DidWeb, DidWebCreateOptions> for DidWeb { | ||
const NAME: &'static str = "web"; | ||
|
||
fn create( | ||
_key_manager: Arc<dyn KeyManager>, | ||
_options: DidWebCreateOptions, | ||
) -> Result<DidWeb, DidMethodError> { | ||
Err(DidMethodError::DidCreationFailure( | ||
"create operation not supported for did:web".to_string(), | ||
)) | ||
} | ||
|
||
async fn resolve_uri(did_uri: &str) -> DidResolutionResult { | ||
let input_metadata = ResolutionInputMetadata::default(); | ||
let (did_resolution_metadata, did_document, did_document_metadata) = | ||
SpruceDidWebMethod.resolve(did_uri, &input_metadata).await; | ||
|
||
DidResolutionResult { | ||
did_resolution_metadata, | ||
did_document, | ||
did_document_metadata, | ||
..Default::default() | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn create_fails() { | ||
let key_manager = Arc::new(crypto::key_manager::LocalKeyManager::new_in_memory()); | ||
let result = DidWeb::create(key_manager, DidWebCreateOptions); | ||
assert!(result.is_err()); | ||
} | ||
|
||
#[tokio::test] | ||
async fn resolution_success() { | ||
let did_uri = "did:web:tbd.website"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to add a test like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed 👍 It's strange to me that this test actually passes, though I see the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this valid? I'm trying to use https://dev.uniresolver.io/ and it's not able to resolve this 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, maybe uniresolver doesn't support DID web! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did a little digging. Resolving this DID currently fails because it's using an unknown VerificationMethod property Is there another spec that does include this? If so, why have these diverged? @decentralgabe maybe you have some extra insight here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @amika-sq it's not included in the default context for did-core, but it is included in the security vocab here -- https://github.com/w3c-ccg/security-vocab/pull/40/files this means to make that did web valid it would have to add the context like this {
"@context": ["https://w3id.org/did/v1", "https://w3id.org/security/v3-unstable"],
"id": "did:web:users.tbddev.org:demo",
"verificationMethod": [
{
"id": "did:web:users.tbddev.org:demo#key-0",
"type": "EcdsaSecp256k1VerificationKey2019",
"publicKeyHex": "04e8e82698e64db79e13dc6e7478f47a5fe867c4acd6fa8928b0c93c763a02fd6c54a622de247cc04449531429dbb6a2373b6c685a8e1326913e0531e5f649208d"
}
]
} or not use hex There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. really we should be using JWKs everywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
let result = DidWeb::resolve_uri(did_uri).await; | ||
assert!(result.did_resolution_metadata.error.is_none()); | ||
|
||
let did_document = result.did_document.expect("did_document not found"); | ||
assert_eq!(did_document.id, did_uri); | ||
} | ||
|
||
#[tokio::test] | ||
async fn resolution_failure() { | ||
let result = DidWeb::resolve_uri("did:web:does-not-exist").await; | ||
assert!(result.did_resolution_metadata.error.is_some()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this doing a real resolution? should it be mocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a real resolution. I can add a mock if we want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, Spruce is testing mocked responses here: https://github.com/spruceid/ssi/blob/main/did-web/src/lib.rs#L197-L218
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer not mocking but I believe we're mocking elsewhere in our libs. Just flagging because the test could fail even if it's correct, since it relies on an external resource. if there's a way to distinguish network tests somehow that would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second the intuition of finding a way to distinguish an integration test vs a unit test, though maybe unnecessary at this stage and we could stub in a GitHub Issue on a backlog to address later