feat: list and ability to delete psa request created though deleted tokens #2054

flea89 · 2022-10-19T16:32:21Z

Resolve #1840

While I think a better approach would be to make the whole PSA user scoped, I know there's no consensus on that at the moment.
For that reason, I followed the approach we discussed for the user/list list endpoint. I've created a new DELETE user endpoint for this.

⚠️ Requires a new migration 028-update-list-keys.sql ⚠️

github-actions · 2022-10-19T16:39:56Z

`package-lock.json` changes

Click to toggle table visibility

Name	Previous	Current
`esbuild-android-64`	-	0.14.51
`esbuild-android-arm64`	-	0.13.14
`esbuild-freebsd-64`	-	0.13.14
`esbuild-freebsd-arm64`	-	0.13.14
`esbuild-linux-32`	-	0.13.14
`esbuild-linux-64`	-	0.13.14
`esbuild-linux-arm`	-	0.13.14
`esbuild-linux-arm64`	-	0.13.14
`esbuild-linux-mips64le`	-	0.13.14
`esbuild-linux-ppc64le`	-	0.13.14
`esbuild-linux-riscv64`	-	0.14.23
`esbuild-linux-s390x`	-	0.14.23
`esbuild-netbsd-64`	-	0.13.14
`esbuild-openbsd-64`	-	0.13.14
`esbuild-sunos-64`	-	0.13.14
`esbuild-windows-32`	-	0.13.14
`esbuild-windows-64`	-	0.13.14
`esbuild-windows-arm64`	-	0.13.14

github-actions · 2022-10-19T16:43:14Z

Website preview 🔗✨

build log

joshJarr

Looks good so far! Added a few minor typing/syntax comments.
maybe out of scope of this ticket, but I've noticed the user id being passed around as a Number and I think this should be a string.

joshJarr · 2022-10-20T14:33:34Z

packages/api/src/user.js

+ * @return {Promise<JSONResponse>}
+ */
+export async function userPinDelete (request, env, ctx) {
+  // @ts-ignore


Why are we ignoring this?
Should we add params to the AuthenticatedRequest type?

Good catch!

The issue here is that AuthenticatedRequest extends Request while it should really extend import('itty-router').Request that does define the param property.

The challenge here is that some of the property definitions do not quite match with Request.
If you look here, most of the properties are optional, which I don't think it is necessarely accurate. And updating that type would cause lots of type errors across the whole package.
From a quick look at the code in itty-router I think a better type would be extending Request (Fetch Api one) and adding params and query.

That said, I wonder if it's better to tackle this in its own ticket to gauge what the best approach here, and given it's a common pattern to use request.parameters (with @ts-ignore) across the package to leave it as in here?
Actually I'll update the code to check if params exists given I think it can be udefined.

Possible fixes:

updating the type to extend from ittyrouter.Request or

create our own Request one with the additional properties and have Authenticated Request to extend from it) .
3 something else

Cool, yep that’s a great shout to address the wider issue in a separate ticket.
And also a great shout to check if Paramus exist since we should consider it optional if not typed correctly 👍

packages/api/src/user.js

packages/db/index.js

packages/db/test/user.spec.js

alanshaw · 2022-11-04T18:04:38Z

packages/api/src/user.js

+  }
+
+  // TODO: Improve me, it is un-optimal to get the tokens in a separate request to the db.
+  const tokens = (await env.db.listKeys(request.auth.user._id, { includeDeleted: true })).map((key) => key._id)


Why not just add a new SQL function that deletes a pin request by user ID?

TL;DR: to keep PSA APIs as performant as possible.

Longer piece of rational

Getting the tokens first and then using them to get the pins is already an "accepted" pattern and used in userPinsGet.
Especially listPsaPinRequests has some fairly complex filter and sorting logic that would be quite complex to move to a SQL function (also arguably much harder to maintain or understand).

I reckon we could actually achieve this without resorting to a DB function. We could just change the supabase query pinRequestSelect to join with the user table and update the query to use user_id.

However, that would be another join, with the subsequent performance implication. A join that would be superfluous for "normal" pin list requests, given the PSA APIs are currently scoped by token.

With the assumption that when it comes to PSA pin requests, the most common scenario is we receive calls from the "exposed" APIs ( my assumption is that 90% of the call are likely from Kubo) rather than requests from our UI, I preferred prioritizing the former perf.

On the plus side, this solution does not require a migration and therefore a dependency on you.

While I know there are solutions that could maximize both, I did not consider the effort worth the shot especially in the context of leaving Postgres quite soon in the future.

I think that once we get to the agreement the APIs should be scoped by user and not token, if required we could consider running a migration to de-normalise the user_id on psa_pin_request table.

Let me know what you think

alanshaw · 2022-11-04T18:06:41Z

packages/api/src/user.js

@@ -514,8 +513,7 @@ export async function userPinsGet (request, env) {
    throw psaParams.error
  }

-  // @ts-ignore
-  const tokens = (await env.db.listKeys(request.auth.user._id)).map((key) => key._id)
+  const tokens = (await env.db.listKeys(request.auth.user._id, { includeDeleted: true })).map((key) => key._id)


