Skip to content

Commit

Permalink
Merge pull request #833 from oslokommune/protect-api-secrets
Browse files Browse the repository at this point in the history 
Add alternative auth mechanism for API usage
  • Loading branch information
@petterhj
petterhj authored Nov 7, 2023
2 parents 5987196 + e180685 commit 6e14020
Show file tree
Hide file tree
Showing 30 changed files with 1,388 additions and 186 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ All notable changes to this project will be documented in this file. The format

- New key results are now given a start value of 0, a target value of 100, and
percentage as unit of measurement by default.
- The API authorization mechanism has been reworked. The API now accepts a pair
of `okr-client-id` and `okr-client-secret` headers to authorize clients.
The interface for managing client credentials can be found in the item
navigation bar. The existing `okr-team-secret` header is considered deprecated
and will continue to work for existing clients until migrated.

### Fixed

Expand Down
12 changes: 11 additions & 1 deletion firestore.rules
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ service cloud.firestore {

// Check if the document the user tries to access has an organization - this means that is is a product or department
let hasOrgProperty = 'organization' in request.resource.data;
let isAdminFromOrgOfProdOrDep = userIsAdmin && hasOrgProperty && getAfter(request.resource.data.organization).data.id in userDoc.data.admin;
let isAdminFromOrgOfProdOrDep = userIsAdmin && hasOrgProperty && getAfter(request.resource.data.organization).id in userDoc.data.admin;

// Check if the user has access to the document which is an organization (given that the first check is false)
let isAdminOfOrg = userIsAdmin && request.resource.data.id in userDoc.data.admin;
Expand Down Expand Up @@ -216,6 +216,16 @@ service cloud.firestore {
allow delete: if isSuperAdmin() || isMemberOfParent(document, 'kpis') || isAdminOfParent(document, 'kpis');
}

match /apiClients/{document} {
allow read: if isSignedIn();
allow write: if isSuperAdmin() || isMemberOfParent(document, 'apiClients') || isAdminOfParent(document, 'apiClients');

match /secrets/{key} {
allow read: if false;
allow write: if isSuperAdmin() || isMemberOfParent(document, 'apiClients') || isAdminOfParent(document, 'apiClients');
}
}

match /slugs/{document} {
allow read: if true;
allow write: if false;
Expand Down
80 changes: 80 additions & 0 deletions functions/api/helpers.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import crypto from 'crypto';
import { FieldValue, getFirestore } from 'firebase-admin/firestore';
import { endOfDay, startOfDay, setHours, isWithinInterval, sub } from 'date-fns';

Expand Down Expand Up @@ -209,3 +210,82 @@ function isKPIStale(updateFrequency, progressRecord) {
return null;
}
}

/**
* Return true if the client is authorized to perform the requested API
* operation. Set response data and return false in case the client is not
* authorized based on provided request parameters.
*
* `parentRef` is the item to check API authorization against.
* `req` is the HTTP request object.
* `res` is the HTTP response object.
*/
export async function checkApiAuth(parentRef, req, res) {
const parentData = await parentRef.get().then((snapshot) => snapshot.data());

const {
'okr-client-id': clientId,
'okr-client-secret': clientSecret,
'okr-team-secret': teamSecret,
} = req.matchedData;

if (clientId && clientSecret) {
const db = getFirestore();
const apiClientRef = await db
.collection('apiClients')
.where('parent', '==', parentRef)
.where('clientId', '==', clientId)
.where('archived', '==', false)
.limit(1)
.get()
.then((snapshot) => (!snapshot.empty ? snapshot.docs[0].ref : null));

if (!apiClientRef) {
const { name: parentName } = parentData;
res.status(401).json({
message:
`No API client \`${clientId}\` exists for '${parentName}'. Please ` +
'create one in the OKR Tracker integrations admin.',
});
return false;
}

const apiClientSecret = await apiClientRef
.collection('secrets')
.where('archived', '==', false)
.orderBy('created', 'desc')
.limit(1)
.get()
.then((snapshot) => (!snapshot.empty ? snapshot.docs[0].data() : null));

const hashedSecret = crypto.createHash('sha256').update(clientSecret).digest('hex');

if (!apiClientSecret || apiClientSecret.secret !== hashedSecret) {
res.status(401).json({
message:
`Invalid client secret provided for API client \`${clientId}\`. ` +
'Check the OKR Tracker integrations admin for how to rotate the secret.',
});
return false;
}

await apiClientRef.update({ used: FieldValue.serverTimestamp() });
return true;
}

if (!parentData.secret) {
res.status(401).json({
message:
`'${parentData.name}' is not set up for API usage. Please set ` +
'a secret through the OKR Tracker integrations admin.',
});
return false;
}

if (parentData.secret !== teamSecret) {
res.status(401).json({ message: 'Wrong `okr-team-secret`' });
return false;
}

return true;
}
8 changes: 8 additions & 0 deletions functions/api/index.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import functions from 'firebase-functions';

import express from 'express';
import rateLimit from 'express-rate-limit';
import cors from 'cors';
import morgan from 'morgan';

Expand All @@ -12,9 +13,16 @@ import kpiRoutes from './routes/kpi.js';
import statusRoutes from './routes/status.js';
import userRoutes from './routes/user.js';

const apiLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // max 100 requests per window
message: 'Too many requests, please try again later.',
});

const app = express();

app.use(cors());
app.use(apiLimiter);
app.use(express.json());
app.use(morgan('combined'));

Expand Down
37 changes: 9 additions & 28 deletions functions/api/routes/keyres.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,30 @@ import express from 'express';
import validator from 'express-validator';

import { getFirestore } from 'firebase-admin/firestore';
import { checkApiAuth } from '../helpers.js';
import {
commentValidator,
idValidator,
progressValidator,
teamSecretValidator,
clientSecretValidator,
} from '../validators.js';
import validateRules from '../validateRules.js';

