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

feat(Certificate): show more info to check issuer identity #353

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dariomas
Copy link
Collaborator

Show and in order to help the user to carefully check the Issuer identity.

@dariomas dariomas added the enhancement New feature or request label Feb 18, 2020
@codecov-io
Copy link

Codecov Report

Merging #353 into master will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
+ Coverage   71.39%   71.39%   +<.01%     
==========================================
  Files         127      127              
  Lines        1891     1895       +4     
  Branches      337      338       +1     
==========================================
+ Hits         1350     1353       +3     
- Misses        411      412       +1     
  Partials      130      130
Impacted Files Coverage Δ
...nts/atoms/CertificateDetails/CertificateDetails.js 100% <ø> (ø) ⬆️
.../CertificateDetails/CertificateDetailsContainer.js 100% <ø> (ø) ⬆️
src/selectors/certificate.js 82.6% <75%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c60517...9bbd648. Read the comment docs.

@raiseandfall
Copy link
Member

Hi @dariomas
Thanks for the suggestion! Can you provide unit tests for that feature change?

@lemoustachiste
Copy link
Member

Sorry I forgot to get back to you yesterday, I think it'd be nice also that URLs become links (with target="_blank"), so that people can easily make use of the information.

@dariomas
Copy link
Collaborator Author

Thank you @lemoustachiste and @raiseandfall
I will work on that soon.

@antonellopasella-kedos
Copy link

Could be useful to add the badge.issuer.image field in the header's verifier?

@@ -23,6 +24,7 @@ export default {
issueDate: 'Data',
issuerName: 'Organizzazione Issuer',
issuerPublicKey: 'Chiave pubblica del Issuer',
issuerURL: 'Issuer profile URL',

Choose a reason for hiding this comment

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

URL del profilo del Issuer

@lemoustachiste
Copy link
Member

Hm that's old, I'm not too fresh on that anymore and things have evolved with v3 cert (plus DID support).
Could you sum up again what you were trying to do? Show the URL of the issuer profile, correct?
I've had that in mind for a small while now. Since with DIDs we would still want to trust the issuer's profile origin.

@antonellopasella-kedos
Copy link

@lemoustachiste The issuer image is retrived from the issuer profile json file (external) and the bagde.issuer.image is ignored.

Can we show the badge.issuer.image in the verifier component instead of retrive from the external url?

@lemoustachiste
Copy link
Member

The issuer image is retrived from the issuer profile json file (external) and the bagde.issuer.image is ignored.

hm this is sort of by design as the issuer's identity (or brand) can evolve through time, so as to keep things accurate.
What is your use case, to see if we could find another solution (ie component API flag or something).

Or maybe if there is no image for the distant issuer profile read the local one as a fallback, would that work for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants