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

Add models for Credential Issuer Metadata #304

Merged
merged 9 commits into from
Mar 15, 2023

Conversation

andresuribe87
Copy link
Contributor

This is based in the spec section https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#name-credential-issuer-metadata

I believe the SDK is the best home for this. My expectation is that other folks will need to support this kind of models, and providing a library to do so easily will be beneficial to the community.

I thought about separating it into it's own module, but given that we're packaging all functionality into the SDK, I believe this is the best place for this to live.

Copy link
Contributor

@nitro-neal nitro-neal left a comment

Choose a reason for hiding this comment

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

👍

const (
JWKFormat CryptographicBindingMethodSupported = "jwk"
COSEKey = "cose_key"
AllDIDs = "did"
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to make this all_dids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't. This is part of what's specified in the spec. You made me realize that this approach was sub-par. So I added more documentation, and a new method to get all the Binding DID Methods from the CredentialSupported struct

oidc/issuance/metadata_test.go Show resolved Hide resolved
)

type Logo struct {
Url *util.URL `json:"url,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Url *util.URL `json:"url,omitempty"`
URL *util.URL `json:"url,omitempty"`


const (
JwtVcJson Format = "jwt_vc_json"
JwtVcJsonLd = "jwt_vc_json-ld"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
JwtVcJsonLd = "jwt_vc_json-ld"
JWTVCJSONLD = "jwt_vc_json-ld"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const (
JwtVcJson Format = "jwt_vc_json"
JwtVcJsonLd = "jwt_vc_json-ld"
LdpVc = "ldp_vc"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LdpVc = "ldp_vc"
LDPVC = "ldp_vc"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reads so bad :(

Done

Copy link
Member

Choose a reason for hiding this comment

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

yeah it sucks

type CredentialSupported struct {
Format Format `json:"format" validate:"required"`

Id *string `json:"id,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Id *string `json:"id,omitempty"`
ID *string `json:"id,omitempty"`

is a string ptr needed over a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

ID is an optional field, so wanted to make that clear.

Comment on lines 137 to 140
err := json.Unmarshal(data, &cJSON)
if err != nil {
return errors.Wrap(err, "unmarshalling")
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err := json.Unmarshal(data, &cJSON)
if err != nil {
return errors.Wrap(err, "unmarshalling")
}
if err := json.Unmarshal(data, &cJSON); err != nil {
return errors.Wrap(err, "unmarshalling")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

url.URL
}

func (u URL) MarshalJSON() ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you should be able to just call this Marshal and it will implement the json marshal interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding your suggestion correctly, I don't believe it works. See below:

For the following test code

func TestMarshalURL(t *testing.T) {
	u, _ := url.Parse("https://localhost")
	data, err := json.Marshal(u)
	require.NoError(t, err)
	assert.Equal(t, `"https://localhost"`, string(data))
}

The result is

=== RUN   TestMarshalURL
    metadata_test.go:63: 
        	Error Trace:	/Users/auribe/code/ssi-sdk/oidc/issuance/metadata_test.go:63
        	Error:      	Not equal: 
        	            	expected: "\"https://localhost\""
        	            	actual  : "{\"Scheme\":\"https\",\"Opaque\":\"\",\"User\":null,\"Host\":\"localhost\",\"Path\":\"\",\"RawPath\":\"\",\"OmitHost\":false,\"ForceQuery\":false,\"RawQuery\":\"\",\"Fragment\":\"\",\"RawFragment\":\"\"}"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-"https://localhost"
        	            	+{"Scheme":"https","Opaque":"","User":null,"Host":"localhost","Path":"","RawPath":"","OmitHost":false,"ForceQuery":false,"RawQuery":"","Fragment":"","RawFragment":""}
        	Test:       	TestMarshalURL
--- FAIL: TestMarshalURL (0.00s)

Copy link
Member

Choose a reason for hiding this comment

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

looks like you had it right my bad http://choly.ca/post/go-json-marshalling/

util/url.go Outdated
Comment on lines 21 to 24
err := json.Unmarshal(data, &dataStr)
if err != nil {
return errors.Wrap(err, "unmarshalling")
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err := json.Unmarshal(data, &dataStr)
if err != nil {
return errors.Wrap(err, "unmarshalling")
}
if err := json.Unmarshal(data, &dataStr); err != nil {
return errors.Wrap(err, "unmarshalling")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@codecov-commenter
Copy link

Codecov Report

Merging #304 (36e6c34) into main (e010720) will increase coverage by 0.39%.
The diff coverage is 79.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
+ Coverage   60.54%   60.93%   +0.39%     
==========================================
  Files          41       42       +1     
  Lines        4554     4651      +97     
==========================================
+ Hits         2757     2834      +77     
- Misses       1352     1367      +15     
- Partials      445      450       +5     
Impacted Files Coverage Δ
oidc/issuance/metadata.go 79.38% <79.38%> (ø)

@andresuribe87 andresuribe87 merged commit e0ac99a into TBD54566975:main Mar 15, 2023
decentralgabe pushed a commit that referenced this pull request Mar 19, 2023
* Add simple URL for parsing strings.

* Add credential issuer metadata for oidc.

* Make the linter happy

* PR feedback

* Finish comment

* More PR comments

* Even More PR comments

* Enforce unique CredentialsSupported.ID

ion models

long form did and initial request
decentralgabe added a commit that referenced this pull request Mar 20, 2023
* Add models for Credential Issuer Metadata (#304)

* Add simple URL for parsing strings.

* Add credential issuer metadata for oidc.

* Make the linter happy

* PR feedback

* Finish comment

* More PR comments

* Even More PR comments

* Enforce unique CredentialsSupported.ID

ion models

long form did and initial request

* Added style and best practices (#298)

* Added style and best practices

* nits

deactivate request

update request

recover

lint

test not passing

fix test

fix reveal value; update test

temp

* update to jwx v2

* jwx 2

* bug fix

* works

* lint

* update err messages

* consistent spelling

* pr errs

---------

Co-authored-by: Andres Uribe <auribe@tbd.email>
decentralgabe added a commit that referenced this pull request Mar 21, 2023
* origin/main:
  Bump github.com/multiformats/go-multibase from 0.1.1 to 0.2.0 (#313)
  Bump github.com/go-playground/validator/v10 from 10.11.2 to 10.12.0 (#311)
  Bump github.com/goccy/go-json from 0.10.0 to 0.10.2 (#310)
  Bump golang.org/x/term from 0.5.0 to 0.6.0 (#299)
  Update JWX lib to use v2 (#308)
  Added style and best practices (#298)
  Add models for Credential Issuer Metadata (#304)
  Upgrade go version to 1.20.2 (#305)
  add missing param (#297)
  interface to any (#296)

# Conflicts:
#	cryptosuite/cryptosuite.go
#	cryptosuite/jsonwebkey2020.go
#	cryptosuite/jwssignaturesuite.go
#	cryptosuite/jwssignaturesuite_test.go
#	go.mod
#	go.sum
#	util/helpers.go
#	wasm/static/main.wasm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants