Initial GET and DELETE Operations #271

mprorock · 2022-03-13T18:11:46Z

The bare minimum get and delete operations for credentials and presentations - putting this in for feedback and once merged we can extend the pattern as required. in particular - i think we will want allow a PUT to update and / or replace items by ID

NOTE: Depends on pr #270 and should not be merged until that is resolved

msporny · 2022-03-13T18:17:02Z

NOTE: Depends on pr #270 and should not be merged until that is resolved

Marking 'do not merge' temporarily so no one accidentally merges this before #270.

mprorock · 2022-03-13T18:24:44Z

@msporny i am inclined to set "id" for getters to be UUID or a URI - does this make sense to you?

msporny · 2022-03-13T18:54:16Z

@msporny i am inclined to set "id" for getters to be UUID or a URI - does this make sense to you?

I'm fine to start there. I expect that we'll quickly move towards something that looks more like 'transaction-id' as the next step, and then expand further after that. Every time we try to pick UUID, it turns into "UUID and multi-base 128 bit value", which then turns into "but I want to encode JWT-like objects in the ID, so give me 1024 URL-safe characters to play around with".

I get why we'd want URIs for GET... shudder a bit at having to URIEncode those. No strong feelings, but again, feels like "id"s are just going to turn into "1024 URL-safe characters" over time. Just my $0.02 for now.

mprorock · 2022-03-13T19:03:31Z

just going to turn into "1024 URL-safe characters" over time. Just my $0.02 for now.

yeah - i suspect you are right for top level API and then i also expect profiles to tighten that down to their specific requirements - e.g. i would bet the traceability goes with just UUID for a variety of reasons

peacekeeper · 2022-03-15T20:39:39Z

I think this is useful, but GET and DELETE operations should probably be optional. There may be implementations that can issue VCs but then don't remember them.

msporny · 2022-03-20T18:01:37Z

@mprorock I've merged the other PR that this was based on to main. This needs to be refactored on top of main. I suggest cherry picking the four commits instead of rebasing -- I cleaned up the history to remove the add/removal of package-lock.json as well as the auto-generated files in the commit history.

OR13 · 2022-03-21T13:37:02Z

There are conflicts

mprorock · 2022-03-29T13:48:44Z

Conflicts should now be resolved

msporny · 2022-03-29T13:56:54Z

Conflicts should now be resolved

Still showing conflits with rebase and merge. I can squash-merge if you'd like, but would prefer to keep your history.

OR13 · 2022-03-29T13:57:56Z

as of this comment, this is what I see.

OR13 · 2022-03-29T13:58:31Z

The other code owners have not reviewed.

msporny · 2022-03-29T13:58:44Z

holder.yml

+          name: id
+          schema:
+            type: string
+            format: uuid


Hrm, disagree w/ this only being able to be a UUID -- but we can deal w/ that in another PR. Just noting my objection to this one design choice.

@mprorock we can take this change in our profile and leave this repo with type string where <script> alert(1);</script> is a spec legal identifier.

leave this repo with type string where <script> alert(1);</script> is a spec legal identifier.

Or, you know, do something more sane :P and re-use our existing transactionId/exchangeId regexes, which don't allow the attack you mention above:

https://github.com/w3c-ccg/vc-api/blob/main/components/parameters/path/TransactionId.yml

So I agree that the legal identifier character set should be limited to keep it simple.

However, I think this idea that we are going to play whack-a-mole with data that is allowed in fields based on how it might be abused in other contexts (both known and unknown!) is misguided. That's what encoding / escaping is for. If you're not doing that appropriately in the relevant contexts, then relying on certain fields to not allow certain values is not going to save you.

msporny · 2022-03-29T13:59:44Z

holder.yml

+        "418":
+          description: I'm a teapot - MUST not be returned outside of pre-arranged scenarios between both parties


Har har :) -- presuming we're going to eventually remove this... because if we don't, we'll have to test for it.

msporny · 2022-03-29T14:00:48Z

