Skip to content
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

[Request] Support full DID Syntax in CoreDID #1299

Closed
5 tasks
daniel-mader opened this issue Feb 6, 2024 · 8 comments
Closed
5 tasks

[Request] Support full DID Syntax in CoreDID #1299

daniel-mader opened this issue Feb 6, 2024 · 8 comments
Assignees
Labels
Request Request a feature
Milestone

Comments

@daniel-mader
Copy link
Collaborator

daniel-mader commented Feb 6, 2024

Description

When implementing a custom resolver for the did:web method, one is required to pass a CoreDID to the resolve() function. According to the spec of did:web, one needs to percent-encode the method-specific identifier to prevent the port from being interpreted as another method delimiter, for example: did:web:localhost%3A12345.
However, when calling CoreDID::parse("did:web:localhost%3A12345"), I am getting an InvalidMethodId error.
I do not see any conflict in supporting this as percent-encoding is part of the official DID Syntax.

Motivation

Allow implementation of resolver for did:web.

Requirements

  1. Allow parsing a percent-encoded did:web to a valid CoreDID.

Open questions (optional)

n/a

Are you planning to do it yourself in a pull request?

No

Tasks

Preview Give feedback
@abdulmth
Copy link
Contributor

abdulmth commented Feb 6, 2024

Hi @daniel-mader, unfortunately the error comes from the crate did-url. I'm not sure we can quickly fix it. I will take a closer look.

@eike-hass eike-hass changed the title [Request] [Request] Support full DID Syntax in CoreDID Feb 7, 2024
@eike-hass eike-hass added this to the v1.2 milestone Feb 7, 2024
@eike-hass eike-hass moved this from Product Backlog to Sprint Backlog in IOTA Identity - Framework Developments Feb 7, 2024
@daniel-mader
Copy link
Collaborator Author

The did-url crate looks unmaintained to me. Is forking it and fixing it yourself an option?

@UMR1352
Copy link
Contributor

UMR1352 commented Feb 12, 2024

@daniel-mader I forked and fixed did-url. You can use cargo's [path.crates-io] to get it. In your Cargo.toml file (where you have a dependency to identity_iota) add this:

[patch.crates-io]
did_url = { git = "https://github.com/iotaledger/did_url.git" }

We should be able to get ownership of that crate eventually, but for now this should get you going.

@daniel-mader
Copy link
Collaborator Author

Thank you @UMR1352! I replaced the dependency as you described and I now got this in my Cargo.lock:

[[package]]
name = "did_url"
version = "0.1.0"
source = "git+https://github.com/iotaledger/did_url.git#985e6179644db6c238228175c30883ef8b957840"
dependencies = [
 "form_urlencoded",
 "serde",
]

This looks correct to me, I also ran cargo clean and cargo update to make sure I got the right dependency.

However, I still get the InvalidMethodId when running identity_iota::did::CoreDID::parse("did:web:localhost%3A12345"). The only thing I can think of right now that could cause this is that CoreDID::parse() is not using did_url::DID::parse() as we think it does. Can you verify the fix by running CoreDID::parse() from within identity.rs?

@UMR1352
Copy link
Contributor

UMR1352 commented Feb 13, 2024

@daniel-mader you are right, identity_did performs a second validation of method_id in CoreDID::parse and that fails. Lemme look into this and reach back to you.

@UMR1352
Copy link
Contributor

UMR1352 commented Feb 13, 2024

@daniel-mader I fixed it, but unfortunately we can't merge it into main as long as we are linking to did_url through a git repo. I suggest you get the fix by linking identity_iota through the bug/did-url-par-encoding branch, like this:

identity_iota = { git = "https://github.com/iotaledger/identity.rs/tree/bug/did-url-par-encoding.git" }

Keep #1303 monitored, either we get ownership of did_url or we publish our fork to crates and we merge the PR.

@daniel-mader
Copy link
Collaborator Author

daniel-mader commented Feb 13, 2024

Thanks for the quick fix! I can verify it works for my use case! 🥳

I set the dependency like so:

identity_iota = { git = "https://github.com/iotaledger/identity.rs.git", branch = "bug/did-url-par-encoding" }

(Your suggestion somehow didn't work for me, I got a unexpected http status code: 404; for unknown reasons.)

I also assume that I do not need the [patch.crates-io] anymore, since it's included in your fix, correct?

@UMR1352
Copy link
Contributor

UMR1352 commented Feb 14, 2024

(Your suggestion somehow didn't work for me, I got a unexpected http status code: 404; for unknown reasons.)

My bad, I wrote the snipped without actually trying it. Glad you could make it work anyway.

I also assume that I do not need the [patch.crates-io] anymore, since it's included in your fix, correct?

Exactly, in that branch the library uses did_url through our fork so you don't have to patch it yourself.

@eike-hass eike-hass moved this from In Progress to Product Backlog in IOTA Identity - Framework Developments Mar 11, 2024
@eike-hass eike-hass moved this from Product Backlog to Sprint Backlog in IOTA Identity - Framework Developments Mar 13, 2024
@eike-hass eike-hass moved this from Sprint Backlog to In Progress in IOTA Identity - Framework Developments Mar 25, 2024
@eike-hass eike-hass moved this from In Progress to Done in IOTA Identity - Framework Developments Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Request Request a feature
Projects
Development

No branches or pull requests

4 participants