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

fix: bump ew-credentials to 2.2.1-alpha.296.0 #662

Merged
merged 1 commit into from Sep 22, 2022
Merged

Conversation

whitneypurdum
Copy link
Member

@whitneypurdum whitneypurdum commented Sep 21, 2022

Description

  • Bumps ew-credentials version. If a user has a request to approve a role that she does not have the correct approval role for, the error will specify the role that she does not have.
  • Old Error: No authorative credential found for issuer
  • New Error: No authorative credential found for issuer for role "installer.role..."

@jrhender and @JGiter It sounded from the story that you wanted to add more info to this error. Should ICL catch the error from verifyIssuer, and if the error contains this text return it with more descriptive error message such as 'No authorative credential found for issuer for role "installer.role...". You must obtain and publish this role before approving this request.'
Or should we just throw this new error?

Update - we will just return this error and in the future address the root issue that SSI hub returns role approval requests that a user is not authorized to approve due to not having/publishing role

Contributor checklist

  • Breaking changes - check for any existing interfaces changes that are not backward compatible, removed method etc.
  • Documentation - document your code, add comments for method, remember to check if auto generated docs were updated.
  • Tests - add new or updated existed test for changes you made.
  • Migration guide - add migration guide for every breaking change.
  • Configuration correctness - check that any configuration changes are correct ex. default URLs, chain ids, smart contract verification on Volta explorer or EWC explorer.

@JGiter
Copy link
Collaborator

JGiter commented Sep 22, 2022

Should ICL catch the error from verifyIssuer, and if the error contains this text return it with more descriptive error message?
Yes, I would check if issuer has this role, but hasn't published it, then suggest to publish

@whitneypurdum
Copy link
Member Author

Should ICL catch the error from verifyIssuer, and if the error contains this text return it with more descriptive error message?
Yes, I would check if issuer has this role, but hasn't published it, then suggest to publish

@JGiter currently ew-credentials error pattern is to return 'boolean, string' to ICL. If we want to implement the above, we will need to do some string parsing to 1.) determine that this error is for not having authoritative credential and 2.) then pull out the actual role name. and then 3.) pass the role to check if the user has it. Unless there is a different way, or we want to change how we are handling errors. I feel we could achieve almost the same result by checking if the error is 'no authoritative credential found' and then adding "If you do not have this role, you must obtain it. If you already have this role, please publish it." But, happy to either update how we handle the errors (If we want to get the role and check for it, I suggest we change the ew-credential error to an object) or do regex parsing if we don't want to go this route but still want to check if the user has this role. Let me know if I'm missing something here. Thanks.

@JGiter
Copy link
Collaborator

JGiter commented Sep 22, 2022

currently ew-credentials error pattern is to return 'boolean, string'

Do you mean error thrown during issuance? I think that the purpose of this task is to exlude such errors. So that when issuer receives request to issue, then issuance happens without any issues

@JGiter
Copy link
Collaborator

JGiter commented Sep 22, 2022

My last comment doesn't correspond to actual task description which say about error handling, but nevertheless I think we should be move toward avoiding error

@JGiter
Copy link
Collaborator

JGiter commented Sep 22, 2022

But yes, if error happend you can append it with message saying that issuer role needs to be published and rethrow

@whitneypurdum
Copy link
Member Author

But yes, if error happend you can append it with message saying that issuer role needs to be published and rethrow

At this point we don't know if it needs to be published, or if he has not enrolled in the role at all. I think we should throw an error saying "No authorative credential found for issuer for role "installer.role...". If you do not have this role, you must obtain it. If you do have this role, please publish it."

Either way on SB, it will appear in the red error box. There is no other UI treatment right now for caught errors. That would be an addition to this task I think.

Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

Seems good to me

@whitneypurdum whitneypurdum merged commit 9af689c into develop Sep 22, 2022
ewf-devops pushed a commit that referenced this pull request Sep 22, 2022
## [7.0.0-alpha.3](v7.0.0-alpha.2...v7.0.0-alpha.3) (2022-09-22)

### Bug Fixes

* bump ew-credentials to 2.2.1-alpha.296.0 ([#662](#662)) ([9af689c](9af689c))
@ewf-devops
Copy link

🎉 This PR is included in version 7.0.0-alpha.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jrhender jrhender deleted the #SWTCH-1455 branch September 23, 2022 06:03
ewf-devops pushed a commit that referenced this pull request Dec 6, 2022
## [7.0.0](v6.2.0...v7.0.0) (2022-12-06)

### ⚠ BREAKING CHANGES

* **claims:** Payload of selfsigned claim is arbitrary object

### Bug Fixes

* bump ew-credentials to 2.2.1-alpha.296.0 ([#662](#662)) ([9af689c](9af689c))
* **claims:** change selfsigned claim to record ([9986876](9986876))
* commit for 2c0a860 ([45cdac0](45cdac0))
* publish assets on-chain ([#654](#654)) ([2c0a860](2c0a860))
* verify credential before issuer verification ([111923b](111923b))

### Refactoring

* **claims:** merge ClaimData with vc-verification ([0e1fafa](0e1fafa))

### Documentation

* add asset credential registration to documentation ([ba6f7ba](ba6f7ba))

### Style changes

* add warn for shadow variables (variables in scope that share s… ([#663](#663)) ([dd28b49](dd28b49))
@ewf-devops
Copy link

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants