Skip to content

Commit 539eb04

Browse files
TimCsakyTimCsaky
committed
Apply IDP Permissions to search operations
Check for READ permissions granted to user's IDP in addition to user-level permissions, when config.privacyMask is ON. This allows users to see objects shared with their IDP, even if they don't have direct permissions on the object.
1 parent 8636024 commit 539eb04

File tree

9 files changed

+119
-46
lines changed

9 files changed

+119
-46
lines changed

‎app/src/controllers/object.js‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,7 @@ const controller = {
653653
// if scoping to current user permissions on objects
654654
if (getConfigBoolean('server.privacyMask')) {
655655
params.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));
656+
params.idp = req.currentUser.tokenPayload ? req.currentUser.tokenPayload.identity_provider : undefined;
656657
}
657658
const response = await metadataService.fetchMetadataForObject(params);
658659
res.status(200).json(response);
@@ -682,6 +683,8 @@ const controller = {
682683
// if scoping to current user permissions on objects
683684
if (getConfigBoolean('server.privacyMask')) {
684685
params.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));
686+
// params.idp = 'bc-box-local-4545';
687+
params.idp = req.currentUser.tokenPayload ? req.currentUser.tokenPayload.identity_provider : undefined;
685688
}
686689
const response = await tagService.fetchTagsForObject(params);
687690
res.status(200).json(response);
@@ -1062,23 +1065,25 @@ const controller = {
10621065
};
10631066
// if scoping to current user permissions on objects
10641067
if (getConfigBoolean('server.privacyMask')) {
1068+
params.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));
1069+
params.idp = req.currentUser.tokenPayload ? req.currentUser.tokenPayload.identity_provider : undefined;
10651070

1066-
if (req.currentUser.authType === AuthType.NONE) {
1071+
}
10671072

1068-
// no-auth requests MUST have all of the following:
1069-
// (a) an object or bucket id; (b) ?public=true; (c) not search by S3 path
1070-
if (!(params.bucketId || params.id) || !params.public || params.path) {
1071-
throw new Problem(403, {
1072-
detail: 'User lacks permission to complete this action',
1073-
instance: req.originalUrl
1074-
});
1075-
}
1073+
// if no-auth, MUST have ALL of the following:
1074+
// (a) an object or bucket id; (b) ?public=true; (c) not search by S3 path
1075+
if (req.currentUser.authType === AuthType.NONE) {
1076+
if (!(params.bucketId || params.id) || !params.public || params.path) {
1077+
throw new Problem(403, {
1078+
detail: 'User lacks permission to complete this action',
1079+
instance: req.originalUrl
1080+
});
10761081
}
1077-
params.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));
10781082
}
10791083

10801084
const response = await objectService.searchObjects(params);
10811085

1086+
// if no-auth request, redact sensitive fields from response
10821087
if (req.currentUser.authType === AuthType.NONE) {
10831088
const redactedFields = ['path', 'createdBy', 'updatedBy', 'lastSyncedDate'];
10841089
const redactedResponseData = response.data.map(object => utils.redactSecrets(object, redactedFields));

‎app/src/controllers/version.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const controller = {
3838
// if scoping to current user permissions on objects
3939
if (getConfigBoolean('server.privacyMask')) {
4040
params.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));
41+
params.idp = req.currentUser.tokenPayload ? req.currentUser.tokenPayload.identity_provider : undefined;
4142
params.bucketId = bucketId?.length ? bucketId : undefined;
4243
}
4344
const response = await metadataService.fetchMetadataForVersion(params);
@@ -70,6 +71,7 @@ const controller = {
7071
// if scoping to current user permissions on objects
7172
if (getConfigBoolean('server.privacyMask')) {
7273
params.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));
74+
params.idp = req.currentUser.tokenPayload ? req.currentUser.tokenPayload.identity_provider : undefined;
7375
params.bucketId = bucketId?.length ? bucketId : undefined;
7476
}
7577
const response = await tagService.fetchTagsForVersion(params);

