clarify that `logo_uri` can be of multiple schemes #141

Sakurann · 2023-12-09T00:43:29Z

resolves #43.

openid-4-verifiable-credential-issuance-1_0.md

paulbastian · 2023-12-14T11:06:55Z

As a sidetopic: I wonder why there is a logo for the credential, but not for the issuer?

peppelinux · 2023-12-14T18:19:09Z

As a sidetopic: I wonder why there is a logo for the credential, but not for the issuer?

Good catch @paulbastian.
Using Federation we have logo_uri in the federation_entity and any other protocol specific metadata, so for who uses openid federation this problem doesn't exist ...

openid-4-verifiable-credential-issuance-1_0.md

…_uri

Sakurann · 2023-12-18T13:20:29Z

I changed the structure to the array as suggested by @danielfett; added a reference to RFC2397 for the data: scheme format; and added a logo parameter for the credential issuer as requested by @paulbastian. please re-review

awoie

I don't think we should introduce media_type and I don't think we should encourage people to use data URIs. This would not be great in terms of performance. IMO, it is fine to have one logo URI, and potentially one URL for the thumbnail. The media type of the image is determined by the HTTP response content-type.

Sakurann · 2023-12-20T18:28:08Z

@awoie the current PR text does not introduce 'media_types'.

Some want to use data uris and I don't think it is up to us to prevent that.

Without an agreement in the industry on one image format, I don't see a better option than having an array, even if it might end up having one entry in the end.

awoie · 2023-12-21T14:41:19Z

@awoie the current PR text does not introduce 'media_types'.

Some want to use data uris and I don't think it is up to us to prevent that.

Without an agreement in the industry on one image format, I don't see a better option than having an array, even if it might end up having one entry in the end.

I want to avoid that a wallet has to fetch all logos from the array until the wallet finds a suitable media type. If it is really required to support multiple logos then I support @peppelinux suggestion and we have to include the media type in the metadata to avoid unnecessary fetching of logos of unsupported media types. However, I'm questioning if multiple logos is needed in the first place.

openid-4-verifiable-credential-issuance-1_0.md

Sakurann · 2023-12-21T16:43:19Z

for the purpose of this PR, let's stick to making it a string and renaming to uri to prevent breaking changes before ID-1. any other feature suggestions, please open the issue.

tplooker · 2023-12-21T23:19:35Z

for the purpose of this PR, let's stick to making it a string and renaming to uri to prevent breaking changes before ID-1. any other feature suggestions, please open the issue.

It doesn't feel like there is consensus on this so its going to be hard to prevent a breaking change. Leaving this feature open to all image formats and both remote and local resolution isn't a solution either, standards are about making decisions and this proposal at the moment is the opposite of a decision. I'm fine with merging the PR but only on the basis that post ID-1 we have agreement to revisit and make it stricter, otherwise we need to resolve it now.

awoie

I'm ok with the data URI since it is not a breaking change. Just explains that URL can contain also a data URI which is fine (although not my preference).

However, the language on the issuer logo has to be removed since this is unrelated. See my comment.

awoie · 2023-12-22T12:48:14Z

openid-4-verifiable-credential-issuance-1_0.md

+          * `logo`: OPTIONAL. A JSON object with information about the logo of the Credential Issuer with a following non-exhaustive list of parameters that MAY be included:
+            * `uri`: OPTIONAL. String value that contains a URI where the Wallet can obtain a logo of the Credential from the Credential Issuer. Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc.
+            * `alt_text`: OPTIONAL. String value of an alternative text of a logo image.


Please delete this. This is new and unrelated to #43. If a mechanism for the issuer logo is needed, then this should not be defined format-specific and should rather be a top-level credential issuer metadata parameter.

Suggested change

* `logo`: OPTIONAL. A JSON object with information about the logo of the Credential Issuer with a following non-exhaustive list of parameters that MAY be included:

* `uri`: OPTIONAL. String value that contains a URI where the Wallet can obtain a logo of the Credential from the Credential Issuer. Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc.

* `alt_text`: OPTIONAL. String value of an alternative text of a logo image.

aaaa good catch. I added this in the wrong place.

you are right. this was born out of Paul's comment on this PR, and should be in another PR. i removed this from this PR and moved to PR #170

Sakurann · 2024-01-04T13:00:52Z

I'm fine with merging the PR but only on the basis that post ID-1 we have agreement to revisit and make it stricter, otherwise we need to resolve it now.

sure. @tplooker please open an issue so that we can track it!