holder.yml

+            format: uuid
+      responses:
+        "202":
+          description: Credential deleted - this is a 202 by default as soft deletes and processing time are assumed


This should probably say "Credential marked for deletion". Looks like the proposal here is that this call is sometimes implemented as eventually consistent. This also need to state what the side effects are... like, if associated with a status list, what happens to the status bits?

msporny

Ok to merge as a work in progress. I've noted a few things that are concerning with the PR, but good enough for now.

msporny · 2022-03-29T14:04:00Z

This is what I see:

OR13 · 2022-03-29T14:08:03Z

holder.yml

+          name: id
+          schema:
+            type: string
+            format: uuid


Suggested change

format: uuid

Lets make sure to allow xss / command injection in our identifiers.

No, let's not do that -- we have a more sane path forward that the group already has consensus on: https://github.com/w3c-ccg/vc-api/pull/271/files#r837533740

+1 manu - added an ObjectId that matches that transaction id

OR13 · 2022-03-29T14:10:35Z

holder.yml

+        "400":
+          description: Bad Request
+        "401":
+          description: Not Authorized


are we going to define authorization? and test it?

are we going to define authorization? and test it?

The test suites will need to have some authz mechanism in them because the production software that all implementers are implementing are all (AFAIK) requiring authz. If we have interop on the test suites, the spec will have to reflect reality and document how authz is being performed in an interoperable fashion.

OR13 · 2022-03-29T14:11:15Z

holder.yml

+              type: string
+              oneOf:
+                - "credentials"
+                - "verifiablecredentials"


Suggested change

- "verifiablecredentials"

- "verifiable-credentials"

Why kebab-case instead of camelCase? We're using camelCase everywhere else, no?

its a query string param, and its pluralized... so i figured it would be better to not have it look like verifiableCredential which shows up in VPs, but I am not attached to anything other than fixing the lowercase concatenation issue.

Thanks for the explanation, I didn't see that it was a query param, I agree with your view wrt. casing and query params.

OR13 · 2022-03-29T14:15:21Z

This is what I see:

ahh thats because you are using rebase... you should stop using rebase since it makes it harder for non technical git users to contribute ; )

msporny

Hrm, just spotted the two massive auto-generated files. We have Github Actions to auto-generate files, we should not commit autogenerated files.

OR13

remove auto generated files

mprorock · 2022-03-29T14:35:13Z

Hrm, just spotted the two massive auto-generated files. We have Github Actions to auto-generate files, we should not commit autogenerated files.

that should be fixed now - thanks for catching that

msporny · 2022-03-29T14:50:46Z

ahh thats because you are using rebase... you should stop using rebase since it makes it harder for non technical git users to contribute ; )

When there are non-technical Github people involved, I use squash.

This is not one of those cases :), @mprorock knows what he's doing. I consider it bad form to squash merge contributor's commits if it looks like they've gone to the trouble of creating a clean history. In those cases, I ask them if they're ok with it before doing it.

msporny · 2022-03-29T15:01:27Z

holder.yml

+            type: array
+            items:
+              type: string
+              oneOf:


These options seem a bit arbitrary... fine for now, but we need to define exactly what should be happening when each option is provided... for example, is this really checking the type field, or is it doing something else -- my guess is the latter... while the former might be more useful?

msporny

LGTM. @mprorock, do I have your permission to squash-merge this PR? Or, would you like to rebase so I can rebase-merge? Note: If you rebase, please just make it so that the auto-generated files don't show up in history at all... I had to go in and surgically remove auto-gen'd files from history the last time.

mprorock · 2022-03-29T16:03:34Z

LGTM. @mprorock, do I have your permission to squash-merge this PR? Or, would you like to rebase so I can rebase-merge? Note: If you rebase, please just make it so that the auto-generated files don't show up in history at all... I had to go in and surgically remove auto-gen'd files from history the last time.

personally I am a squash and merge fan @msporny - so good to go at your discretion
edit: in this case, in general I like merge commits but they are not enabled on this repo