‎app/src/db/models/tables/objectModel.js‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ class ObjectModel extends Timestamps(Model) {
1414
const Bucket = require('./bucket');
1515
const ObjectPermission = require('./objectPermission');
1616
const BucketPermission = require('./bucketPermission');
17+
const ObjectIdpPermission = require('./objectIdpPermission');
18+
const BucketIdpPermission = require('./bucketIdpPermission');
1719
const Version = require('./version');
1820

1921
return {
@@ -48,7 +50,23 @@ class ObjectModel extends Timestamps(Model) {
4850
from: 'object.bucketId',
4951
to: 'bucket_permission.bucketId'
5052
}
51-
}
53+
},
54+
objectIdpPermission: {
55+
relation: Model.HasManyRelation,
56+
modelClass: ObjectIdpPermission,
57+
join: {
58+
from: 'object.id',
59+
to: 'object_idp_permission.objectId'
60+
}
61+
},
62+
bucketIdpPermission: {
63+
relation: Model.HasManyRelation,
64+
modelClass: BucketIdpPermission,
65+
join: {
66+
from: 'object.bucketId',
67+
to: 'bucket_idp_permission.bucketId'
68+
}
69+
},
5270
};
5371
}
5472

@@ -133,14 +151,17 @@ class ObjectModel extends Timestamps(Model) {
133151
findPath(query, value) {
134152
if (value) query.where('object.path', value);
135153
},
136-
hasPermission(query, userId, permCode) {
137-
if (userId && permCode) {
154+
hasPermission(query, { userId, idp, permCode }) {
155+
// userId will be defined if config.privacyMask is ON, in which case we want to filter by permissions.
156+
if (userId && idp && permCode) {
138157
query
139158
// withGraphFetched keep joining using default 'left join' operation,
140159
// to fix default behavior we are adding extra joinOperation which seems to be working with
141160
// corresponding JoinRelated
142-
.withGraphFetched('[objectPermission, bucketPermission]', { joinOperation: 'fullOuterJoinRelated' })
143-
.fullOuterJoinRelated('[objectPermission, bucketPermission]')
161+
.withGraphFetched('[objectPermission, bucketPermission, objectIdpPermission, bucketIdpPermission]', {
162+
joinOperation: 'fullOuterJoinRelated'
163+
})
164+
.fullOuterJoinRelated('[objectPermission, bucketPermission, objectIdpPermission, bucketIdpPermission]')
144165
// wrap in WHERE to make contained clauses exclusive of root query
145166
.where(query => {
146167
query
@@ -157,6 +178,20 @@ class ObjectModel extends Timestamps(Model) {
157178
'bucketPermission.permCode': permCode,
158179
'bucketPermission.userId': userId
159180
});
181+
})
182+
.orWhere(query => {
183+
query
184+
.where({
185+
'objectIdpPermission.permCode': permCode,
186+
'objectIdpPermission.idp': idp
187+
});
188+
})
189+
.orWhere(query => {
190+
query
191+
.where({
192+
'bucketIdpPermission.permCode': permCode,
193+
'bucketIdpPermission.idp': idp
194+
});
160195
});
161196
});
162197
} else {

‎app/src/middleware/authorization.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ const {
2323
* @function _checkIdpPermission
2424
* Checks if the current user is authorized to perform the operation because
2525
* they have the required permission through their identity provider (idp)
26-
* the identity provider of their athentication (eg: idir, bcsc, etc)
26+
* of their athentication (eg: idir, bcsc, etc)
2727
* @param {object} req.currentUser Express request object currentUser
2828
* @param {object} req.params Express request object params
2929
* @param {string} permission The permission to check against

‎app/src/services/metadata.js‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ const service = {
182182
fetchMetadataForObject: (params) => {
183183
return ObjectModel.query()
184184
.select('object.id AS objectId', 'object.bucketId as bucketId')
185-
.allowGraph('[bucketPermission, objectPermission, version.metadata]')
185+
.allowGraph('[bucketPermission, objectPermission, objectIdpPermission, bucketIdpPermission, version.metadata]')
186186
.withGraphJoined('version.metadata')
187187
// get latest version that isn't a delete marker by default
188188
.modifyGraph('version', builder => {
@@ -206,14 +206,17 @@ const service = {
206206
// match on bucketIds parameter
207207
.modify('filterBucketIds', params.bucketIds)
208208
// scope to objects that user(s) has READ permission at object or bucket-level
209-
.modify('hasPermission', params.userId, 'READ')
209+
.modify('hasPermission', { userId: params.userId, idp: params.idp, permCode: 'READ' })
210210
// re-structure result like: [{ objectId: abc, metadata: [{ key: a, value: b }] }]
211-
.then(result => result.map(row => {
212-
return {
213-
objectId: row.objectId,
214-
metadata: row.version && row.version.length ? row.version[0].metadata : []
215-
};
216-
}));
211+
.then(result =>
212+
// de-dupe based on objectId, keeping all associated tags for each object
213+
Array.from(new Map(result.map(item => [item.objectId, item])).values())
214+
.map(row => {
215+
return {
216+
objectId: row.objectId,
217+
metadata: row.version && row.version.length ? row.version[0].metadata : []
218+
};
219+
}));
217220
},
218221

219222
/**
@@ -250,7 +253,11 @@ const service = {
250253
query
251254
.allowGraph('object')
252255
.withGraphJoined('object')
253-
.modifyGraph('object', query => { query.modify('hasPermission', params.userId, 'READ'); })
256+
.modifyGraph('object', query => {
257+
query.modify('hasPermission', {
258+
userId: params.userId, idp: params.idp, permCode: 'READ'
259+
});
260+
})
254261
.whereNotNull('object.id');
255262
}
256263
if (params.bucketId) {

‎app/src/services/object.js‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ const service = {
128128
// GroupBy() seems to be working faster with ObjectionJS Graphs
129129
// when comparing with distinct()
130130
response.data = await ObjectModel.query(trx)
131-
.allowGraph('[bucketPermission, objectPermission, version]')
131+
.allowGraph('[bucketPermission, objectPermission, objectIdpPermission, bucketIdpPermission, version]')
132132
.groupBy('object.id')
133133
// object
134134
.modify('filterIds', params.id)
@@ -149,9 +149,10 @@ const service = {
149149
// permissions
150150
// if userId is provided (privacyMask is ON) then filter where:
151151
// - user has READ on object
152-
// - user has READ on parent folder
153-
// - any parent folder.public is truthy
154-
.modify('hasPermission', params.userId, 'READ')
152+
// - OR user has READ on parent folder
153+
// - OR user has READ permission granted to their IDP
154+
// - OR any parent folder.public is truthy
155+
.modify('hasPermission', { userId: params.userId, idp: params.idp, permCode: 'READ' })
155156
// pagination
156157
.modify('pagination', params.page, params.limit)
157158
// sort results
@@ -168,8 +169,16 @@ const service = {
168169
}
169170
return Promise.all(
170171
results.map(row => {
171-
// eslint-disable-next-line no-unused-vars
172-
const { objectPermission, bucketPermission, version, ...object } = row;
172+
// destructure permissions out of the row for cleaner response formatting,
173+
// and filter down to only current user permissions if privacyMask is ON
174+
/* eslint-disable */
175+
const {
176+
objectPermission,
177+
bucketPermission,
178+
objectIdpPermission,
179+
bucketIdpPermission,
180+
version, ...object } = row;
181+
/* eslint-enable */
173182
if (row.id && params.permissions) {
174183
object.permissions = [];
175184
if (objectPermission && params.userId && params.userId !== SYSTEM_USER) {

‎app/src/services/tag.js‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ const service = {
233233
fetchTagsForObject: (params) => {
234234
return ObjectModel.query()
235235
.select('object.id AS objectId', 'object.bucketId as bucketId')
236-
.allowGraph('[bucketPermission, objectPermission, version.tag]')
236+
.allowGraph('[bucketPermission, objectPermission, objectIdpPermission, bucketIdpPermission, version.tag]')
237237
.withGraphJoined('version.tag')
238238
// get latest version that isn't a delete marker by default
239239
.modifyGraph('version', builder => {
@@ -257,14 +257,17 @@ const service = {
257257
// match on bucketIds parameter
258258
.modify('filterBucketIds', params.bucketIds)
259259
// scope to objects that user(s) has READ permission at object or bucket-level
260-
.modify('hasPermission', params.userId, 'READ')
261-
// re-structure result like: [{ objectId: abc, tagset: [{ key: a, value: b }] }]
262-
.then(result => result.map(row => {
263-
return {
264-
objectId: row.objectId,
265-
tagset: row.version && row.version.length ? row.version[0].tag : []
266-
};
267-
}));
260+
.modify('hasPermission', { userId: params.userId, idp: params.idp, permCode: 'READ' })
261+
.then(result =>
262+
// de-dupe based on objectId, keeping all associated tags for each object
263+
Array.from(new Map(result.map(item => [item.objectId, item])).values())
264+
// re-structure result like: [{ objectId: abc, tagset: [{ key: a, value: b }] }]
265+
.map(row => {
266+
return {
267+
objectId: row.objectId,
268+
tagset: row.version && row.version.length ? row.version[0].tag : []
269+
};
270+
}));
268271
},
269272

270273
/**
@@ -301,7 +304,11 @@ const service = {
301304
query
302305
.allowGraph('object')
303306
.withGraphJoined('object')
304-
.modifyGraph('object', query => { query.modify('hasPermission', params.userId, 'READ'); })
307+
.modifyGraph('object', query => {
308+
query.modify('hasPermission', {
309+
userId: params.userId, idp: params.idp, permCode: 'READ'
310+
});
311+
})
305312
.whereNotNull('object.id');
306313
}
307314
if (params.bucketId) {

‎app/tests/unit/services/metadata.spec.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,9 @@ describe('fetchMetadataForObject', () => {
185185
expect(ObjectModel.modifyGraph).toHaveBeenCalledWith('version.metadata', expect.anything());
186186
expect(ObjectModel.modify).toHaveBeenCalledTimes(3);
187187
expect(ObjectModel.modify).toHaveBeenCalledWith('filterIds', params.objId);
188-
expect(ObjectModel.modify).toHaveBeenCalledWith('hasPermission', params.userId, 'READ');
188+
expect(ObjectModel.modify).toHaveBeenCalledWith('hasPermission', {
189+
idp: undefined, permCode: 'READ', userId: '00000000-0000-0000-0000-000000000000'
190+
});
189191
expect(ObjectModel.then).toHaveBeenCalledTimes(1);
190192
});
191193
});

‎app/tests/unit/services/object.spec.js‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ describe('searchObjects', () => {
141141
expect(ObjectModel.query).toHaveBeenCalledTimes(1);
142142
expect(ObjectModel.query).toHaveBeenCalledWith(expect.anything());
143143
expect(ObjectModel.allowGraph).toHaveBeenCalledTimes(1);
144-
expect(ObjectModel.allowGraph).toHaveBeenCalledWith('[bucketPermission, objectPermission, version]');
144+
expect(ObjectModel.allowGraph)
145+
.toHaveBeenCalledWith('[bucketPermission, objectPermission, objectIdpPermission, bucketIdpPermission, version]');
145146
expect(ObjectModel.groupBy).toHaveBeenCalledTimes(1);
146147
expect(ObjectModel.modify).toHaveBeenCalledTimes(11);
147148
expect(ObjectModel.modify).toHaveBeenNthCalledWith(1, 'filterIds', params.id);
@@ -156,7 +157,9 @@ describe('searchObjects', () => {
156157
metadata: params.metadata,
157158
tag: params.tag
158159
});
159-
expect(ObjectModel.modify).toHaveBeenNthCalledWith(9, 'hasPermission', params.userId, 'READ');
160+
expect(ObjectModel.modify).toHaveBeenNthCalledWith(9, 'hasPermission', {
161+
idp: undefined, permCode: 'READ', userId: params.userId
162+
});
160163
expect(ObjectModel.modify).toHaveBeenNthCalledWith(10, 'pagination', params.page, params.limit);
161164
expect(ObjectModel.modify).toHaveBeenNthCalledWith(11, 'sortOrder', params.sort, params.order);
162165
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);
@@ -187,7 +190,8 @@ describe('searchObjects', () => {
187190
expect(ObjectModel.query).toHaveBeenCalledTimes(1);
188191
expect(ObjectModel.query).toHaveBeenCalledWith(expect.anything());
189192
expect(ObjectModel.allowGraph).toHaveBeenCalledTimes(1);
190-
expect(ObjectModel.allowGraph).toHaveBeenCalledWith('[bucketPermission, objectPermission, version]');
193+
expect(ObjectModel.allowGraph)
194+
.toHaveBeenCalledWith('[bucketPermission, objectPermission, objectIdpPermission, bucketIdpPermission, version]');
191195
expect(ObjectModel.groupBy).toHaveBeenCalledTimes(1);
192196
expect(ObjectModel.modify).toHaveBeenCalledTimes(11);
193197
expect(ObjectModel.modify).toHaveBeenNthCalledWith(1, 'filterIds', params.id);
@@ -203,7 +207,9 @@ describe('searchObjects', () => {
203207
metadata: params.metadata,
204208
tag: params.tag
205209
});
206-
expect(ObjectModel.modify).toHaveBeenNthCalledWith(9, 'hasPermission', params.userId, 'READ');
210+
expect(ObjectModel.modify).toHaveBeenNthCalledWith(9, 'hasPermission', {
211+
idp: undefined, permCode: 'READ', userId: params.userId
212+
});
207213
expect(ObjectModel.modify).toHaveBeenNthCalledWith(10, 'pagination', params.page, params.limit);
208214
expect(ObjectModel.modify).toHaveBeenNthCalledWith(11, 'sortOrder', params.sort, params.order);
209215
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);

0 commit comments

Comments
 (0)