spruceid / ssi

Core library for decentralized identity.
https://spruceid.dev
Apache License 2.0
180 stars 54 forks source link

Enforce non-empty subject for VCDM v2 #572

Closed sbihel closed 2 weeks ago

sbihel commented 2 weeks ago

As described in the specs https://www.w3.org/TR/vc-data-model-2.0/#credential-subject

timothee-haudebourg commented 2 weeks ago

There is a difference between "A verifiable credential must have a credentialSubject property" and "the credentialSubject property must not be empty". So do we really need to ensure it is non-empty? Or just require it is present?

sbihel commented 2 weeks ago

There is a difference between "A verifiable credential must have a credentialSubject property" and "the credentialSubject property must not be empty". So do we really need to ensure it is non-empty? Or just require it is present?

You're right, but I think simply requiring it to be present doesn't make much sense because:

But I'll defer to your judgement

timothee-haudebourg commented 2 weeks ago

I don't think it's in the spirit of the specs

I would tend to agree? I think? It's really hard to tell. If we stick to the JSON-LD interpretation, even if the value is a single object and not an array, then JSON-LD will expand it into an array. But then if this array is empty the RDF deserialization algorithm won't generate a triple with the https://www.w3.org/2018/credentials#credentialSubject property, which may be what they really mean by "must have a credentialSubject property".

Just to understand what we're loosing here: I can see some situations where the app adds the credential subjects to an existing credential like this:

fn populate_credential(cred: &mut JsonCredential) {
  cred.credential_subjects.push(stuff)
}

This pattern would not be possible if we forbid empty credential subjects at the type-system level. And also its easier to implement, just by changing

#[serde(
    with = "value_or_array",
    default,
    skip_serializing_if = "Vec::is_empty"
)]
pub credential_subjects: Vec<S>,

into

#[serde(with = "value_or_array")]
pub credential_subjects: Vec<S>,

With that in mind, I let you choose if you want to proceed, I convinced myself that I don't have a strong opinion here. Just remove the Clone bound if you go ahead :smile:

sbihel commented 2 weeks ago

I've decided to move forward with this because I think it would be too easy to make a mistake otherwise (and the API already requires a subject at initialisation time)