Add support for issuer custom/alternative origin #2356
Conversation
This adds frontend support and end-to-end testing for custom issuer derivation origins. In particular: * Updates the VC flow to support looking up & validating the derivation origin (see spec for details) * Fixes a couple of bugs in VC issuer canister provisioning * Adds support for provisioning the VC issuer canister with derivation origin used in the end-to-end tests * Remove frontend checks in the issuer canister (caused issues in the end-to-end tests because non-configurable (beyond provisioning) and not actually used) * Add a nice origin for the issuer canister in the vite config * Update all VC tests to use the `derivation_origin` for issuer (somewhat fiddly/brittle due to having a single issuer)
There was a problem hiding this comment.
DId you reply the last time I asked? I didn't see the response or maybe I didn't send the question... Which is the command to generate this file?
There was a problem hiding this comment.
I missed it! It's run with npm run generate:types-issuer. I'm actually surprised it isn't run on CI, probably something you should look into.
| @@ -22,23 +23,41 @@ test("Can issue credential with alternative RP derivation origin", async () => { | |||
|
|
|||
| const authConfig = await register["webauthn"](browser); | |||
|
|
|||
| // 1. Add employee | |||
| // XXX: The issuer always specifies a derivation origin, which we need | |||
There was a problem hiding this comment.
It means "warning, fiddly stuff ahead": https://stackoverflow.com/a/1452945
| @@ -302,6 +318,46 @@ const getAliasCredentials = async ({ | |||
| return { ok: { rpAliasCredential, issuerAliasCredential } }; | |||
| }; | |||
|
|
|||
| // Looks up and verify the derivation origin to be used by the issuer | |||
| const getValidatedIssuerDerivationOrigin = async ({ | |||
There was a problem hiding this comment.
Do I understand correctly that there are no unit tests because everything is tested with e2e tests?
There was a problem hiding this comment.
No, there's no particular reason. How would you test this with a unit test?
| @@ -109,3 +134,60 @@ test("Cannot issue credential with bad alternative RP derivation origin", async | |||
| expect(result.reason).toBe("bad_derivation_origin_rp"); | |||
| }); | |||
| }, 300_000); | |||
|
|
|||
| test("Can issue credential with alternative issuer derivation origin", async () => { | |||
There was a problem hiding this comment.
Should we add a test where the getting the derivation origin fails?
There was a problem hiding this comment.
Yes! See list of TODOs I shared offline
| @@ -371,23 +371,6 @@ fn should_return_derivation_origin_with_custom_init() { | |||
| assert_eq!(response.origin, custom_init.derivation_origin); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Please annotate with #[ignore] rather than deleting.
There was a problem hiding this comment.
Can you explain why that's better? If the test needs to be resurrected it can always be found in the git history
There was a problem hiding this comment.
It is more convenient to re-enable and is nagging in the test output to do something about it.
There was a problem hiding this comment.
is nagging in the test output to do something about it.
That's unfortunate! I think the cleanest way would be to track this somewhere, otherwise (by ignoring the test) it might never get done though it will always be a slight annoyance when running the tests. And in case we never re-add the test, having it ignored is just wasted real estate in the file! Nothing as permanent as temporary solutions.
src/frontend/src/test-e2e/verifiableCredentials/alternativeOrigins.test.ts
Outdated
Show resolved
Hide resolved
This adds frontend support and end-to-end testing for custom issuer derivation origins. In particular:
derivation_originfor issuer (somewhat fiddly/brittle due to having a single issuer)🟡 Some screens were changed