Why not just add a SQL function that lists pin requests by user ID?

Co-authored-by: Alan Shaw <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [7.13.0](api-v7.12.0...api-v7.13.0) (2022-11-30) ### Features * list and ability to delete psa request created though deleted tokens ([#2054](#2054)) ([e150d1f](e150d1f)) ### Bug Fixes * allow update PSA pin requests with same CID ([#2125](#2125)) ([0013efd](0013efd)), closes [#1547](#1547) * PSA compliance fixes ([#2091](#2091)) ([8b1c1d3](8b1c1d3)), closes [#1579](#1579) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [2.33.0](website-v2.32.0...website-v2.33.0) (2022-12-01) ### Features * add tooltip to dashboard storage totals ([#2101](#2101)) ([2056ec6](2056ec6)), closes [#2081](#2081) * list and ability to delete psa request created though deleted tokens ([#2054](#2054)) ([e150d1f](e150d1f)) * simple analytics ([#2110](#2110)) ([4d5a1a0](4d5a1a0)) ### Bug Fixes * styles missing from custom storage form ([#2135](#2135)) ([04b66bc](04b66bc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

flea89 added 4 commits October 19, 2022 15:20

Start work on user scope pin deletion

8f2fd9c

update list key to optionally list deleted keys

5ea75cc

Update apis to use deleted kes

1e1dfa5

Add new api to routes and update fe to use it

7346261

flea89 changed the title ~~Feat/pin deletion user~~ fit: list and ability to delete psa request created though deleted tokens Oct 19, 2022

flea89 changed the title ~~fit: list and ability to delete psa request created though deleted tokens~~ feat: list and ability to delete psa request created though deleted tokens Oct 19, 2022

Revert changes to package.json

17abd51

flea89 temporarily deployed to production October 19, 2022 16:47 Inactive

flea89 added 3 commits October 19, 2022 22:48

Make linter happier

b9a461d

Fix tests

0896777

Update types

96b4e34

flea89 temporarily deployed to production October 19, 2022 21:59 Inactive

joshJarr reviewed Oct 20, 2022

View reviewed changes

update listKeys signature annotation to be string

1f6e6b0

flea89 temporarily deployed to production October 21, 2022 10:14 Inactive

params can be undefined

708d330

flea89 requested a review from joshJarr October 21, 2022 10:41

flea89 marked this pull request as ready for review October 21, 2022 10:41

flea89 temporarily deployed to production October 21, 2022 10:43 Inactive

joshJarr approved these changes Oct 21, 2022

View reviewed changes

flea89 requested review from it-dag-house and olizilla and removed request for it-dag-house October 25, 2022 10:47

flea89 mentioned this pull request Oct 25, 2022

Unable to remove pinned item #1981

Open

Drop function in migration

fc730b0

alanshaw requested changes Nov 4, 2022

View reviewed changes

Update packages/db/test/user.spec.js

c1c699a

Co-authored-by: Alan Shaw <[email protected]>

alanshaw approved these changes Nov 29, 2022

View reviewed changes

Merge branch 'main' into feat/pin-deletion-user

413e567

flea89 requested a review from a team as a code owner November 30, 2022 12:24

flea89 merged commit e150d1f into main Nov 30, 2022

flea89 deleted the feat/pin-deletion-user branch November 30, 2022 12:31

This was referenced Nov 30, 2022

chore(main): release website 2.33.0 #2136

Merged

chore(main): release api 7.13.0 #2137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: list and ability to delete psa request created though deleted tokens #2054

feat: list and ability to delete psa request created though deleted tokens #2054

flea89 commented Oct 19, 2022 •

edited

Loading

github-actions bot commented Oct 19, 2022

github-actions bot commented Oct 19, 2022 •

edited

Loading

joshJarr left a comment

joshJarr Oct 20, 2022

flea89 Oct 21, 2022

joshJarr Oct 21, 2022

alanshaw Nov 4, 2022

flea89 Nov 7, 2022

alanshaw Nov 4, 2022

feat: list and ability to delete psa request created though deleted tokens #2054

feat: list and ability to delete psa request created though deleted tokens #2054

Conversation

flea89 commented Oct 19, 2022 • edited Loading

github-actions bot commented Oct 19, 2022

package-lock.json changes

github-actions bot commented Oct 19, 2022 • edited Loading

Website preview 🔗✨

joshJarr left a comment

Choose a reason for hiding this comment

joshJarr Oct 20, 2022

Choose a reason for hiding this comment

flea89 Oct 21, 2022

Choose a reason for hiding this comment

joshJarr Oct 21, 2022

Choose a reason for hiding this comment

alanshaw Nov 4, 2022

Choose a reason for hiding this comment

flea89 Nov 7, 2022

Choose a reason for hiding this comment

Longer piece of rational

alanshaw Nov 4, 2022

Choose a reason for hiding this comment

flea89 commented Oct 19, 2022 •

edited

Loading

`package-lock.json` changes

github-actions bot commented Oct 19, 2022 •

edited

Loading