7 approvals. open for more than a week. no objections to merge during the last WG call. * adding credential issuer logo uri based on PR #141 * Apply editorial suggestions from code review * make uri required * typo Co-authored-by: Joseph Heenan <joseph@heenan.me.uk> * change logo uri for each credential to be required too * Apply suggestions from Mike's code review * Apply suggestions from Mike's code review Co-authored-by: Michael B. Jones <michael_b_jones@hotmail.com> --------- Co-authored-by: Kristina <=> Co-authored-by: Joseph Heenan <joseph@heenan.me.uk> Co-authored-by: Michael B. Jones <michael_b_jones@hotmail.com>

clarify logo_uri can be of multiple schemes

873fdfb

Sakurann requested review from jogu, tlodderstedt, pmhsfelix, peppelinux, tplooker and paulbastian December 9, 2023 00:43

peppelinux reviewed Dec 9, 2023

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

peppelinux reviewed Dec 15, 2023

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

Kristina added 3 commits December 18, 2023 05:06

Merge branch 'main' of https://github.com/openid/OpenID4VCI into logo…

41e52b1

…_uri

add logo image for the credential issuer and change uri to an array

64968a8

change param name to uris

b2f91cf

awoie requested changes Dec 20, 2023

View reviewed changes

Sakurann added this to the ID-1 milestone Dec 21, 2023

Sakurann commented Dec 21, 2023

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

Sakurann commented Dec 21, 2023

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

fall back to a string

2edb499

Sakurann requested review from awoie, peppelinux and danielfett December 21, 2023 16:43

awoie requested changes Dec 22, 2023

View reviewed changes

tlodderstedt approved these changes Dec 22, 2023

View reviewed changes

resolve merge conflict

85413bb

Kristina added 2 commits December 22, 2023 15:27

move credential issuer logo_uri to a correct place

81af7de

remove issuer logo from this PR to turn into another PR

87a5921

Sakurann pushed a commit that referenced this pull request Dec 22, 2023

adding credential issuer logo uri based on PR #141

97c624b

Sakurann mentioned this pull request Dec 22, 2023

adding credential issuer logo parameter in the issuer metadata #170

Merged

jogu approved these changes Dec 22, 2023

View reviewed changes

Sakurann requested a review from awoie December 22, 2023 23:32

Kristina and others added 2 commits December 22, 2023 15:34

add history entry

a94aa5e

Merge branch 'main' into logo_uri

10f16d3

awoie approved these changes Jan 4, 2024

View reviewed changes

Sakurann merged commit 5084a07 into main Jan 4, 2024
2 checks passed

peppelinux mentioned this pull request Jan 9, 2025

Security risks of using logo_uri in Trust Marks openid/federation#174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clarify that `logo_uri` can be of multiple schemes #141

clarify that `logo_uri` can be of multiple schemes #141

Sakurann commented Dec 9, 2023

paulbastian commented Dec 14, 2023

peppelinux commented Dec 14, 2023 •

edited

Loading

Sakurann commented Dec 18, 2023

awoie left a comment

Sakurann commented Dec 20, 2023

awoie commented Dec 21, 2023

Sakurann commented Dec 21, 2023

tplooker commented Dec 21, 2023 •

edited

Loading

awoie left a comment

awoie Dec 22, 2023 •

edited

Loading

Sakurann Dec 22, 2023

Sakurann Dec 22, 2023

Sakurann commented Jan 4, 2024 •

edited

Loading

	* `logo`: OPTIONAL. A JSON object with information about the logo of the Credential Issuer with a following non-exhaustive list of parameters that MAY be included:
	* `uri`: OPTIONAL. String value that contains a URI where the Wallet can obtain a logo of the Credential from the Credential Issuer. Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc.
	* `alt_text`: OPTIONAL. String value of an alternative text of a logo image.

clarify that logo_uri can be of multiple schemes #141

clarify that logo_uri can be of multiple schemes #141

Conversation

Sakurann commented Dec 9, 2023

paulbastian commented Dec 14, 2023

peppelinux commented Dec 14, 2023 • edited Loading

Sakurann commented Dec 18, 2023

awoie left a comment

Choose a reason for hiding this comment

Sakurann commented Dec 20, 2023

awoie commented Dec 21, 2023

Sakurann commented Dec 21, 2023

tplooker commented Dec 21, 2023 • edited Loading

awoie left a comment

Choose a reason for hiding this comment

awoie Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

Sakurann Dec 22, 2023

Choose a reason for hiding this comment

Sakurann Dec 22, 2023

Choose a reason for hiding this comment

Sakurann commented Jan 4, 2024 • edited Loading

clarify that `logo_uri` can be of multiple schemes #141

clarify that `logo_uri` can be of multiple schemes #141

peppelinux commented Dec 14, 2023 •

edited

Loading

tplooker commented Dec 21, 2023 •

edited

Loading

awoie Dec 22, 2023 •

edited

Loading

Sakurann commented Jan 4, 2024 •

edited

Loading