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

Adding Earner Name to AchievementSubject #537

Closed
kayaelle opened this issue Jul 21, 2023 · 19 comments · Fixed by #541
Closed

Adding Earner Name to AchievementSubject #537

kayaelle opened this issue Jul 21, 2023 · 19 comments · Fixed by #541

Comments

@kayaelle
Copy link
Contributor

Could someone provide an example of how they would include an earner name in the AchievementSubject? There's an identifier referencing the identity object but name isn't one of the explicit options.

Also is Profile intended to be used only with the issuer?

We have a credential where we want to include a person's name -- ideally in one name field (vs given & family name)

Thanks for your help!

@kayaelle
Copy link
Contributor Author

The @context does allow for "name" to be used throughout the credential. It is a high level property, not specific to AchievementSubject. The schema does not allow for name to be used in AchievementSubject. So it seems that either the schema should be adjusted or we should discuss how to include full names in the AchievementSubject.

At DCC, we have examples of name being included in credentials because it aids in subject verification.

@xaviaracil
Copy link
Collaborator

Why AchievementSubject is not extensible? In my opinion, it should be, so it could accept the fields for another types of CredentialSubjects (notice that the type property allow multiple types to be set in AchievementSubject).

@amiller-ims
Copy link
Collaborator

amiller-ims commented Jul 25, 2023

It is extensible through inheritence. The AchievementSubject model is derived from the CredentialSubject model which allows extensions.

https://github.com/1EdTech/openbadges-specification/blob/develop/ob_v3p0/common_credentials.lines#L42
https://github.com/1EdTech/openbadges-specification/blob/develop/ob_v3p0/common_credentials.lines#L201

For some reason, it is not showing as extensible in the spec HTML. Perhaps there is a bug in the renderer.

I did not check the generated JSON Schema, but would not be surprised if it does not allow additional properties since both the JSON Schema and the Spec HTML use the MPS to interpret the model.

Perhaps MPS logic was changed at some point to not propagate Mixins to derived types.

@kayaelle
Copy link
Contributor Author

I chatted with @ottonomy and he suggested that if I had type "Profile" to the CredentialSubject then name could be added but I'm still getting schema errors.

Could you provide an example of how this could be extended and how the schema could/would allow it?

Thanks!

@xaviaracil
Copy link
Collaborator

xaviaracil commented Jul 25, 2023

