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: PSA compliance fixes #2091

Merged
merged 6 commits into from
Nov 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/api/src/pins.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export async function pinsGet (request, env, ctx) {
/**
* Transform a PinRequest into a PinStatus
*
* @param { Object } pinRequest
* @param { import('../../db/db-client-types.js').PsaPinRequestUpsertOutput } pinRequest
* @returns { PsaPinStatusResponse }
*/
export function toPinStatusResponse (pinRequest) {
Expand All @@ -198,9 +198,10 @@ export function toPinStatusResponse (pinRequest) {
created: pinRequest.created,
pin: {
cid: pinRequest.sourceCid,
...pinRequest
...pinRequest.name && { name: pinRequest.name },
...pinRequest.origins && { origins: pinRequest.origins },
...pinRequest.meta && { meta: pinRequest.meta }
},
// TODO(https://github.com/web3-storage/web3.storage/issues/792)
delegates: []
}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/api/src/utils/psa.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export const MAX_PIN_LISTING_LIMIT = 1000
// Validation schemas
const listPinsValidator = new Validator({
type: 'object',
required: ['status'],
required: [],
properties: {
name: { type: 'string', maxLength: 255 },
after: { type: 'string', format: 'date-time' },
Expand Down Expand Up @@ -235,7 +235,7 @@ export function validateSearchParams (queryString) {

if (result.valid) {
// Map statuses for DB compatibility.
opts.statuses = psaStatusesToDBStatuses(opts.status)
opts.statuses = opts.status && psaStatusesToDBStatuses(opts.status)
data = opts
} else {
error = parseValidatorErrors(result.errors)
Expand Down
71 changes: 43 additions & 28 deletions packages/api/test/pin.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import {
} from '../src/utils/psa.js'
import { PSAErrorResourceNotFound, PSAErrorInvalidData, PSAErrorRequiredData } from '../src/errors.js'

const REQUEST_EXPECTED_PROPERTIES = ['requestid', 'status', 'created', 'delegates', 'info', 'pin']
const PIN_EXPECTED_PROPERTIES = ['cid', 'name', 'origins', 'meta']

/**
*
* @param {import('../../db/postgres/pg-rest-api-types').definitions['pin']['status']} status
Expand Down Expand Up @@ -42,24 +45,41 @@ const assertCorrectPinResponse = (data) => {
assert.ok(Date.parse(data.created), 'created should be valid date string')
assert.ok(Array.isArray(data.delegates), 'delegates should be an array')

if (data.info) {
// @ts-ignore https://github.com/microsoft/TypeScript/issues/44253
if (Object.hasOwn(data, 'info')) {
assert.ok(typeof data.info === 'object', 'info should be an object')
}

assert.ok(typeof data.pin === 'object', 'pin should be an object')
assert.ok(typeof data.pin.cid === 'string', 'pin.cid should be an string')

if (data.pin.name) {
// @ts-ignore https://github.com/microsoft/TypeScript/issues/44253
if (Object.hasOwn(data.pin, 'name')) {
assert.ok(typeof data.pin.name === 'string', 'pin.name should be an string')
}

if (data.pin.origins) {
// @ts-ignore https://github.com/microsoft/TypeScript/issues/44253
if (Object.hasOwn(data.pin, 'origins')) {
assert.ok(Array.isArray(data.pin.origins), 'pin.origins should be an array')
}

if (data.pin.meta) {
// @ts-ignore https://github.com/microsoft/TypeScript/issues/44253
if (Object.hasOwn(data.pin, 'meta')) {
assert.ok(typeof data.pin.meta === 'object', 'pin.meta should be an object')
}

assert.ok(Object.keys(data).every(property => REQUEST_EXPECTED_PROPERTIES.includes(property)), 'request contains not valid properties')
assert.ok(Object.keys(data.pin).every(property => PIN_EXPECTED_PROPERTIES.includes(property)), 'request.pin contains not valid properties')
}

/**
*
* @param {object} data
*/
const assertCorretPinResponseList = (data) => {
assert.ok(typeof data.count === 'number', 'count should be a number')
data.results.forEach(r => assertCorrectPinResponse(r))
Object.keys(data).every(property => ['count', 'results'].includes(property))
}

/**
Expand Down Expand Up @@ -122,24 +142,6 @@ describe('Pinning APIs endpoints', () => {
assert.strictEqual(error.details, '#/limit: Instance type "number" is invalid. Expected "integer".')
})

it('requires status', async () => {
const url = new URL(`${baseUrl}`).toString()
const res = await fetch(
url, {
method: 'GET',
headers: {
Authorization: `Bearer ${token}`,
'Content-Type': 'application/json'
}
})

assert(res, 'Server responded')
assert.strictEqual(res.status, ERROR_CODE)
const { error } = await res.json()
assert.strictEqual(error.reason, PSAErrorRequiredData.CODE)
assert.strictEqual(error.details, 'Instance does not have required property "status".')
})

it('validates meta filter is json object', async () => {
const opts = new URLSearchParams({
meta: `[
Expand Down Expand Up @@ -252,9 +254,7 @@ describe('Pinning APIs endpoints', () => {
})

it('returns only successful pins when no filter values are specified', async () => {
const opts = new URLSearchParams({
status: 'pinned'
})
const opts = new URLSearchParams({})
const url = new URL(`${baseUrl}?${opts}`).toString()
const res = await fetch(
url, {
Expand All @@ -269,6 +269,7 @@ describe('Pinning APIs endpoints', () => {
assert(res.ok, 'Server response is ok')
const data = await res.json()
assert.strictEqual(data.count, 1)
assertCorretPinResponseList(data)
})

it('filters pins on CID, for this user', async () => {
Expand All @@ -295,6 +296,7 @@ describe('Pinning APIs endpoints', () => {
assert(res.ok, 'Server response is ok')
const data = await res.json()
assert.strictEqual(data.count, 2)
assertCorretPinResponseList(data)
})

it('filters case sensitive exact match on name', async () => {
Expand All @@ -316,6 +318,7 @@ describe('Pinning APIs endpoints', () => {
assert(res.ok, 'Server response is ok')
const data = await res.json()
assert.strictEqual(data.count, 1)
assertCorretPinResponseList(data)
})

it('filters case insensitive partial match on name', async () => {
Expand All @@ -338,6 +341,7 @@ describe('Pinning APIs endpoints', () => {
assert(res.ok, 'Server response is ok')
const data = await res.json()
assert.strictEqual(data.count, 4)
assertCorretPinResponseList(data)
})

it('filters pins by status', async () => {
Expand All @@ -360,6 +364,7 @@ describe('Pinning APIs endpoints', () => {
assert.strictEqual(data.count, 1)
assert.strictEqual(data.results.length, 1)
assert.strictEqual(data.results[0].pin.name, 'FailedPinning.doc')
assertCorretPinResponseList(data)
})

it('filters pins by multiple statuses', async () => {
Expand All @@ -380,6 +385,7 @@ describe('Pinning APIs endpoints', () => {
assert(res.ok, 'Server response is ok')
const data = await res.json()
assert.strictEqual(data.count, 5)
assertCorretPinResponseList(data)
})

it('filters pins by queued', async () => {
Expand All @@ -400,6 +406,7 @@ describe('Pinning APIs endpoints', () => {
assert(res.ok, 'Server response is ok')
const data = await res.json()
assert.strictEqual(data.count, 1)
assertCorretPinResponseList(data)
})

it('filters pins created before a date', async () => {
Expand All @@ -422,6 +429,7 @@ describe('Pinning APIs endpoints', () => {
const data = await res.json()
assert.strictEqual(data.results.length, 1)
assert.strictEqual(data.count, 1)
assertCorretPinResponseList(data)
})

it('filters pins created after a date', async () => {
Expand All @@ -444,6 +452,7 @@ describe('Pinning APIs endpoints', () => {
const data = await res.json()
assert.strictEqual(data.results.length, 2)
assert.strictEqual(data.count, 2)
assertCorretPinResponseList(data)
})

it('limits the number of pins returned for this user and includes the total', async () => {
Expand All @@ -466,6 +475,7 @@ describe('Pinning APIs endpoints', () => {
const data = await res.json()
assert.strictEqual(data.count, 7)
assert.strictEqual(data.results.length, 3)
assertCorretPinResponseList(data)
})

it('filters pins by meta', async () => {
Expand All @@ -488,6 +498,7 @@ describe('Pinning APIs endpoints', () => {
const data = await res.json()
assert.strictEqual(data.count, 1)
assert.strictEqual(data.results[0].pin.name, 'Image.jpeg')
assertCorretPinResponseList(data)
})
})

Expand All @@ -510,10 +521,12 @@ describe('Pinning APIs endpoints', () => {
assert(res, 'Server responded')
assert(res.ok, 'Server response ok')
const data = await res.json()
assertCorrectPinResponse(data)
assert.strictEqual(data.pin.cid, cid)
assert.strictEqual(data.pin.name, null)
assert.strictEqual(data.pin.origins, null)
assert.strictEqual(data.pin.meta, null)
console.log(data.pin.cid, cid)
assert.strictEqual(data.pin.name, undefined)
assert.strictEqual(data.pin.origins, undefined)
assert.strictEqual(data.pin.meta, undefined)
})

it('requires cid', async () => {
Expand Down Expand Up @@ -575,6 +588,7 @@ describe('Pinning APIs endpoints', () => {
const data = await res.json()
assert(res, 'Server responded')
assert(res.ok, 'Server response ok')
assertCorrectPinResponse(data)
assert.strictEqual(data.pin.cid, cid)
assert.deepStrictEqual(data.pin.name, name)
assert.deepStrictEqual(data.pin.origins, origins)
Expand Down Expand Up @@ -972,6 +986,7 @@ describe('Pinning APIs endpoints', () => {
assert(replaceResponse.ok, 'Replace request was not successful')
assert.strictEqual(replaceResponse.status, 202)
const data = await replaceResponse.json()
assertCorrectPinResponse(data)
assert.strictEqual(data.pin.cid, newCid)
assert.deepStrictEqual(data.pin.name, name)
assert.deepStrictEqual(data.pin.origins, origins)
Expand Down
2 changes: 1 addition & 1 deletion packages/api/test/user.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ describe('GET /user/pins', () => {

assert(res.ok)
const body = await res.json()
assert.equal([...new Set(body.results.map(x => x.pin.authKey))].length, 2)
assert.equal(body.count, 8)
})
})

Expand Down
17 changes: 13 additions & 4 deletions packages/db/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,10 @@ export class DBClient {
async listPsaPinRequests (authKey, opts = {}) {
const match = opts?.match || 'exact'
const limit = opts?.limit || 10
/**
* @type {Array.<string>|undefined}
*/
let statuses

let query = this._client
.from(psaPinRequestTableName)
Expand All @@ -1180,12 +1184,17 @@ export class DBClient {
query = query.range(rangeFrom, rangeTo - 1)
}

if (!opts.cid && !opts.name && !opts.statuses) {
query = query.eq('content.pins.status', 'Pinned')
// If not specified we default to pinned only if no other filters are provided.
// While slightly inconsistent, that's the current expectation.
// This is being discussed in https://github.com/ipfs-shipyard/pinning-service-compliance/issues/245
if (!opts.cid && !opts.name && !opts.meta && !opts.statuses) {
statuses = ['Pinned']
} else {
statuses = opts.statuses
}

if (opts.statuses) {
query = query.in('content.pins.status', opts.statuses)
if (statuses) {
query = query.in('content.pins.status', statuses)
}

if (opts.cid) {
Expand Down