Skip to content

Commit

Permalink
feat: handle authentication on top level routes (#542)
Browse files Browse the repository at this point in the history
  • Loading branch information
fenos authored Sep 6, 2024
1 parent e416b7a commit 040871f
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 13 deletions.
17 changes: 17 additions & 0 deletions src/http/plugins/jwt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,15 @@ import { ERRORS } from '@internal/errors'

declare module 'fastify' {
interface FastifyRequest {
isAuthenticated: boolean
jwt: string
jwtPayload?: JwtPayload & { role?: string }
owner?: string
}

interface FastifyContextConfig {
allowInvalidJwt?: boolean
}
}

const BEARER = /^Bearer\s+/i
Expand All @@ -22,13 +27,25 @@ export const jwt = fastifyPlugin(async (fastify) => {
fastify.addHook('preHandler', async (request, reply) => {

Check warning on line 27 in src/http/plugins/jwt.ts

View workflow job for this annotation

GitHub Actions / Test / OS ubuntu-20.04 / Node 20

'reply' is defined but never used
request.jwt = (request.headers.authorization || '').replace(BEARER, '')

if (!request.jwt && request.routeConfig.allowInvalidJwt) {
request.jwtPayload = { role: 'anon' }
request.isAuthenticated = false
return
}

const { secret, jwks } = await getJwtSecret(request.tenantId)

try {
const payload = await verifyJWT(request.jwt, secret, jwks || null)
request.jwtPayload = payload
request.owner = payload.sub
request.isAuthenticated = true
} catch (err: any) {

Check warning on line 43 in src/http/plugins/jwt.ts

View workflow job for this annotation

GitHub Actions / Test / OS ubuntu-20.04 / Node 20

Unexpected any. Specify a different type
request.isAuthenticated = false

if (request.routeConfig.allowInvalidJwt) {
return
}
throw ERRORS.AccessDenied(err.message, err)
}
})
Expand Down
34 changes: 30 additions & 4 deletions src/http/routes/object/getObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { IncomingMessage, Server, ServerResponse } from 'http'
import { getConfig } from '../../../config'
import { AuthenticatedRangeRequest } from '../../types'
import { ROUTE_OPERATIONS } from '../operations'
import { ERRORS } from '@internal/errors'
import { Obj } from '@storage/schemas'

const { storageS3Bucket } = getConfig()

Expand Down Expand Up @@ -42,10 +44,34 @@ async function requestHandler(
const { download } = request.query
const objectName = request.params['*']

const obj = await request.storage.from(bucketName).findObject(objectName, 'id, version')

// send the object from s3
const s3Key = `${request.tenantId}/${bucketName}/${objectName}`
const bucket = await request.storage.asSuperUser().findBucket(bucketName, 'id,public', {
dontErrorOnEmpty: true,
})

// The request is not authenticated
if (!request.isAuthenticated) {
// The bucket must be public to access its content
if (!bucket?.public) {
throw ERRORS.AccessDenied('Access denied to this bucket')
}
}

// The request is authenticated
if (!bucket) {
throw ERRORS.NoSuchBucket(bucketName)
}

let obj: Obj | undefined

if (bucket.public) {
// request is authenticated but we still use the superUser as we don't need to check RLS
obj = await request.storage.asSuperUser().from(bucketName).findObject(objectName, 'id, version')
} else {
// request is authenticated use RLS
obj = await request.storage.from(bucketName).findObject(objectName, 'id, version')
}

return request.storage.renderer('asset').render(request, response, {
bucket: storageS3Bucket,
Expand Down Expand Up @@ -85,14 +111,14 @@ export default async function routes(fastify: FastifyInstance) {
// @todo add success response schema here
schema: {
params: getObjectParamsSchema,
headers: { $ref: 'authSchema#' },
summary: 'Get object',
description: 'use GET /object/authenticated/{bucketName} instead',
description: 'Serve objects',
response: { '4xx': { $ref: 'errorSchema#' } },
tags: ['deprecated'],
},
config: {
operation: { type: ROUTE_OPERATIONS.GET_AUTH_OBJECT },
allowInvalidJwt: true,
},
},
async (request, response) => {
Expand Down
27 changes: 21 additions & 6 deletions src/http/routes/object/getObjectInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getConfig } from '../../../config'
import { AuthenticatedRangeRequest } from '../../types'
import { Obj } from '@storage/schemas'
import { ROUTE_OPERATIONS } from '../operations'
import { ERRORS } from '@internal/errors'

const { storageS3Bucket } = getConfig()

Expand Down Expand Up @@ -38,11 +39,25 @@ async function requestHandler(

const s3Key = `${request.tenantId}/${bucketName}/${objectName}`

const bucket = await request.storage.asSuperUser().findBucket(bucketName, 'id,public', {
dontErrorOnEmpty: true,
})

// Not Authenticated flow
if (!request.isAuthenticated) {
if (!bucket?.public) {
throw ERRORS.AccessDenied('Access denied to this bucket')
}
}

// Authenticated flow
if (!bucket) {
throw ERRORS.NoSuchBucket(bucketName)
}

let obj: Obj
if (publicRoute) {
await request.storage.asSuperUser().findBucket(bucketName, 'id', {
isPublic: true,
})

if (bucket.public || publicRoute) {
obj = await request.storage
.asSuperUser()
.from(bucketName)
Expand Down Expand Up @@ -147,14 +162,14 @@ export async function authenticatedRoutes(fastify: FastifyInstance) {
{
schema: {
params: getObjectParamsSchema,
headers: { $ref: 'authSchema#' },
summary,
description: 'use HEAD /object/authenticated/{bucketName} instead',
response: { '4xx': { $ref: 'errorSchema#' } },
tags: ['deprecated'],
},
config: {
operation: { type: 'object.get_authenticated_info' },
allowInvalidJwt: true,
},
},
async (request, response) => {
Expand All @@ -167,14 +182,14 @@ export async function authenticatedRoutes(fastify: FastifyInstance) {
{
schema: {
params: getObjectParamsSchema,
headers: { $ref: 'authSchema#' },
summary,
description: 'use HEAD /object/authenticated/{bucketName} instead',
response: { '4xx': { $ref: 'errorSchema#' } },
tags: ['deprecated'],
},
config: {
operation: { type: 'object.head_authenticated_info' },
allowInvalidJwt: true,
},
},
async (request, response) => {
Expand Down
2 changes: 1 addition & 1 deletion src/storage/database/knex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
SearchObjectOption,
} from './adapter'
import { DatabaseError } from 'pg'
import { DBMigration, getTenantConfig, TenantConnection } from '@internal/database'
import { DBMigration, TenantConnection } from '@internal/database'
import { DbQueryPerformance } from '@internal/monitoring/metrics'
import { isUuid } from '../limits'

Expand Down
5 changes: 4 additions & 1 deletion src/storage/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getFileSizeLimit, mustBeValidBucketName, parseFileSizeToBytes } from '.
import { getConfig } from '../config'
import { ObjectStorage } from './object'
import { InfoRenderer } from '@storage/renderer/info'
import { logger, logSchema } from '@internal/monitoring'

const { requestUrlLengthLimit, storageS3Bucket } = getConfig()

Expand Down Expand Up @@ -206,7 +207,9 @@ export class Storage {
return all
}, [] as string[])
// delete files from s3 asynchronously
this.backend.deleteObjects(storageS3Bucket, params)
this.backend.deleteObjects(storageS3Bucket, params).catch((e) => {
logSchema.error(logger, 'Failed to delete objects from s3', { type: 's3', error: e })
})
}

if (deleted?.length !== objects.length) {
Expand Down
75 changes: 74 additions & 1 deletion src/test/object.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,20 @@ describe('testing GET object', () => {
expect(S3Backend.prototype.getObject).toBeCalled()
})

test('check if RLS policies are respected: authenticated user is able to read authenticated resource without /authenticated prefix', async () => {
const response = await app().inject({
method: 'GET',
url: '/object/bucket2/authenticated/casestudy.png',
headers: {
authorization: `Bearer ${process.env.AUTHENTICATED_KEY}`,
},
})
expect(response.statusCode).toBe(200)
expect(response.headers['etag']).toBe('abc')
expect(response.headers['last-modified']).toBe('Thu, 12 Aug 2021 16:00:00 GMT')
expect(S3Backend.prototype.getObject).toBeCalled()
})

test('forward 304 and If-Modified-Since/If-None-Match headers', async () => {
const mockGetObject = jest.spyOn(S3Backend.prototype, 'getObject')
mockGetObject.mockRejectedValue({
Expand Down Expand Up @@ -99,10 +113,48 @@ describe('testing GET object', () => {
expect(response.headers['cache-control']).toBe('no-cache')
})

test('get authenticated object info without the /authenticated prefix', async () => {
const response = await app().inject({
method: 'HEAD',
url: '/object/bucket2/authenticated/casestudy.png',
headers: {
authorization: `Bearer ${process.env.AUTHENTICATED_KEY}`,
},
})
expect(response.statusCode).toBe(200)
expect(response.headers['etag']).toBe('abc')
expect(response.headers['last-modified']).toBe('Wed, 12 Oct 2022 11:17:02 GMT')
expect(response.headers['content-length']).toBe(3746)
expect(response.headers['cache-control']).toBe('no-cache')
})

test('cannot get authenticated object info without the /authenticated prefix if no jwt is provided', async () => {
const response = await app().inject({
method: 'HEAD',
url: '/object/bucket2/authenticated/casestudy.png',
})
expect(response.statusCode).toBe(400)
})

test('get public object info without using the /public prefix', async () => {
const response = await app().inject({
method: 'HEAD',
url: '/object/public-bucket-2/favicon.ico',
headers: {
authorization: ``,
},
})
expect(response.statusCode).toBe(200)
expect(response.headers['etag']).toBe('abc')
expect(response.headers['last-modified']).toBe('Wed, 12 Oct 2022 11:17:02 GMT')
expect(response.headers['content-length']).toBe(3746)
expect(response.headers['cache-control']).toBe('no-cache')
})

test('get public object info', async () => {
const response = await app().inject({
method: 'HEAD',
url: '/object/public/public-bucket-2/favicon.ico',
url: '/object/public-bucket-2/favicon.ico',
headers: {
authorization: ``,
},
Expand Down Expand Up @@ -158,6 +210,18 @@ describe('testing GET object', () => {
expect(S3Backend.prototype.getObject).not.toHaveBeenCalled()
})

test('check if RLS policies are respected: anon user is not able to read authenticated resource without /authenticated prefix', async () => {
const response = await app().inject({
method: 'GET',
url: '/object/bucket2/authenticated/casestudy.png',
headers: {
authorization: `Bearer ${anonKey}`,
},
})
expect(response.statusCode).toBe(400)
expect(S3Backend.prototype.getObject).not.toHaveBeenCalled()
})

test('user is not able to read a resource without Auth header', async () => {
const response = await app().inject({
method: 'GET',
Expand All @@ -167,6 +231,15 @@ describe('testing GET object', () => {
expect(S3Backend.prototype.getObject).not.toHaveBeenCalled()
})

test('user is not able to read a resource without Auth header without the /authenticated prefix', async () => {
const response = await app().inject({
method: 'GET',
url: '/object/bucket2/authenticated/casestudy.png',
})
expect(response.statusCode).toBe(400)
expect(S3Backend.prototype.getObject).not.toHaveBeenCalled()
})

test('return 400 when reading a non existent object', async () => {
const response = await app().inject({
method: 'GET',
Expand Down

0 comments on commit 040871f

Please sign in to comment.