const { param, matchedData, validationResult } = validator;
const { param, matchedData } = validator;
const router = express.Router();

router.post(
'/:id',
teamSecretValidator,
clientSecretValidator,
idValidator,
progressValidator,
commentValidator,
validateRules,
async (req, res) => {
const result = validationResult(req);

if (!result.isEmpty()) {
res.status(400).json({
message: 'Invalid request data',
errors: result.mapped(),
});
return;
}

const { 'okr-team-secret': teamSecret, id, progress, comment } = matchedData(req);
const { id, progress, comment } = req.matchedData;

const db = getFirestore();
const collection = await db.collection('keyResults');
const collection = db.collection('keyResults');
let keyRes;

try {
Expand All @@ -47,19 +40,7 @@ router.post(

const { parent } = keyRes.data();

const parentData = await parent.get().then((snapshot) => snapshot.data());

if (!parentData.secret) {
res
.status(401)
.send(
`'${parentData.name}' is not set up for API usage. Please set ` +
'a secret using the OKR Tracker admin interface.'
);
return;
}
if (parentData.secret !== teamSecret) {
res.status(401).send('Wrong okr-team-secret');
if (!(await checkApiAuth(parent, req, res))) {
return;
}

Expand Down
91 changes: 16 additions & 75 deletions functions/api/routes/kpi.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,38 +8,30 @@ import {
isArchived,
refreshKPILatestValue,
updateKPIProgressionValue,
checkApiAuth,
} from '../helpers.js';
import {
commentValidator,
dateValidator,
idValidator,
progressValidator,
teamSecretValidator,
clientSecretValidator,
valueValidator,
} from '../validators.js';
import validateRules from '../validateRules.js';

const { matchedData, validationResult } = validator;
const { matchedData } = validator;
const router = express.Router();

router.post(
'/:id',
teamSecretValidator,
clientSecretValidator,
idValidator,
progressValidator,
commentValidator,
validateRules,
async (req, res) => {
const result = validationResult(req);

if (!result.isEmpty()) {
res.status(400).json({
message: 'Invalid request data',
errors: result.mapped(),
});
return;
}

const { 'okr-team-secret': teamSecret, id, progress, comment } = matchedData(req);

const { id, progress, comment } = req.matchedData;
const db = getFirestore();

try {
Expand All @@ -54,19 +46,7 @@ router.post(

const { parent } = kpi.data();

const parentData = await parent.get().then((snapshot) => snapshot.data());

if (!parentData.secret) {
res
.status(401)
.send(
`'${parentData.name}' is not set up for API usage. Please set ` +
'a secret using the OKR Tracker admin interface.'
);
return;
}
if (parentData.secret !== teamSecret) {
res.status(401).send('Wrong okr-team-secret');
if (!(await checkApiAuth(parent, req, res))) {
return;
}

Expand Down Expand Up @@ -188,25 +168,15 @@ router.get('/:id/values', idValidator, async (req, res) => {

router.put(
'/:id/values/:date',
teamSecretValidator,
clientSecretValidator,
idValidator,
dateValidator,
valueValidator,
commentValidator,
validateRules,
async (req, res) => {
const result = validationResult(req);

if (!result.isEmpty()) {
res.status(400).json({
message: 'Invalid request data',
errors: result.mapped(),
});
return;
}

const { 'okr-team-secret': teamSecret, id, date, value, comment } = matchedData(req);
const { id, date, value, comment } = req.matchedData;
const formattedDate = format(date, 'yyyy-MM-dd');

const db = getFirestore();

try {
Expand All @@ -220,18 +190,8 @@ router.put(
}

const { parent } = kpi.data();
const parentData = await parent.get().then((snapshot) => snapshot.data());

if (!parentData.secret) {
res.status(401).json({
message:
`'${parentData.name}' is not set up for API usage. Please set ` +
'a secret using the OKR Tracker admin interface.',
});
return;
}
if (parentData.secret !== teamSecret) {
res.status(401).json({ message: 'Wrong okr-team-secret' });
if (!(await checkApiAuth(parent, req, res))) {
return;
}

Expand All @@ -250,21 +210,12 @@ router.put(

router.delete(
'/:id/values/:date',
teamSecretValidator,
clientSecretValidator,
idValidator,
dateValidator,
validateRules,
async (req, res) => {
const result = validationResult(req);

if (!result.isEmpty()) {
res.status(400).json({
message: 'Invalid request data',
errors: result.mapped(),
});
return;
}

const { 'okr-team-secret': teamSecret, id, date } = matchedData(req);
const { id, date } = req.matchedData;
const formattedDate = format(date, 'yyyy-MM-dd');

const db = getFirestore();
Expand All @@ -280,18 +231,8 @@ router.delete(
}

const { parent } = kpi.data();
const parentData = await parent.get().then((snapshot) => snapshot.data());

if (!parentData.secret) {
res.status(401).json({
message:
`'${parentData.name}' is not set up for API usage. Please set ` +
'a secret using the OKR Tracker admin interface.',
});
return;
}
if (parentData.secret !== teamSecret) {
res.status(401).json({ message: 'Wrong okr-team-secret' });
if (!(await checkApiAuth(parent, req, res))) {
return;
}

Expand Down
17 changes: 17 additions & 0 deletions functions/api/validateRules.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { matchedData, validationResult } from 'express-validator';

const validateRules = (req, res, next) => {
const errors = validationResult(req);

if (errors.isEmpty()) {
req.matchedData = matchedData(req);
return next();
}

return res.status(400).json({
message: 'Invalid request data',
errors: errors.mapped(),
});
};

export default validateRules;
Loading

0 comments on commit 6e14020

Please sign in to comment.