Skip to content

Commit

Permalink
Merge branch 'main' into forms-960-idp-refactor
Browse files Browse the repository at this point in the history
  • Loading branch information
@usingtechnology
usingtechnology authored Jan 25, 2024
2 parents c0c81a0 + 5855386 commit 77db78b
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 464 deletions.
115 changes: 30 additions & 85 deletions app/src/forms/auth/middleware/userAccess.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ const getToken = (req) => {
const setUser = async (req, _res, next) => {
try {
const token = getToken(req);
// we can limit the form list from query string or url params. Url params override query params
// ex. /forms/:formId=ABC/version?formId=123
// the ABC in the url will be used... so don't do that.
const params = { ...req.query, ...req.params };
req.currentUser = await service.login(token, params, req.chefsLoadForms);
req.currentUser = await service.login(token);
next();
} catch (error) {
next(error);
Expand All @@ -43,54 +39,12 @@ const currentUser = async (req, res, next) => {
return setUser(req, res, next);
};

// Temporary version of currentUser that only calls sets req.currentUser for
// requests using Bearer tokens. This can only be used for routes that call
// controller methods that don't use req.currentUser.
//
// Plan: If this code is a success it will eventually be rolled out to all
// endpoints that don't need req.currentUser for API Key use. Then, all those
// that do need req.currentUser will be refactored to use the minimal query we
// need.
const currentUserTemp = async (req, res, next) => {
// Check if authorization header is a bearer token
if (req.headers && req.headers.authorization && req.headers.authorization.startsWith('Bearer ')) {
// need to check keycloak, ensure the bearer token is valid
const token = req.headers.authorization.substring(7);
const ok = await keycloak.grantManager.validateAccessToken(token);
if (!ok) {
return new Problem(403, { detail: 'Authorization token is invalid.' }).send(res);
}
}

// Temporarily set a flag to prevent expensive database calls. We will
// eventually move all routes to working in this way, and then the extra
// downstream logic can be removed.
req.chefsLoadForms = false;

return setUser(req, res, next);
};

// To deal with performance problems, we are going to move away from setting the
// req.currentUser.forms and req.currentUser.deletedForms data. The code using
// those will eventually be removed from this function.
const _getForm = async (currentUser, formId) => {
let forms;
if (currentUser.forms) {
forms = currentUser.forms;
} else {
forms = await service.getUserForms(currentUser, { active: true, formId: formId });
}

const forms = await service.getUserForms(currentUser, { active: true, formId: formId });
let form = forms.find((f) => f.formId === formId);

if (!form) {
let deletedForms;
if (currentUser.deletedForms) {
deletedForms = currentUser.deletedForms;
} else {
deletedForms = await service.getUserForms(currentUser, { active: false, formId: formId });
}

const deletedForms = await service.getUserForms(currentUser, { active: false, formId: formId });
form = deletedForms.find((f) => f.formId === formId);
}

Expand Down Expand Up @@ -160,13 +114,7 @@ const hasSubmissionPermissions = (permissions) => {

// Does the user have permissions for this submission due to their FORM permissions
if (req.currentUser) {
let forms;
if (req.currentUser.forms) {
forms = req.currentUser.forms;
} else {
forms = await service.getUserForms(req.currentUser, { active: true, formId: submissionForm.form.id });
}

const forms = await service.getUserForms(req.currentUser, { active: true, formId: submissionForm.form.id });
let formFromCurrentUser = forms.find((f) => f.formId === submissionForm.form.id);
if (formFromCurrentUser) {
// Do they have the submission permissions being requested on this FORM
Expand Down Expand Up @@ -252,15 +200,17 @@ const filterMultipleSubmissions = () => {
};
};

const hasFormRole = (formId, user, role) => {
const hasFormRole = async (formId, user, role) => {
let hasRole = false;
form_loop: for (let i = 0; i < user.forms.length; i++) {
if (user.forms[i].formId === formId) {
for (let j = 0; j < user.forms[i].roles.length; j++) {
if (user.forms[i].roles[j] === role) {
hasRole = true;
break form_loop;
}

const forms = await service.getUserForms(user, { active: true, formId: formId });
const form = forms.find((f) => f.formId === formId);

if (form) {
for (let j = 0; j < form.roles.length; j++) {
if (form.roles[j] === role) {
hasRole = true;
break;
}
}
}
Expand All @@ -277,26 +227,22 @@ const hasFormRoles = (formRoles, hasAll = false) => {
return new Problem(401, { detail: 'Form Id not found on request.' }).send(res);
}

// Iterate all the forms the current user has access to
form_loop: for (let formIndex = 0; formIndex < req.currentUser.forms.length; formIndex++) {
// If the indexed form is the form we're checking role access
if (req.query.formId === req.currentUser.forms[formIndex].formId) {
// Iterate all the roles for this form
for (let roleIndex = 0; roleIndex < req.currentUser.forms[formIndex].roles.length; roleIndex++) {
let index = formRoles.indexOf(req.currentUser.forms[formIndex].roles[roleIndex]);
// If the user has the indexed role requested by the route
if (index > -1) {
// If the route specifies all roles must exist for the form
if (hasAll)
// Remove that role from the search
formRoles.splice(index, 1);
// The user has at least one of the roles
else return next();
}

// The user has all of the required roles
if (formRoles.length == 0) break form_loop;
const forms = await service.getUserForms(req.currentUser, { active: true, formId: formId });
const form = forms.find((f) => f.formId === formId);
if (form) {
for (let roleIndex = 0; roleIndex < form.roles.length; roleIndex++) {
let index = formRoles.indexOf(form.roles[roleIndex]);
// If the user has the indexed role requested by the route
if (index > -1) {
// If the route specifies all roles must exist for the form
if (hasAll)
// Remove that role from the search
formRoles.splice(index, 1);
// The user has at least one of the roles
else return next();
}
// The user has all of the required roles
if (formRoles.length == 0) break;
}
}

Expand All @@ -321,7 +267,7 @@ const hasRolePermissions = (removingUsers = false) => {
const currentUser = req.currentUser;
const data = req.body;

const isOwner = hasFormRole(formId, currentUser, Roles.OWNER);
const isOwner = await hasFormRole(formId, currentUser, Roles.OWNER);

if (removingUsers) {
if (data.includes(currentUser.id)) return next(new Problem(401, { detail: "You can't remove yourself from this form." }));
Expand Down Expand Up @@ -384,7 +330,6 @@ const hasRolePermissions = (removingUsers = false) => {

module.exports = {
currentUser,
currentUserTemp,
hasFormPermissions,
hasSubmissionPermissions,
hasFormRoles,
Expand Down
12 changes: 2 additions & 10 deletions app/src/forms/auth/service.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,19 +201,11 @@ const service = {
};
},

login: async (token, params = {}, loadForms = true) => {
login: async (token) => {
const userInfo = service.parseToken(token);
const user = await service.getUserId(userInfo);

if (!loadForms) {
return { ...user };
}

const forms = await service.getUserForms(user, params); // get forms for user (filtered by params)...
params.active = false;
const deletedForms = await service.getUserForms(user, params); // get forms for user (filtered by params)...

return { ...user, forms: forms, deletedForms: deletedForms };
return { ...user };
},

// -------------------------------------------------------------------------------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions app/src/forms/form/routes.js
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
const config = require('config');
const routes = require('express').Router();
const apiAccess = require('../auth/middleware/apiAccess');
const { currentUserTemp, hasFormPermissions } = require('../auth/middleware/userAccess');
const { currentUser, hasFormPermissions } = require('../auth/middleware/userAccess');
const P = require('../common/constants').Permissions;
const rateLimiter = require('../common/middleware').apiKeyRateLimiter;

const keycloak = require('../../components/keycloak');
const controller = require('./controller');

routes.use(currentUserTemp);
routes.use(currentUser);

routes.get('/', keycloak.protect(`${config.get('server.keycloak.clientId')}:admin`), async (req, res, next) => {
await controller.listForms(req, res, next);
Expand Down
4 changes: 2 additions & 2 deletions app/src/forms/rbac/routes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ const controller = require('./controller');
const keycloak = require('../../components/keycloak');
const P = require('../common/constants').Permissions;
const R = require('../common/constants').Roles;
const { currentUserTemp, hasFormPermissions, hasSubmissionPermissions, hasFormRoles, hasRolePermissions } = require('../auth/middleware/userAccess');
const { currentUser, hasFormPermissions, hasSubmissionPermissions, hasFormRoles, hasRolePermissions } = require('../auth/middleware/userAccess');

routes.use(currentUserTemp);
routes.use(currentUser);

routes.get('/current', keycloak.protect(), async (req, res, next) => {
await controller.getCurrentUser(req, res, next);
Expand Down
4 changes: 2 additions & 2 deletions app/src/forms/submission/routes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ const routes = require('express').Router();

const controller = require('./controller');
const P = require('../common/constants').Permissions;
const { currentUserTemp, hasSubmissionPermissions, filterMultipleSubmissions } = require('../auth/middleware/userAccess');
const { currentUser, hasSubmissionPermissions, filterMultipleSubmissions } = require('../auth/middleware/userAccess');
const rateLimiter = require('../common/middleware').apiKeyRateLimiter;

routes.use(currentUserTemp);
routes.use(currentUser);

routes.get('/:formSubmissionId', rateLimiter, apiAccess, hasSubmissionPermissions(P.SUBMISSION_READ), async (req, res, next) => {
await controller.read(req, res, next);
Expand Down
8 changes: 1 addition & 7 deletions app/tests/unit/forms/auth/authService.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,17 @@ describe('formAccessToForm', () => {
describe('login', () => {
const resultSample = {
user: 'me',
forms: [{ formID: 1 }, { formId: 2 }],
deletedForms: [{ formID: 1 }, { formId: 2 }],
};

it('returns a currentUser object', async () => {
service.parseToken = jest.fn().mockReturnValue('userInf');
service.getUserId = jest.fn().mockReturnValue({ user: 'me' });
service.getUserForms = jest.fn().mockReturnValue([{ formID: 1 }, { formId: 2 }]);
const params = { p: 1 };
const token = 'token';
const result = await service.login(token, params);
const result = await service.login(token);
expect(service.parseToken).toHaveBeenCalledTimes(1);
expect(service.parseToken).toHaveBeenCalledWith(token);
expect(service.getUserId).toHaveBeenCalledTimes(1);
expect(service.getUserId).toHaveBeenCalledWith('userInf');
expect(service.getUserForms).toHaveBeenCalledTimes(2);
expect(service.getUserForms).toHaveBeenCalledWith({ user: 'me' }, { p: 1, active: false });
expect(result).toBeTruthy();
expect(result).toEqual(resultSample);
});
Expand Down
Loading

0 comments on commit 77db78b

Please sign in to comment.