@amiller-ims I've checked MPS logic and it doesn't propagate Mixins extensions to derived types (https://github.com/1EdTech/unifiedmodel/blob/develop/unifiedmodel-core/src/main/java/org/imsglobal/unifiedmodel/core/build/modify/ExtensibilityHandler.java#L39). I'll add extensibility to AchievementSubject for now while looking deeper at that reason.

@xaviaracil
Copy link
Collaborator

@kayaelle Please update the schema, it should now allow additional properties.

@kayaelle
Copy link
Contributor Author

@xaviaracil Thanks!

I'm going to put a partial example here to see if this is how other's would like to add a earner/subject name to an achievementSubject:

{
	"@context": [
		"https://www.w3.org/2018/credentials/v1",
		"https://purl.imsglobal.org/spec/ob/v3p0/context-3.0.1.json",
		"https://w3id.org/security/suites/ed25519-2020/v1"],
		"id":"urn:uuid:115d5db1-59ee-460e-bb9f-0aac71eede6d",
		"type":["VerifiableCredential","OpenBadgeCredential"],
		"name":"Evaluator",
		"issuer":{
			"type":["Profile"],
			"id":"did:key:z6MkvCQzWzLE6t7CbvHLXkEpYrQeqCfem3rFds6ZDjmarLGU",
			"name":"Issuer Name",
		},
		"issuanceDate":"2023-07-20T14:52:38.637Z",
		"credentialSubject":{
			"id":"did:key:z6MkvCQzWzLE6t7CbvHLXkEpYrQeqCfem3rFds6ZDjmarLGU",
			"name": "Earner Full Name",
			"type":["AchievementSubject"],
			"achievement":{
				"id":"urn:uuid:bd6d9316-f7ae-4073-a1e5-2f7f5bd22922",
				"type":["Achievement"],
				"achievementType":"Badge",
				"name":"Documentation and SOP",
				"description":"Desscriotion here.",
				"criteria":{
					"type":"Criteria",
					"narrative":"Criteria narrative here"
				},
			},
		},
}

@ottonomy
Copy link
Contributor

ottonomy commented Jul 27, 2023

Thanks for stepping in and solving the issue with the schema @xaviaracil !

@kayaelle when we were talking about this, I mentioned the possibility of adding the Profile type to credentialSubject.type in addition to AchievementSubject. That would enable some things like:

"credentialSubject":{
    "id": "did:key:z6MkvCQzWzLE6t7CbvHLXkEpYrQeqCfem3rFds6ZDjmarLGU",
    "type": [ "AchievementSubject", "Profile" ],
    "givenName": "Stevie",
    "familyName": "MacBeth",
    ...

Because the givenName and familyName properties and many more about humans are in the type-specific context for Profile.

name as you used in your example @kayaelle is defined at the top level of the context, so no additional type is needed.

But sounds like indeed the issue was a mistaken "additionalProperties": false in the JSON schema. It was absolutely a bug if that was set to false for AchievementSubject.

I went through the schema and audited all other classes where additionalProperties is false. I'm not sure it is ever necessary to lock down the schema in this way, but certain places will be more restrictive of use cases than others. We've gone through several cases now one by one as we find what use cases had been blocked by the restrictions.

  • IdentityObject: this seems fine, very specific to this spec
  • IdentifierEntry: this seems fine
  • AchievementCredential: this is risky. It seems likely that in the future, there will be many use cases for other data that might appear at the root level of the Verifiable Credential.
  • EndorsementCredential: this seems risky, same as above
  • Image: this seems like a potential minor irritation. There are a lot of properties on schema:ImageObject that implementers might at some point want to play around with in order to enable additional accessibility features or describe rendering, for instance. Or the MIME type and height and width.
  • Related: I think this should change. The idea of "related" is to be able to express a connection to a different object that differs from the present object. Being able to describe how that object differs is done by expressing that difference. There may be many reasons that issuers might want to experiment with this beyond the language and version ones hinted at in the app. (for example, the implementation guide recommends using version to link to the OB 2.0 representation of the object if relevant, which is a slight extension of the idea of version already).
  • EndorsementSubject: this needs to change, an exact parallel to the present issue that has been solved. Comments are cool, but being able to assert other things about an endorsed object could be quite important for certain use cases.
  • Address: I could see some use cases for trying out some additional properties from PostalAddress like hoursAvailable in some cases.
  • GeoCoordinates: I could see products wishing to use the elevation property as well as the ones defined in our spec.

And from the CLR schema, which did not seem to have the same problems. ClrSubject for instance wasn't restricted already.

  • Image: see note above
  • IdentityObject: see note above, seems fine
  • Association: I could definitely see implementers wishing to provide more metadata about associations, such as a description. Ok to have it restricted, though, because it's very specific to this spec. It cuts off this minor experimentation for years if it stays restricted in CLR 2.0 though.
  • IdentifierEntry: see note above, seems fine
  • Related: see note above
  • Address: see note above
  • GeoCoordinates: see note above
  • EndorsementCredential: see note above
  • EndorsementSubject: see note above

@xaviaracil let me know if you want me to open any additional issues to track these.

@kayaelle
Copy link
Contributor Author

kayaelle commented Aug 8, 2023

Thanks @ottonomy - apologies for delay. Will review your comment above and reply soon.

@kayaelle
Copy link
Contributor Author

Here's a question - if we can now use "name" in credentialSubject and name is an attribute of profile but also a high level attribute that is used elsewhere, do we need to add Profile to type? givenName, familyName, are specific to profile but name is not and for many (especially international implementations) a single filed for an earner's name is appropriate.

I don't have an opinion either way but I think it will be helpful for the group to make a recommendation and document it because many AchievementTypes will want to include the earner name including degrees, licenses, etc.

Thanks!

@kayaelle
Copy link
Contributor Author

kayaelle commented Aug 10, 2023

Additional Question - should the earner name be one of the identities in the IdentityObject instead of using Profile? seems like name could be added to this class

And that would make it clear OR Profile could be added to this?

OR... is userName close enough?

@justinpitcher
Copy link
Contributor

I am leaning towards yeah, adding a new type to IdentifierTypeEnum for personName(?) and then it just gets added to the IdentityObject

@ottonomy
Copy link
Contributor

if we can now use "name" in credentialSubject and name is an attribute of profile but also a high level attribute that is used elsewhere, do we need to add Profile to type?

You do not need to add Profile to credentialSubject.type to use the name property. The only exceptions would be around edge cases of precedence of handling of @protected definitions (probably intentional manipulation specifically to create a confusing condition). It may be possible to construct a credential in which credentialSubject.name did not match the mapping of the OB context for name through use of another context that defines name in a type-specific context and application of that type in credentialSubject.type.

is userName close enough?
I don't think userName is close enough.

As to what's "best", it's kind of up to how verifiers want to work to interpret various options. I think it would be more likely that such verifiers would have implemented some kind of handling for identifier versus for properties from Profile (probably at a basic level just for displaying all included to users, with occasional specific handling, such as for authentication of emailAddress) of IdentityObject values.

Without specific coordination between issuers, wallets, and verifiers, I would expect that data from Profile like givenName and familyName would be much less likely to result in a display to users or handling based on specific business logic.

@ottonomy
Copy link
Contributor

Whoops, my commit message on the wrong branch accidentally closed this issue. Resolving.

@kayaelle
Copy link
Contributor Author

I'm leaning with @justinpitcher. Adding personName to the Identity Object is inline with the other identifiers.

@ottonomy
Copy link
Contributor

ottonomy commented Sep 1, 2023

@kayaelle cool, the contents of PR #541 in the implementation guide could be pretty easily modified to change ext:name to the new term when the new term is added to allowed values.

@kayaelle
Copy link
Contributor Author

kayaelle commented Sep 7, 2023

Hey @ottonomy - this is a step in the right direction but I will argue on behalf of the DCC and our members that are issuing degrees using Open Badges that name should be an explicit in the list of identifiers: https://1edtech.github.io/openbadges-specification/ob_v3p0.html#org.1edtech.ob.v3p0.identifiertypeenum.class

Name will be used for subject verification where an earner's id could be presented at the time of presentation. If userName is on this list and email address, I don't see why name can't be added too so that the ecosystem has a consistent and interoperable way to identify their subjects by name.

Request that we reopen this one and discuss at the next WG. Thanks!

@kayaelle kayaelle reopened this Sep 7, 2023
xaviaracil pushed a commit that referenced this issue Sep 7, 2023
@xaviaracil xaviaracil reopened this Sep 8, 2023
@xaviaracil
Copy link
Collaborator

xaviaracil commented Sep 8, 2023

Reopened for further discussion about adding a new IdentifierType for Name. Changes to the schema for allowing extensibility made

@xaviaracil
Copy link
Collaborator

New IdentifierType added on September 22th.

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

Successfully merging a pull request may close this issue.

5 participants