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: refactor pinning authorization logic to use user_tag table #1389

Merged
merged 3 commits into from
Mar 16, 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
3 changes: 2 additions & 1 deletion packages/api/db/reset.sql
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ DROP TYPE IF EXISTS pin_status_type cascade;
DROP TYPE IF EXISTS service_type cascade;
DROP TYPE IF EXISTS auth_key_blocked_status_type cascade;
DROP TYPE IF EXISTS user_tag_type cascade;
DROP TYPE IF EXISTS user_tag_value_type cascade;

DROP TABLE IF EXISTS upload CASCADE;
DROP TABLE IF EXISTS pin;
Expand All @@ -15,4 +16,4 @@ DROP TABLE IF EXISTS public.user CASCADE;
DROP TABLE IF EXISTS cargo.aggregate_entries;
DROP TABLE IF EXISTS cargo.aggregates;
DROP TABLE IF EXISTS cargo.deals;
DROP SERVER IF EXISTS dag_cargo_server CASCADE;
DROP SERVER IF EXISTS dag_cargo_server CASCADE;
7 changes: 0 additions & 7 deletions packages/api/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,6 @@ export const database = {

export const isDebug = DEBUG === 'true'

/**
* The list of user IDs that are allowed to use the Pinning Service API. By
* default ["*"] - meaning anyone can use it.
*/
export const psaAllow =
typeof PSA_ALLOW !== 'undefined' ? PSA_ALLOW.split(',') : ['*']

export const s3 = {
endpoint: typeof S3_ENDPOINT !== 'undefined' ? S3_ENDPOINT : '',
region: typeof S3_REGION !== 'undefined' ? S3_REGION : '',
Expand Down
22 changes: 20 additions & 2 deletions packages/api/src/middleware/psa.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { maybeCapture, ErrorPinningUnauthorized } from '../errors.js'
import { JSONResponse } from '../utils/json-response.js'
import { validate } from '../utils/auth.js'
import { psaAllow } from '../constants.js'

/** @typedef {import('../bindings').Handler} Handler */

Expand All @@ -28,6 +27,21 @@ export function withPsaErrorHandler(handler) {
}
}
}
/**
* Return true if a user has a tag with a given name and value.
*
* @param {import('../utils/db-client-types.js').UserOutput} user
* @param {string} tagName
* @param {string} value
* @returns {boolean}
*/
function hasTag(user, tagName, value) {
return Boolean(
user.tags?.find(
(tag) => tag.tag === tagName && tag.value === value && !tag.deleted_at
)
)
}

/**
* Verify that the authenticated request is for a user who is authorized to use
Expand All @@ -40,7 +54,11 @@ export function withPinningAuthorized(handler) {
return async (event, ctx) => {
// TODO: we need withAuth middleware so we don't have to do this twice
const { user } = await validate(event, ctx)
const authorized = psaAllow.includes(String(user.id)) || psaAllow[0] === '*'

const authorized =
hasTag(user, 'HasPsaAccess', 'true') &&
!hasTag(user, 'HasAccountRestriction', 'true')

if (!authorized) {
throw new ErrorPinningUnauthorized()
}
Expand Down
6 changes: 6 additions & 0 deletions packages/api/src/utils/db-client-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,14 @@ export type UserOutputKey = Pick<
'user_id' | 'id' | 'name' | 'secret'
>

export type UserOutputTag = Pick<
definitions['user_tag'],
'user_id' | 'id' | 'tag' | 'value' | 'deleted_at'
>

export type UserOutput = definitions['user'] & {
keys: Array<UserOutputKey>
tags: Array<UserOutputTag>
}

export type UploadOutput = definitions['upload'] & {
Expand Down
28 changes: 28 additions & 0 deletions packages/api/src/utils/db-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export class DBClient {
github_id,
did,
keys:auth_key_user_id_fkey(user_id,id,name,secret)
tags:user_tag_user_id_fkey(user_id,id,tag,value)
`
)
.or(`magic_link_id.eq.${id},github_id.eq.${id},did.eq.${id}`)
Expand Down Expand Up @@ -401,6 +402,33 @@ export class DBClient {
return data
}

/**
* Create a new user tag
*
* @param {Object} tag
* @param {number} tag.user_id
* @param {string} tag.value
* @param {string} tag.value_type
* @param {string} tag.inserted_at
* @param {string} tag.reason
*/
async createUserTag(tag) {
/** @type {PostgrestQueryBuilder<definitions['user_tag']>} */
const query = this.client.from('user_tag')

const { data, error } = await query.upsert(tag).single()

if (error) {
throw new DBError(error)
}

if (!data) {
throw new Error('User tag not created.')
}

return data
}

/**
* List auth keys
*
Expand Down
2 changes: 2 additions & 0 deletions packages/api/src/utils/db-types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1347,6 +1347,8 @@ export interface definitions {
/** Format: text */
value: string
/** Format: text */
user_tag_value_type: string
/** Format: text */
reason: string
/**
* Format: timestamp with time zone
Expand Down
41 changes: 41 additions & 0 deletions packages/api/test/scripts/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,32 @@ export async function createTestUser({
return createTestUserWithFixedToken({ token, publicAddress, issuer, name })
}

/**
* Create a new user tag
*
* @param {Object} tag
* @param {number} tag.user_id
* @param {string} tag.tag
* @param {string} tag.value
* @param {string} tag.inserted_at
* @param {string} tag.reason
*/
async function createUserTag(tag) {
const query = rawClient.from('user_tag')

const { data, error } = await query.upsert(tag).single()

if (error) {
throw error;
}

if (!data) {
throw new Error('User tag not created.')
}

return data
}

/**
* @param {{publicAddress?: string, issuer?: string, name?: string, token?: string}} userInfo
*/
Expand Down Expand Up @@ -62,6 +88,21 @@ export async function createTestUserWithFixedToken({
userId: user.id,
})

await createUserTag({
user_id: user.id,
tag: 'HasPsaAccess',
value: 'true',
reason: '',
inserted_at: '2/22/2022',
})

await createUserTag({
user_id: user.id,
tag: 'HasAccountRestriction',
value: 'false',
reason: '',
inserted_at: '2/22/2022',
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering: Do these two writes happen in a transaction? If the first succeds and the second throws, is that a valid db state?

return { token, userId: user.id, githubId: user.github_id }
}

Expand Down