Skip to content
This repository has been archived by the owner on Aug 9, 2021. It is now read-only.

Commit

Permalink
Make global permissions explicit (#359)
Browse files Browse the repository at this point in the history
* Only show active devices in browse

* - Only show active devices in browse
- Refactor some stations APIs to return more info about what stations were added/updated and how many recordings were matched, if any.
- Simpler get APIs for retrieving group devices, group users.

* Revert docker files

* Fix stations tests

* Python formatting

* Adding missing apidoc info

* All api listing commands will automatically return all results if the user is a super-admin
user.  This PR adds an optional "view-mode" query param to these APIs,
which, if set to "limited", will restrict results to items only directly viewable by the super-admin user.
This is then plumbed through to the various database queries generated.

This does not change any existing behaviour, as that is still the default.

* Fix comparison for retired stations in where clause.

* Fix viewAsSuperAdmin arg passing.

* Addressing review feedback
  • Loading branch information
hardiesoft authored Mar 23, 2021
1 parent 3ded795 commit c1959e4
Show file tree
Hide file tree
Showing 10 changed files with 158 additions and 46 deletions.
8 changes: 5 additions & 3 deletions api/V1/Device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export default function (app: Application, baseUrl: string) {
* @apiGroup Device
* @apiParam onlyActive {Boolean} Only return active devices, defaults to 'true'
* If we want to return *all* devices this must be present and set to 'false'
* @apiParam {string} view-mode (Optional) - can be set to "user"
*
* @apiDescription Returns all devices the user can access
* through both group membership and direct assignment.
Expand All @@ -104,14 +105,15 @@ export default function (app: Application, baseUrl: string) {
apiUrl,
[
auth.authenticateUser,
param("onlyActive").optional().isBoolean().toBoolean()
middleware.viewMode(),
query("onlyActive").optional().isBoolean().toBoolean()
],
middleware.requestWrapper(async (request, response) => {
const onlyActiveDevices = request.param.onlyActive !== false;
const onlyActiveDevices = request.query.onlyActive !== false;
const devices = await models.Device.allForUser(
request.user,
onlyActiveDevices
);
, request.body.viewAsSuperAdmin);
return responseUtil.send(response, {
devices: devices,
statusCode: 200,
Expand Down
87 changes: 79 additions & 8 deletions api/V1/Group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import responseUtil from "./responseUtil";
import { body, param, query } from "express-validator/check";
import { Application } from "express";
import { Validator } from "jsonschema";
import logger from "../../logging";

const JsonSchema = new Validator();

const validateStationsJson = (val, { req }) => {
Expand Down Expand Up @@ -92,7 +90,7 @@ export default function (app: Application, baseUrl: string) {
);

/**
* @api {get} /api/v1/groups Get groups
* @api {get} /api/v1/groups Get all groups the user has access to
* @apiName GetGroups
* @apiGroup Group
*
Expand All @@ -107,11 +105,16 @@ export default function (app: Application, baseUrl: string) {
*/
app.get(
apiUrl,
[auth.authenticateUser, middleware.parseJSON("where", query)],
[
auth.authenticateUser,
middleware.parseJSON("where", query).optional(),
middleware.viewMode()
],
middleware.requestWrapper(async (request, response) => {
const groups = await models.Group.query(
request.query.where,
request.user
request.query.where || {},
request.user,
request.body.viewAsSuperAdmin
);
return responseUtil.send(response, {
statusCode: 200,
Expand All @@ -121,6 +124,39 @@ export default function (app: Application, baseUrl: string) {
})
);

/**
* @api {get} /api/v1/groups/{groupNameOrId} Get a group by name or id
* @apiName GetGroup
* @apiGroup Group
*
* @apiUse V1UserAuthorizationHeader
*
* @apiUse V1ResponseSuccess
* @apiSuccess {Group} groups Array of groups (but should only contain one item)
*
* @apiUse V1ResponseError
*/
app.get(
`${apiUrl}/:groupIdOrName`,
[
auth.authenticateUser,
middleware.getGroupByNameOrIdDynamic(param, "groupIdOrName"),
middleware.viewMode()
],
middleware.requestWrapper(async (request, response) => {
const groups = await models.Group.query(
{ "id": request.body.group.id },
request.user,
request.viewAsSuperAdmin
);
return responseUtil.send(response, {
statusCode: 200,
messages: [],
groups
});
})
);

/**
* @api {get} /api/v1/groups/{groupIdOrName}/devices Retrieves all active devices for a group.
* @apiName GetDevicesForGroup
Expand Down Expand Up @@ -149,7 +185,7 @@ export default function (app: Application, baseUrl: string) {
});
return responseUtil.send(response, {
statusCode: 200,
data: devices.map(({ id, devicename }) => ({
devices: devices.map(({ id, devicename }) => ({
id,
deviceName: devicename
})),
Expand Down Expand Up @@ -185,7 +221,42 @@ export default function (app: Application, baseUrl: string) {
});
return responseUtil.send(response, {
statusCode: 200,
data: users.map(({ username, id, GroupUsers }) => ({
users: users.map(({username, id, GroupUsers }) => ({
userName: username,
id,
isGroupAdmin: GroupUsers.admin
})),
messages: ["Got users for group"]
});
})
);

/**
* @api {get} /api/v1/groups/{groupIdOrName}/users Retrieves all users for a group.
* @apiName GetUsersForGroup
* @apiGroup Group
* @apiDescription A group member or an admin member with globalRead permissions can view users that belong
* to a group.
*
* @apiUse V1UserAuthorizationHeader
*
* @apiParam {Number|String} group name or group id
*
* @apiUse V1ResponseSuccess
* @apiUse V1ResponseError
*/
app.get(
`${apiUrl}/:groupIdOrName/users`,
[
auth.authenticateUser,
middleware.getGroupByNameOrIdDynamic(param, "groupIdOrName"),
auth.userHasReadAccessToGroup
],
middleware.requestWrapper(async (request, response) => {
const users = await request.body.group.getUsers({ attributes: ["id", "username"] });
return responseUtil.send(response, {
statusCode: 200,
users: users.map(({ username, id, GroupUsers }) => ({
userName: username,
id,
isGroupAdmin: GroupUsers.admin
Expand Down
26 changes: 22 additions & 4 deletions api/V1/Recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ export default (app: Application, baseUrl: string) => {
* @apiName QueryVisits
* @apiGroup Recordings
*
* @apiParam {string} view-mode (Optional) - can be set to "user"
*
* @apiUse V1UserAuthorizationHeader
* @apiUse BaseQueryParams
* @apiUse RecordingOrder
Expand All @@ -231,7 +233,11 @@ export default (app: Application, baseUrl: string) => {
*/
app.get(
apiUrl + "/visits",
[auth.authenticateUser, ...queryValidators],
[
auth.authenticateUser,
middleware.viewMode(),
...queryValidators,
],
middleware.requestWrapper(
async (request: e.Request, response: e.Response) => {
const result = await recordingUtil.queryVisits(
Expand Down Expand Up @@ -259,6 +265,8 @@ export default (app: Application, baseUrl: string) => {
* @apiName QueryRecordings
* @apiGroup Recordings
*
* @apiParam {string} view-mode (Optional) - can be set to "user"
*
* @apiUse V1UserAuthorizationHeader
* @apiUse BaseQueryParams
* @apiUse RecordingOrder
Expand All @@ -269,7 +277,11 @@ export default (app: Application, baseUrl: string) => {
*/
app.get(
apiUrl,
[auth.authenticateUser, ...queryValidators],
[
auth.authenticateUser,
middleware.viewMode(),
...queryValidators
],
middleware.requestWrapper(
async (request: e.Request, response: e.Response) => {
const result = await recordingUtil.query(
Expand All @@ -292,6 +304,8 @@ export default (app: Application, baseUrl: string) => {
* @apiName QueryRecordingsCount
* @apiGroup Recordings
*
* @apiParam {string} view-mode (Optional) - can be set to "user"
*
* @apiUse V1UserAuthorizationHeader
* @apiUse BaseQueryParams
* @apiUse MoreQueryParams
Expand All @@ -301,7 +315,11 @@ export default (app: Application, baseUrl: string) => {
*/
app.get(
`${apiUrl}/count`,
[auth.authenticateUser, ...queryValidators],
[
auth.authenticateUser,
middleware.viewMode(),
...queryValidators
],
middleware.requestWrapper(
async (request: RecordingQuery, response: e.Response) => {
const user = request.user;
Expand All @@ -326,7 +344,7 @@ export default (app: Application, baseUrl: string) => {
}
]
};
if (user.hasGlobalRead()) {
if (request.body.viewAsSuperAdmin && user.hasGlobalRead()) {
// Dont' filter on user if the user has global read permissons.
delete countQuery.include[0].include;
}
Expand Down
10 changes: 6 additions & 4 deletions api/V1/recordingUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import log from "../../logging";
import models from "../../models";
import responseUtil from "./responseUtil";
import util from "./util";
import { Response } from "express";
import { Response, Request } from "express";
import {
AudioRecordingMetadata,
Recording,
Expand All @@ -48,7 +48,7 @@ import {
} from "./Visits";
import { Station, StationId } from "../../models/Station";

export interface RecordingQuery {
export interface RecordingQuery extends Request {
user: User;
query: {
where: null | any;
Expand Down Expand Up @@ -181,7 +181,8 @@ async function query(
request.query.tags,
request.query.offset,
request.query.limit,
request.query.order
request.query.order,
request.body.viewAsSuperAdmin
);
builder.query.distinct = true;
const result = await models.Recording.findAndCountAll(builder.get());
Expand Down Expand Up @@ -695,7 +696,8 @@ async function queryVisits(
request.query.tags,
request.query.offset,
queryLimit,
null
null,
request.body.viewAsSuperAdmin
);
builder.query.distinct = true;
builder.addAudioEvents(
Expand Down
2 changes: 1 addition & 1 deletion api/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ const userCanAccessDevices = async (request, response, next) => {
}

try {
await request.user.checkUserControlsDevices(devices);
await request.user.checkUserControlsDevices(devices, request.body.viewAsSuperAdmin);
} catch (e) {
return response.status(403).json({ messages: [e.message] });
}
Expand Down
15 changes: 14 additions & 1 deletion api/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
import {
body,
oneOf,
query,
ValidationChain,
ValidationChainBuilder,
validationResult
Expand Down Expand Up @@ -215,6 +216,17 @@ const checkNewPassword = function (field: string): ValidationChain {
});
};

const viewMode = function(): ValidationChain {
// All api listing commands will automatically return all results if the user is a super-admin
// There is now an optional "view-mode" query param to these APIs, which, if set to "user",
// will restrict results to items only directly viewable by the super-admin user.
// The default behaviour remains unchanged, and this will do nothing for non-admin users.
return query("view-mode").custom((value, {req}) => {
req.body.viewAsSuperAdmin = value !== "user";
return true;
});
};

/**
* Extract and decode a JSON object from the request object.
* If the entry is a string, it will be converted to a proper object,
Expand Down Expand Up @@ -334,5 +346,6 @@ export default {
requestWrapper,
isDateArray,
getUserByEmail,
setGroupName
setGroupName,
viewMode
};
17 changes: 9 additions & 8 deletions models/Device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export interface DeviceStatic extends ModelStaticCommon<Device> {
allForUser: (
user: User,
onlyActive: boolean
) => Promise<{ rows: Device[]; count: number }>;
, viewAsSuperAdmin: boolean) => Promise<{ rows: Device[]; count: number }>;
removeUserFromDevice: (
authUser: User,
device: Device,
Expand Down Expand Up @@ -242,10 +242,11 @@ export default function (
Device.onlyUsersDevicesMatching = async function (
user,
conditions = null,
includeData = null
includeData = null,
viewAsSuperAdmin = true
) {
// Return all devices if user has global write/read permission.
if (user.hasGlobalRead()) {
if (viewAsSuperAdmin && user.hasGlobalRead()) {
return this.findAndCountAll({
where: conditions,
attributes: ["devicename", "id", "GroupId", "active"],
Expand All @@ -254,7 +255,7 @@ export default function (
});
}

const whereQuery = await addUserAccessQuery(user, conditions);
const whereQuery = await addUserAccessQuery(user, conditions, viewAsSuperAdmin);

return this.findAndCountAll({
where: whereQuery,
Expand All @@ -264,7 +265,7 @@ export default function (
});
};

Device.allForUser = async function (user, onlyActive: boolean) {
Device.allForUser = async function (user, onlyActive: boolean, viewAsSuperAdmin: boolean) {
const includeData = [
{
model: models.User,
Expand All @@ -277,7 +278,7 @@ export default function (
user,
includeOnlyActiveDevices,
includeData
);
, viewAsSuperAdmin);
};

Device.newUserPermissions = function (enabled) {
Expand Down Expand Up @@ -666,8 +667,8 @@ order by hour;
*
filters the supplied query by devices and groups authUser is authorized to access
*/
async function addUserAccessQuery(authUser, whereQuery) {
if (authUser.hasGlobalRead()) {
async function addUserAccessQuery(authUser, whereQuery, viewAsSuperAdmin = true) {
if (viewAsSuperAdmin && authUser.hasGlobalRead()) {
return whereQuery;
}
const deviceIds = await authUser.getDeviceIds();
Expand Down
Loading

0 comments on commit c1959e4

Please sign in to comment.