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

Update the identifier validation in the anoncreds-data-types module #9

Closed
7 tasks
TimoGlastra opened this issue Nov 3, 2022 · 14 comments
Closed
7 tasks
Assignees

Comments

@TimoGlastra
Copy link
Member

The anoncreds-data-types module contains an identifiers directory with logic around creation, splitting and validation of the identifiers in anoncreds objects.

The AnonCreds specification has updated identifiers to allow both the legacy indy style identifiers (different per object type) and URIs.

As identifiers don't contain specific elements anymore, but just an URI we need to determine how much validation is still needed.

These are the identifiers that should be updated:

  • CredentialDefinitionId
  • RevocationRegistryId
  • SchemaId

This task only focusses on adding validation support for the URI type, but doesn't remove logic related to handling the legacy identifier type, this will be handled in separate tasks.

For each of the identifiers the following should happen:

  • Update the Validatable implementation for the identifier type to accept both the legacy indy style (currently implemented) and the new URI style according to the AnonCreds specification.
  • Remove the Qualifiable implementation for the identifier as it isn't needed for the anoncreds implementation
  • Add utility methods to determine if the identifier is of type legacy indy identifier or URI. Can be either through something like an identifier type (uri, legacy_indy), or just adding two methods is_legacy_indy_identifier() and is_uri_identifier() (thinks the second is simplest).
  • Rename .parts() to .legacy_indy_identifier_parts(). This should throw an error if the type of the identifier is not a legacy indy identifier
@dkulic dkulic self-assigned this Nov 16, 2022
@dkulic
Copy link
Contributor

dkulic commented Nov 16, 2022

Where can I find more documentation of the id format?
I looked into https://hyperledger.github.io/anoncreds-spec/ but didn't find any clear explanation.

@TimoGlastra
Copy link
Member Author

Id format can be the legacy indy style identifier (which is different per object type) or an URI

@TimoGlastra
Copy link
Member Author

There's a short section on it in the anoncreds spec: https://hyperledger.github.io/anoncreds-spec/#anoncreds-identifiers

@dkulic
Copy link
Contributor

dkulic commented Nov 16, 2022

So I should assume it is a generic URI and try to validate. I thought there will be more details, but this should be sufficient.
Thanks.

@TimoGlastra
Copy link
Member Author

Not exactly. You should assume it's either a legacy type indy identifier (and validate it like it is done now) or a generic URI

@dkulic
Copy link
Contributor

dkulic commented Nov 16, 2022

Yeah, it is clear for the legacy. I was thinking how in other case we may do validation.

@dkulic
Copy link
Contributor

dkulic commented Nov 17, 2022

I had some thinking regarding how to differentiate legacy id from new id, and it is harder than I expected. Because URI can be consisted of parts separated by colon (:), is_legacy_indy_identifier can return wrong result. Any ideas?

@dkulic
Copy link
Contributor

dkulic commented Nov 18, 2022

Qualified methods (mostly to_unqualified()) are used a lot trough the code. I am not sure how to deal with them, if we remove support, then the user would need to know which one to supply which may break backwards compatibility.

@TimoGlastra
Copy link
Member Author

I had some thinking regarding how to differentiate legacy id from new id, and it is harder than I expected. Because URI can be consisted of parts separated by colon (:), is_legacy_indy_identifier can return wrong result. Any ideas?

I think the legacy indy identifier is quite speicifc so if something exactly matches it it would cause issues in a lot of other places (as you're not able to detect it from a real legacy identifier).

E.g. a legacy schema identifier has the structure of:

<did>:2:<schema-name>:<schema-version>

Where did is base58 22-23 characters, and the :2: should provide enough uniqueness I think

@TimoGlastra
Copy link
Member Author

Qualified methods (mostly to_unqualified()) are used a lot trough the code. I am not sure how to deal with them, if we remove support, then the user would need to know which one to supply which may break backwards compatibility.

AFAIK real qualified was never really used yet and I think we should remove it completely. there's no qualified and unqualified, there's just legacy and URI based identifiers now. The user is going to need the identifier from now on as we can't generate it anymore, thus we're going to break the current API anyway. I would say just remove it.

@TimoGlastra
Copy link
Member Author

@dkulic what is the status of this issue?

@blu3beri with all PRs you merged, is this issue now completed?

@berendsliedrecht
Copy link
Contributor

Yes I believe so. This is the regex I used for URI validation:

let uri_regex = regex::Regex::new(r"^[a-zA-Z0-9\+\-\.]+:.+$").unwrap();

I think it is correct and I tested it with some examples. I will create some tests for this inside the code to make sure it works correct.

@dkulic
Copy link
Contributor

dkulic commented Jan 5, 2023

Hi, sorry for late response. As far as I can tell, @blu3beri has done the work requested in this ticket (except for legacy_indy_identifier_parts() method). Do we need it, I guess not?

@TimoGlastra
Copy link
Member Author

No guess it's not needed after all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants