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: list and ability to delete psa request created though deleted tokens #2054

Merged
merged 13 commits into from Nov 30, 2022
1,205 changes: 1,204 additions & 1 deletion package-lock.json

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion packages/api/src/index.js
Expand Up @@ -21,7 +21,8 @@ import {
userUploadsDelete,
userUploadGet,
userUploadsGet,
userUploadsRename
userUploadsRename,
userPinDelete
} from './user.js'
import { pinDelete, pinGet, pinPost, pinsGet } from './pins.js'
import { blogSubscriptionCreate } from './blog.js'
Expand Down Expand Up @@ -109,6 +110,7 @@ router.delete('/user/tokens/:id', auth['👤🗑️'](userTokensDelete))
router.get('/user/account', auth['👤'](userAccountGet))
router.get('/user/info', auth['👤'](userInfoGet))
router.get('/user/pins', auth['📌⚠️'](userPinsGet))
router.delete('/user/pins/:requestId', auth['👤🗑️'](userPinDelete))
router.get('/user/payment', auth['👤'](userPaymentGet))
router.put('/user/payment', auth['👤'](userPaymentPut))

Expand Down
4 changes: 2 additions & 2 deletions packages/api/src/pins.js
Expand Up @@ -226,7 +226,7 @@ export async function pinDelete (request, env, ctx) {

try {
// Update deleted_at (and updated_at) timestamp for the pin request.
await env.db.deletePsaPinRequest(requestId, authToken._id)
await env.db.deletePsaPinRequest(requestId, [authToken._id])
} catch (e) {
console.error(e)
throw new PSAErrorResourceNotFound()
Expand Down Expand Up @@ -265,7 +265,7 @@ async function replacePin (normalizedCid, newPinData, requestId, authTokenId, en
}

try {
await env.db.deletePsaPinRequest(requestId, authTokenId)
await env.db.deletePsaPinRequest(requestId, [authTokenId])
} catch (e) {
console.error(e)
throw new PSAErrorDB()
Expand Down
44 changes: 39 additions & 5 deletions packages/api/src/user.js
@@ -1,7 +1,7 @@
import * as JWT from './utils/jwt.js'
import { JSONResponse, notFound } from './utils/json-response.js'
import { JWT_ISSUER } from './constants.js'
import { HTTPError, RangeNotSatisfiableError } from './errors.js'
import { HTTPError, PSAErrorInvalidData, PSAErrorRequiredData, PSAErrorResourceNotFound, RangeNotSatisfiableError } from './errors.js'
import { getTagValue, hasPendingTagProposal, hasTag } from './utils/tags.js'
import {
NO_READ_OR_WRITE,
Expand All @@ -11,7 +11,7 @@ import {
} from './maintenance.js'
import { pagination } from './utils/pagination.js'
import { toPinStatusResponse } from './pins.js'
import { validateSearchParams } from './utils/psa.js'
import { INVALID_REQUEST_ID, REQUIRED_REQUEST_ID, validateSearchParams } from './utils/psa.js'
import { magicLinkBypassForE2ETestingInTestmode } from './magic.link.js'
import { CustomerNotFound, getPaymentSettings, initializeBillingForNewUser, isStoragePriceName, savePaymentSettings } from './utils/billing.js'

Expand Down Expand Up @@ -315,7 +315,6 @@ export async function userRequestPost (request, env) {
* @param {import('./env').Env} env
*/
export async function userTokensGet (request, env) {
// @ts-ignore
const tokens = await env.db.listKeys(request.auth.user._id)

return new JSONResponse(tokens)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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


let pinRequests

Expand Down Expand Up @@ -569,6 +567,42 @@ export async function userPinsGet (request, env) {
}, { headers })
}

/**
* List a user's pins regardless of the token used.
* As we don't want to scope the Pinning Service API to users
* we need a new endpoint as an umbrella.
*
* @param {import('./user').AuthenticatedRequest} request
* @param {import('./env').Env} env
* @param {import('./index').Ctx} ctx
* @return {Promise<JSONResponse>}
*/
export async function userPinDelete (request, env, ctx) {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. updating the type to extend from ittyrouter.Request or
  2. create our own Request one with the additional properties and have Authenticated Request to extend from it) .
    3 something else

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

const requestId = request.params?.requestId

if (!requestId) {
throw new PSAErrorRequiredData(REQUIRED_REQUEST_ID)
}

if (typeof requestId !== 'string') {
throw new PSAErrorInvalidData(INVALID_REQUEST_ID)
}

// 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)
joshJarr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


try {
// Update deleted_at (and updated_at) timestamp for the pin request.
await env.db.deletePsaPinRequest(requestId, tokens)
} catch (e) {
console.error(e)
throw new PSAErrorResourceNotFound()
}

return new JSONResponse({}, { status: 202 })
}

/**
* @param {string} userProposalForm
* @param {string} tagName
Expand Down
4 changes: 2 additions & 2 deletions packages/api/test/fixtures/init-data.sql
Expand Up @@ -70,8 +70,8 @@ VALUES (6, 'test-pinning-and-restriction', 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ
(7, 'test-restriction', 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ0ZXN0LXJlc3RyaWN0aW9uIiwiaXNzIjoid2ViMy1zdG9yYWdlIiwiaWF0IjoxNjMzOTU3Mzg5ODcyLCJuYW1lIjoidGVzdC1yZXN0cmljdGlvbiJ9.uAklG7dHOxRD85c564RBcBqeFGUGNBper7VLaXBGnFg', 10000000000000007);

-- Used to test pinning from an existing user with a different token
INSERT INTO auth_key (id, name, secret, user_id)
VALUES (8, 'test-pinning-b', 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ0ZXN0LXBpbm5pbmctYiIsImlzcyI6IndlYjMtc3RvcmFnZSIsImlhdCI6MTYzMzk1NzM4OTg3MiwibmFtZSI6InRlc3QtcGlubmluZy1iIn0.B2qeE5mHW93saJx-Nu8p8Io_z7_3VRaHoBAVxPbCP5c', 10000000000000004);
INSERT INTO auth_key (id, name, secret, user_id, deleted_at)
VALUES (8, 'test-pinning-b', 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ0ZXN0LXBpbm5pbmctYiIsImlzcyI6IndlYjMtc3RvcmFnZSIsImlhdCI6MTYzMzk1NzM4OTg3MiwibmFtZSI6InRlc3QtcGlubmluZy1iIn0.B2qeE5mHW93saJx-Nu8p8Io_z7_3VRaHoBAVxPbCP5c', 10000000000000004, '2021-07-14T19:27:14.934572+00:00');

-- Reset the primary key next value
SELECT pg_catalog.setval(pg_get_serial_sequence('auth_key', 'id'), MAX(id)) FROM auth_key;
Expand Down
2 changes: 2 additions & 0 deletions packages/api/test/user.spec.js
Expand Up @@ -417,6 +417,8 @@ describe('GET /user/pins', () => {

assert(res.ok)
const body = await res.json()

// one of the auth tokens is deleted, but the request should still appear in the list.
assert.equal([...new Set(body.results.map(x => x.pin.authKey))].length, 2)
})
})
Expand Down
4 changes: 4 additions & 0 deletions packages/db/db-client-types.ts
Expand Up @@ -353,3 +353,7 @@ export type LogEmailSentInput = {
emailType: string,
messageId: string
}

export type ListKeysOptions = {
includeDeleted: boolean
}
7 changes: 4 additions & 3 deletions packages/db/index.d.ts
Expand Up @@ -31,6 +31,7 @@ import type {
EmailSentInput,
LogEmailSentInput,
GetUserOptions,
ListKeysOptions,
} from './db-client-types'

export class DBClient {
Expand Down Expand Up @@ -60,12 +61,12 @@ export class DBClient {
getDealsForCids (cids: string[]): Promise<Record<string, Deal[]>>
createKey (key: CreateAuthKeyInput): Promise<CreateAuthKeyOutput>
getKey (issuer: string, secret: string): Promise<AuthKey>
listKeys (userId: number): Promise<Array<AuthKeyItemOutput>>
listKeys (userId: string, opts?: ListKeysOptions): Promise<Array<AuthKeyItemOutput>>
createPsaPinRequest (pinRequest: PsaPinRequestUpsertInput): Promise<PsaPinRequestUpsertOutput>
getPsaPinRequest (authKey: string, pinRequestId: string) : Promise<PsaPinRequestUpsertOutput>
listPsaPinRequests (authKey: string, opts?: ListPsaPinRequestOptions ) : Promise<ListPsaPinRequestResults>
deletePsaPinRequest (pinRequestId: string, authKey: string) : Promise<PsaPinRequestItem>
deleteKey (id: number): Promise<void>
deletePsaPinRequest (pinRequestId: string, authKey: Array<string>) : Promise<PsaPinRequestItem>
deleteKey (userId: number, keyId: number): Promise<void>
query<T, V>(document: RequestDocument, variables: V): Promise<T>
createUserTag(userId: string, tag: UserTagInput): Promise<boolean>
getUserTags(userId: string): Promise<UserTagInfo[]>
Expand Down
15 changes: 9 additions & 6 deletions packages/db/index.js
Expand Up @@ -1004,13 +1004,15 @@ export class DBClient {
/**
* List auth keys of a given user.
*
* @param {number} userId
* @param {string} userId
* @param {import('./db-client-types').ListKeysOptions} opts
* @return {Promise<Array<import('./db-client-types').AuthKeyItemOutput>>}
*/
async listKeys (userId) {
async listKeys (userId, { includeDeleted } = { includeDeleted: false }) {
joshJarr marked this conversation as resolved.
Show resolved Hide resolved
/** @type {{ error: PostgrestError, data: Array<import('./db-client-types').AuthKeyItem> }} */
const { data, error } = await this._client.rpc('user_auth_keys_list', {
query_user_id: userId
query_user_id: userId,
include_deleted: includeDeleted
})

if (error) {
Expand Down Expand Up @@ -1244,9 +1246,9 @@ export class DBClient {
* Delete a user PA pin request.
*
* @param {number} requestId
* @param {string} authKey
* @param {string[]} authKeys
*/
async deletePsaPinRequest (requestId, authKey) {
async deletePsaPinRequest (requestId, authKeys) {
const date = new Date().toISOString()
/** @type {{ data: import('./db-client-types').PsaPinRequestItem, error: PostgrestError }} */
const { data, error } = await this._client
Expand All @@ -1255,7 +1257,8 @@ export class DBClient {
deleted_at: date,
updated_at: date
})
.match({ auth_key_id: authKey, id: requestId })
.match({ id: requestId })
.in('auth_key_id', authKeys)
.filter('deleted_at', 'is', null)
.single()

Expand Down
5 changes: 3 additions & 2 deletions packages/db/postgres/functions.sql
Expand Up @@ -356,7 +356,7 @@ BEGIN
END
$$;

CREATE OR REPLACE FUNCTION user_auth_keys_list(query_user_id BIGINT)
CREATE OR REPLACE FUNCTION user_auth_keys_list(query_user_id BIGINT, include_deleted BOOLEAN default false)
RETURNS TABLE
(
"id" text,
Expand All @@ -374,7 +374,8 @@ SELECT (ak.id)::TEXT AS id,
ak.inserted_at AS created,
EXISTS(SELECT 42 FROM upload u WHERE u.auth_key_id = ak.id) AS has_uploads
FROM auth_key ak
WHERE ak.user_id = query_user_id AND ak.deleted_at IS NULL
WHERE ak.user_id = query_user_id AND
(include_deleted OR ak.deleted_at IS NULL)
$$;

CREATE OR REPLACE FUNCTION find_deals_by_content_cids(cids text[])
Expand Down
23 changes: 23 additions & 0 deletions packages/db/postgres/migrations/028-update-list-keys.sql
@@ -0,0 +1,23 @@
DROP FUNCTION IF EXISTS user_auth_keys_list;

CREATE OR REPLACE FUNCTION user_auth_keys_list(query_user_id BIGINT, include_deleted BOOLEAN default false)
RETURNS TABLE
(
"id" text,
"name" text,
"secret" text,
"created" timestamptz,
"has_uploads" boolean
)
LANGUAGE sql
AS
$$
SELECT (ak.id)::TEXT AS id,
ak.name AS name,
ak.secret AS secret,
ak.inserted_at AS created,
EXISTS(SELECT 42 FROM upload u WHERE u.auth_key_id = ak.id) AS has_uploads
FROM auth_key ak
WHERE ak.user_id = query_user_id AND
(include_deleted OR ak.deleted_at IS NULL)
$$;