Uptake JSON Schema for FPWD #494

decentralgabe · 2023-05-26T23:07:25Z

Fixes #493

refactored some cred logic along the way
refactored some config logic to include base paths, needed for surfacing querying of schemas
disabled signing schemas for now; to be re-enabled in Support CredentialSchema2023 #501

decentralgabe · 2023-05-30T21:03:38Z

pkg/server/server_test.go

@@ -129,22 +129,26 @@ func newRequestContextWithParams(w http.ResponseWriter, req *http.Request, param
 	return c
 }

-func getValidManifestRequest(issuerDID, issuerKID, schemaID string) model.CreateManifestRequest {
+func getValidCreateManifestRequest(issuerDID, issuerKID, schemaID string) model.CreateManifestRequest {
 	return model.CreateManifestRequest{


updated this test to use real input/output descriptors for readability

decentralgabe · 2023-05-30T21:03:55Z

pkg/service/credential/service.go

 	if request.SchemaID != "" {
 		// resolve schema and save it for validation later
 		gotSchema, err := s.schema.GetSchema(ctx, schema.GetSchemaRequest{ID: request.SchemaID})
 		if err != nil {
 			return nil, sdkutil.LoggingErrorMsgf(err, "failed to create credential; could not get schema: %s", request.SchemaID)
 		}
 		knownSchema = &gotSchema.Schema
-
+		schemaType := schemalib.CredentialSchema2023Type
+		if gotSchema.CredentialSchema == nil {


for now this branch will always be true

As mentioned in other places, I think we should completely remove.

decentralgabe · 2023-05-30T21:04:09Z

pkg/service/credential/status.go

@@ -0,0 +1,97 @@
+package credential


moved this to another file for readability

codecov-commenter · 2023-05-30T21:35:14Z

Codecov Report

Merging #494 (e5e95da) into main (47db532) will increase coverage by 0.44%.
The diff coverage is 12.21%.

❗ Current head e5e95da differs from pull request most recent head dfa57f6. Consider uploading reports for the commit dfa57f6 to get more accurate results

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   20.57%   21.02%   +0.44%     
==========================================
  Files          48       49       +1     
  Lines        5769     5803      +34     
==========================================
+ Hits         1187     1220      +33     
+ Misses       4373     4358      -15     
- Partials      209      225      +16

Impacted Files	Coverage Δ
integration/common.go	`1.96% <ø> (ø)`
pkg/server/router/did.go	`4.73% <ø> (ø)`
pkg/server/router/schema.go	`9.63% <0.00%> (+0.84%)`	⬆️
pkg/server/server.go	`68.05% <0.00%> (-0.15%)`	⬇️
pkg/service/credential/model.go	`0.00% <ø> (ø)`
pkg/service/credential/service.go	`0.00% <0.00%> (ø)`
pkg/service/credential/status.go	`0.00% <0.00%> (ø)`
pkg/service/credential/storage.go	`1.68% <0.00%> (ø)`
pkg/service/did/handler.go	`0.00% <0.00%> (ø)`
pkg/service/did/resolution/resolver.go	`0.00% <0.00%> (ø)`
... and 6 more

andresuribe87 · 2023-05-30T21:12:01Z

config/config.go

+			BaseServiceConfig: new(BaseServiceConfig),
+		}
+	}
+	services.DIDConfig.ServiceEndpoint = endpoint + "/dids"


If someone sets the service_endpoint for a single service and not others, seems like this will override. Is that intentional? Should defaults be applied before decoding the file?

this needs to be refactored because we don't want to enable config for setting service endpoints. I'll open a new issue

andresuribe87 · 2023-05-30T21:14:36Z

pkg/server/router/manifest_test.go

@@ -155,6 +165,14 @@ func TestManifestRouter(t *testing.T) {
 			ID:       storage.StatusObjectID(createdApplicationResponseOp.ID),
 			Approved: true,
 			Reason:   "ApprovalMan is here",
+			CredentialOverrides: map[string]model.CredentialOverride{


Why add this here? I.e. was there a test that was failing? Or was the test not exercising the desired behavior?

the schema the test was using did not have any required fields - so it wasn't a great test. once I added them this is necessary

andresuribe87 · 2023-05-30T21:15:21Z

pkg/server/router/schema.go

+	Name        string `json:"name" validate:"required"`
+	Description string `json:"description,omitempty"`


Mind adding godocs for these two fields?

andresuribe87 · 2023-05-30T21:19:03Z

pkg/server/router/schema.go

 	Schema schemalib.JSONSchema `json:"schema" validate:"required"`

+	// TODO(gabe): re-enable in https://github.com/TBD54566975/ssi-service/issues/493


I don't quite understand why this change requires disabling an already existing feature. Was it broken?

it's a slightly different feature, since before we weren't creating credentials just JWTs

andresuribe87 · 2023-05-30T21:21:02Z

pkg/server/router/schema.go

Why not keep the verification endpoint?

since it will be a credential and we already have a credential verification endpoint

we could add a new endpoint to see whether a cred is valid against a schema - but that's a new ticket

andresuribe87 · 2023-05-31T16:03:11Z

pkg/service/schema/model.go

-	Reason   string `json:"reason,omitempty"`
+	ID               string            `json:"id"`
+	Schema           schema.JSONSchema `json:"schema"`
+	CredentialSchema *keyaccess.JWT    `json:"credentialSchema,omitempty"`


I don't think this should be returned from the schema.Service.

going to remove this and save it for the next issue

andresuribe87 · 2023-05-31T16:04:16Z

pkg/service/schema/model.go

-	SchemaJWT *keyaccess.JWT      `json:"schemaJwt,omitempty"`
+	ID               string            `json:"id"`
+	Schema           schema.JSONSchema `json:"schema"`
+	CredentialSchema *keyaccess.JWT    `json:"credentialSchema,omitempty"`


Same as above. Suggestion is that schema.Service only stores plain schemas.

let's take this in the next issue

andresuribe87 · 2023-05-31T16:30:28Z

pkg/service/schema/service.go

 	}

+	// TODO(gabe) support signing credential schemas
+	// create schema
+	storedSchema := StoredSchema{ID: schemaID, Schema: jsonSchema}


There are two IDs within a StoredSchema: id and schema.$id. I think it's easily confused. Would it make sense to only have one?

the reason for two: when you view the schema on it's own you need a fully qualified URI to resolve it (e.g. tbd.website/schemas/01841408-1408140148

when you are talking to the service, it is awkward to request anything but 01841408-1408140148

so 01841408-1408140148 is the service identifier, whereas the $id is the fully qualified identifier

andresuribe87 · 2023-05-31T16:31:50Z

pkg/service/schema/storage.go

 		return nil, nil
 	}
 	var stored []StoredSchema
 	for _, schemaBytes := range gotSchemas {
 		var nextSchema StoredSchema
-		if err := json.Unmarshal(schemaBytes, &nextSchema); err == nil {
+		if err = json.Unmarshal(schemaBytes, &nextSchema); err == nil {


Invert the order to reduce nesting:

if err != nil { // log here continue } stored = append(stored, nextSchema)

andresuribe87 · 2023-05-31T16:32:41Z

pkg/service/credential/service.go

 	if request.SchemaID != "" {
 		// resolve schema and save it for validation later
 		gotSchema, err := s.schema.GetSchema(ctx, schema.GetSchemaRequest{ID: request.SchemaID})
 		if err != nil {
 			return nil, sdkutil.LoggingErrorMsgf(err, "failed to create credential; could not get schema: %s", request.SchemaID)
 		}
 		knownSchema = &gotSchema.Schema
-
+		schemaType := schemalib.CredentialSchema2023Type
+		if gotSchema.CredentialSchema == nil {


As mentioned in other places, I think we should completely remove.

gabe added 9 commits May 26, 2023 15:07

fix config and some storage stuff

3bca2f0

split out credential status

836eeb9

progress

1d9b88d

fix some tests; make manifest tests more readible

6150b91

fix tests

e0998b1

fix tests

830b00e

spec

e5e95da

remove signed schema params

251a7b1

remove signed schema params

b667691

decentralgabe marked this pull request as ready for review May 30, 2023 20:55

decentralgabe requested review from nitro-neal and andresuribe87 as code owners May 30, 2023 20:55

port

dfa57f6

decentralgabe commented May 30, 2023

View reviewed changes

pkg/service/credential/status.go

@@ -0,0 +1,97 @@

package credential

Copy link

Member Author

decentralgabe May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this to another file for readability

andresuribe87 reviewed May 31, 2023

View reviewed changes

decentralgabe mentioned this pull request May 31, 2023

Config should not expose non-configurable values #502

Closed

gabe added 2 commits May 31, 2023 10:04

pr comments

d78df12

spec

4b2e62f

andresuribe87 approved these changes May 31, 2023

View reviewed changes

decentralgabe merged commit e76fb15 into main May 31, 2023

decentralgabe deleted the update-schema-fpwd branch May 31, 2023 20:10

This was referenced Jun 1, 2023

Support credentialschema2023 #505

Merged

Update credential creation so the ID is a URL #500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uptake JSON Schema for FPWD #494

Uptake JSON Schema for FPWD #494

decentralgabe commented May 26, 2023 •

edited

Loading

decentralgabe May 30, 2023

decentralgabe May 30, 2023

andresuribe87 May 31, 2023

decentralgabe May 30, 2023

codecov-commenter commented May 30, 2023

andresuribe87 May 30, 2023

decentralgabe May 31, 2023

decentralgabe May 31, 2023

andresuribe87 May 30, 2023

decentralgabe May 31, 2023

andresuribe87 May 30, 2023

decentralgabe May 31, 2023

andresuribe87 May 30, 2023

decentralgabe May 31, 2023

andresuribe87 May 30, 2023

decentralgabe May 31, 2023

andresuribe87 May 31, 2023

decentralgabe May 31, 2023

andresuribe87 May 31, 2023

decentralgabe May 31, 2023

andresuribe87 May 31, 2023

decentralgabe May 31, 2023

andresuribe87 May 31, 2023

decentralgabe May 31, 2023

andresuribe87 May 31, 2023

		Name string `json:"name" validate:"required"`
		Description string `json:"description,omitempty"`

		Schema schemalib.JSONSchema `json:"schema" validate:"required"`

		// TODO(gabe): re-enable in https://github.com/TBD54566975/ssi-service/issues/493

Uptake JSON Schema for FPWD #494

Uptake JSON Schema for FPWD #494

Conversation

decentralgabe commented May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 30, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentralgabe commented May 26, 2023 •

edited

Loading