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

Revert "Revert "feature/DEVSU-2384-permission-storage-improvement"" #389

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
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
13 changes: 13 additions & 0 deletions app/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,17 @@ module.exports = {
MASTER_REPORT_ACCESS: ['admin', 'manager'],
ALL_PROJECTS_ACCESS: ['admin', 'all projects access'],
UPDATE_METHODS: ['POST', 'PUT', 'DELETE'],
USER_GROUPS: [
'admin',
'manager',
'report assignment access',
'create report access',
'germline access',
'non-production access',
'unreviewed access',
'all projects access',
'template edit access',
'appendix edit access',
'variant-text edit access',
],
};
1 change: 0 additions & 1 deletion app/libs/__mocks__/getUser.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const include = [
attributes: {
exclude: ['id', 'deletedAt', 'updatedAt', 'createdAt', 'updatedBy'],
},
through: {attributes: []},
},
{
model: db.models.project,
Expand Down
3 changes: 1 addition & 2 deletions app/libs/getUser.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ const include = [
model: db.models.userGroup,
as: 'groups',
attributes: {
exclude: ['id', 'deletedAt', 'updatedAt', 'createdAt', 'updatedBy'],
exclude: ['id', 'deletedAt', 'updatedAt', 'createdAt', 'updatedBy', 'userId'],
},
through: {attributes: []},
},
{
model: db.models.project,
Expand Down
11 changes: 5 additions & 6 deletions app/middleware/group.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,12 @@ const db = require('../models');
const logger = require('../log');

// Middleware for user groups
module.exports = async (req, res, next, ident) => {
module.exports = async (req, res, next, group) => {
let result;
try {
result = await db.models.userGroup.findOne({
where: {ident},
result = await db.models.user.scope('public').findAll({
include: [
{as: 'users', model: db.models.user, attributes: {exclude: ['id', 'deletedAt', 'password', 'updatedBy']}, through: {attributes: []}},
{as: 'groups', model: db.models.userGroup, attributes: [], where: {name: group}},
],
});
} catch (error) {
Expand All @@ -20,12 +19,12 @@ module.exports = async (req, res, next, ident) => {
}

if (!result) {
logger.error(`Unable to find group ${ident}`);
logger.error(`Unable to find group ${group}`);
return res.status(HTTP_STATUS.NOT_FOUND).json({
error: {message: 'Unable to find group'},
});
}

req.group = result;
req.group = {name: group, users: result};
return next();
};
2 changes: 1 addition & 1 deletion app/models/clearCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const CLEAR_GERMLINE_CACHE_MODELS = [
];

const CLEAR_USER_CACHE_MODELS = [
'userGroup', 'userGroupMember', 'userProject',
'userGroup', 'userProject',
];

/**
Expand Down
14 changes: 2 additions & 12 deletions app/models/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,9 @@ user.belongsToMany(analysisReports, {
});

const userGroup = require('./user/userGroup')(sequelize, Sq);
const userGroupMember = require('./user/userGroupMember')(sequelize, Sq);

user.belongsToMany(userGroup, {
as: 'groups', through: {model: userGroupMember, unique: false}, foreignKey: 'user_id', otherKey: 'group_id', onDelete: 'CASCADE',
});
userGroup.belongsToMany(user, {
as: 'users', through: {model: userGroupMember, unique: false}, foreignKey: 'group_id', otherKey: 'user_id', onDelete: 'CASCADE',
user.hasMany(userGroup, {
as: 'groups', foreignKey: 'userId', onDelete: 'CASCADE', constraints: true,
});

// IMPORTANT must be done before the variant models are defined
Expand Down Expand Up @@ -487,9 +483,6 @@ const notification = require('./notification/notification')(sequelize, Sq);
user.hasMany(notification, {
as: 'notifications', foreignKey: 'userId', onDelete: 'CASCADE', constraints: true,
});
userGroup.hasMany(notification, {
as: 'notifications', foreignKey: 'userGroupId', onDelete: 'CASCADE', constraints: true,
});
project.hasMany(notification, {
as: 'notifications', foreignKey: 'projectId', onDelete: 'CASCADE', constraints: true,
});
Expand All @@ -505,9 +498,6 @@ notification.belongsTo(user, {
notification.belongsTo(project, {
as: 'project', foreignKey: 'projectId', targetKey: 'id', onDelete: 'CASCADE', constraints: true,
});
notification.belongsTo(userGroup, {
as: 'userGroup', foreignKey: 'userGroupId', targetKey: 'id', onDelete: 'CASCADE', constraints: true,
});

const notificationTrack = require('./notification/notificationTrack')(sequelize, Sq);

Expand Down
17 changes: 6 additions & 11 deletions app/models/notification/notification.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const {DEFAULT_COLUMNS, DEFAULT_OPTIONS} = require('../base');
const {USER_GROUPS} = require('../../constants');

module.exports = (sequelize, Sq) => {
const notification = sequelize.define(
Expand Down Expand Up @@ -34,16 +35,12 @@ module.exports = (sequelize, Sq) => {
key: 'id',
},
},
userGroupId: {
type: Sq.INTEGER,
name: 'userGroupId',
field: 'user_group_id',
userGroup: {
name: 'userGroup',
field: 'user_group',
unique: false,
allowNull: true,
references: {
model: 'user_groups',
key: 'id',
},
type: Sq.ENUM(USER_GROUPS),
},
templateId: {
type: Sq.INTEGER,
Expand All @@ -62,19 +59,17 @@ module.exports = (sequelize, Sq) => {
scopes: {
public: {
attributes: {
exclude: ['id', 'deletedAt', 'updatedBy', 'userId', 'projectId', 'userGroupId', 'templateId'],
exclude: ['id', 'deletedAt', 'updatedBy', 'userId', 'projectId', 'templateId'],
},
include: [
{model: sequelize.models.user.scope('minimal'), as: 'user'},
{model: sequelize.models.userGroup.scope('minimal'), as: 'userGroup'},
{model: sequelize.models.template.scope('minimal'), as: 'template'},
{model: sequelize.models.project.scope('minimal'), as: 'project'},
],
},
extended: {
include: [
{model: sequelize.models.user, as: 'user'},
{model: sequelize.models.userGroup, as: 'userGroup', include: {model: sequelize.models.user, as: 'users', through: {attributes: []}}},
{model: sequelize.models.template, as: 'template'},
{model: sequelize.models.project, as: 'project'},
],
Expand Down
24 changes: 14 additions & 10 deletions app/models/user/userGroup.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,36 @@
const {DEFAULT_COLUMNS, DEFAULT_OPTIONS} = require('../base');
const {USER_GROUPS} = require('../../constants');

module.exports = (sequelize, Sq) => {
const userGroup = sequelize.define('userGroup', {
...DEFAULT_COLUMNS,
name: {
type: Sq.STRING,
userId: {
name: 'userId',
field: 'user_id',
type: Sq.INTEGER,
references: {
model: 'users',
key: 'id',
},
allowNull: false,
},
description: {
type: Sq.STRING,
allowNull: true,
name: {
type: Sq.ENUM(USER_GROUPS),
allowNull: false,
},
}, {
...DEFAULT_OPTIONS,
tableName: 'user_groups',
scopes: {
public: {
order: [['name', 'ASC']],
order: [['user_id', 'ASC']],
attributes: {
exclude: ['id', 'deletedAt', 'updatedBy'],
},
include: [
{as: 'users', model: sequelize.models.user, attributes: {exclude: ['id', 'deletedAt', 'password', 'updatedBy']}, through: {attributes: []}},
{as: 'users', model: sequelize.models.user, attributes: {exclude: ['id', 'deletedAt', 'password', 'updatedBy']}},
],
},
minimal: {
attributes: ['ident', 'name'],
},
},
});

Expand Down
32 changes: 0 additions & 32 deletions app/models/user/userGroupMember.js

This file was deleted.

25 changes: 20 additions & 5 deletions app/routes/notification/notification.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const HTTP_STATUS = require('http-status-codes');
const express = require('express');
const {USER_GROUPS} = require('../../constants');

const db = require('../../models');
const logger = require('../../log');
Expand All @@ -12,7 +13,6 @@ const pairs = {
user: db.models.user,
project: db.models.project,
template: db.models.template,
user_group: db.models.userGroup,
};

// for each entry in pairs, assumes the key-named value in
Expand Down Expand Up @@ -72,10 +72,18 @@ router.route('/')
});
}
}

if (!USER_GROUPS.includes(req.body.user_group) && req.body.user_group) {
logger.error(`${req.body.user_group} group not found`);
return res.status(HTTP_STATUS.NOT_FOUND).json({
error: {message: `${req.body.user_group} group not found`},
});
}

try {
const whereClause = {
...((req.body.user_id == null) ? {} : {user_id: req.body.user_id}),
...((req.body.user_group_id == null) ? {} : {user_group_id: req.body.user_group_id}),
...((req.body.user_group == null) ? {} : {user_group: req.body.user_group}),
...((req.body.template_id == null) ? {} : {template_id: req.body.template_id}),
...((req.body.project_id == null) ? {} : {project_id: req.body.project_id}),
};
Expand All @@ -100,13 +108,13 @@ router.route('/')
return res.status(HTTP_STATUS.FORBIDDEN);
}

if (req.body.user_id && req.body.user_group_id) {
if (req.body.user_id && req.body.user_group) {
return res.status(HTTP_STATUS.CONFLICT).json({
error: {message: 'Only one of user and user group should be specified'},
});
}

if (!req.body.user_id && !req.body.user_group_id) {
if (!req.body.user_id && !req.body.user_group) {
return res.status(HTTP_STATUS.CONFLICT).json({
error: {message: 'Exactly one of user and user group should be specified'},
});
Expand All @@ -130,6 +138,13 @@ router.route('/')
});
}

if (!USER_GROUPS.includes(req.body.user_group) && req.body.user_group) {
logger.error(`${req.body.user_group} group not found`);
return res.status(HTTP_STATUS.NOT_FOUND).json({
error: {message: `${req.body.user_group} group not found`},
});
}

// check the given user, if any, is bound to the given project (or is admin)
if (req.body.user_id) {
let notifUser;
Expand Down Expand Up @@ -173,7 +188,7 @@ router.route('/')
where: {
projectId: req.body.project_id,
userId: req.body.user_id ? req.body.user_id : null,
userGroupId: req.body.user_group_id ? req.body.user_group_id : null,
userGroup: req.body.user_group ? req.body.user_group : null,
eventType: req.body.event_type,
templateId: req.body.template_id,
},
Expand Down
4 changes: 2 additions & 2 deletions app/routes/swagger/schemas.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const ID_FIELDS = [
];
const PUBLIC_VIEW_EXCLUDE = [...ID_FIELDS, 'id', 'reportId', 'geneId', 'deletedAt', 'updatedBy'];
const GENERAL_EXCLUDE = REPORT_EXCLUDE.concat(ID_FIELDS);
const GENERAL_EXCLUDE_ASSOCIATIONS = ['report', 'reports', 'germlineReport', 'userProject', 'userGroupMember'];
const GENERAL_EXCLUDE_ASSOCIATIONS = ['report', 'reports', 'germlineReport', 'userProject'];

const MODELS_WITH_VARIANTS = ['kbMatches', 'genes'];

Expand Down Expand Up @@ -96,7 +96,7 @@ const getExcludes = (model) => {

// Remove joining models from the list of models to use for generating schemas
const {
userGroupMember, reportProject, userProject, ...models
reportProject, userProject, ...models
} = db.models;

// Generate schemas from Sequelize models. One for the public returned value, one
Expand Down
Loading
Loading