mprorock added 11 commits March 13, 2022 11:25

feat: add initial bundling of api

c68cb2b

feat: add stubs for common openapi ux

2a53cae

chore: stub in api viewers

01db86f

chore: update build script

594fc5c

chore: add initial tags

50b20ea

chore: update bundling and outputs

4298e47

chore: specify dev dependencies

cfe28f3

feat: add gh workflow to build and deploy docs

4adeeb1

doc: update readme for npm build

19c3bff

chore: add initial holder getters for credentials and presentations

d0e5a31

chore: add delete for creds and presos

1041f1e

mprorock requested review from peacekeeper, msporny, mavarley, OR13 and mkhraisha as code owners March 13, 2022 18:11

mprorock added 2 commits March 13, 2022 14:14

chore: remove any auto-gen-files

ca74e07

chore: update gitignore for auto-gen files

9df6b57

msporny added the do not merge label Mar 13, 2022

chore: merge from auto-gen removed files

baf487c

OR13 approved these changes Mar 14, 2022

View reviewed changes

mkhraisha approved these changes Mar 15, 2022

View reviewed changes

This was referenced Mar 20, 2022

Initial PR for exchange discovery. #268

Merged

confusion around endpoint naming #97

Closed

mprorock added 2 commits March 29, 2022 09:43

chore: updates prior to merge

e6990c8

chore: merge upstream main and clean up tags

f871d1f

msporny reviewed Mar 29, 2022

View reviewed changes

msporny approved these changes Mar 29, 2022

View reviewed changes

msporny removed the do not merge label Mar 29, 2022

OR13 reviewed Mar 29, 2022

View reviewed changes

msporny requested changes Mar 29, 2022

View reviewed changes

OR13 requested changes Mar 29, 2022

View reviewed changes

mprorock added 2 commits March 29, 2022 10:31

chore: update to use ref to object id rather than uuid

6dc46c9

chore: remove autogen files

bc57899

OR13 approved these changes Mar 29, 2022

View reviewed changes

mprorock requested a review from msporny March 29, 2022 14:40

msporny reviewed Mar 29, 2022

View reviewed changes

msporny approved these changes Mar 29, 2022

View reviewed changes

msporny merged commit 88a2217 into w3c-ccg:main Mar 29, 2022

mprorock mentioned this pull request Mar 29, 2022

Get credentials issued by query #114

Closed

		"418":
		description: I'm a teapot - MUST not be returned outside of pre-arranged scenarios between both parties

Initial GET and DELETE Operations #271

Initial GET and DELETE Operations #271

Conversation

mprorock commented Mar 13, 2022

msporny commented Mar 13, 2022

mprorock commented Mar 13, 2022

msporny commented Mar 13, 2022 • edited Loading

mprorock commented Mar 13, 2022

peacekeeper commented Mar 15, 2022

msporny commented Mar 20, 2022 • edited Loading

OR13 commented Mar 21, 2022

mprorock commented Mar 29, 2022

msporny commented Mar 29, 2022

OR13 commented Mar 29, 2022

OR13 commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msporny Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msporny Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msporny left a comment

Choose a reason for hiding this comment

msporny commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msporny Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

OR13 commented Mar 29, 2022

msporny left a comment

Choose a reason for hiding this comment

OR13 left a comment

Choose a reason for hiding this comment

mprorock commented Mar 29, 2022

msporny commented Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

msporny left a comment • edited Loading

Choose a reason for hiding this comment

mprorock commented Mar 29, 2022 • edited Loading

msporny commented Mar 13, 2022 •

edited

Loading

msporny commented Mar 20, 2022 •

edited

Loading

msporny Mar 29, 2022 •

edited

Loading

msporny Mar 29, 2022 •

edited

Loading

msporny Mar 29, 2022 •

edited

Loading

msporny commented Mar 29, 2022 •

edited

Loading

msporny left a comment •

edited

Loading

mprorock commented Mar 29, 2022 •

edited

Loading