Skip to content
This repository was archived by the owner on Nov 16, 2023. It is now read-only.

Commit fc2afc1

Browse files
yiyioneyiyione
authored
Update the permission check of add/delete item's categories and tags (#312)
* update the permission check of add/delete item's tag & category * update the permission check of add/delete item's tag & category * update * update * use Array.prototype.some() instead of ...map().includes() * move all OFFICIAL_EXAMPLE to a same place * fix * merge checkReadPermission/checkWritePermission
1 parent 2599041 commit fc2afc1

File tree

4 files changed

+116
-77
lines changed

4 files changed

+116
-77
lines changed

‎rest_server/src/controllers/item_controller.js‎

Lines changed: 46 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,35 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33
const { isNil } = require('lodash');
4-
const { MarketplaceItem } = require('../models');
4+
const { MarketplaceItem, ItemCategory } = require('../models');
55
const asyncHandler = require('./async_handler');
66
const yaml = require('js-yaml');
77
const protocolValidator = require('../utils/protocol');
88
const error = require('../models/error');
99

10-
function checkReadPermission(userInfo, item) {
11-
if (userInfo.admin === true) {
10+
async function checkWritePermission(tokenInfo, item, categories) {
11+
if (tokenInfo.admin === true) {
1212
return true;
1313
}
14-
if (userInfo.username === item.author) {
14+
if (categories === undefined) {
15+
categories = await MarketplaceItem.getCategories(item);
16+
}
17+
if (
18+
!categories.some(
19+
category => category.name === ItemCategory.OFFICIAL_EXAMPLE,
20+
) &&
21+
tokenInfo.username === item.author
22+
) {
1523
return true;
1624
}
17-
if (item.isPublic) {
25+
return false;
26+
}
27+
28+
async function checkReadPermission(userInfo, item, categories) {
29+
if (
30+
item.isPublic ||
31+
(await checkWritePermission(userInfo, item, categories))
32+
) {
1833
return true;
1934
}
2035
if (!item.isPrivate && (userInfo.grouplist && item.groupList)) {
@@ -27,16 +42,6 @@ function checkReadPermission(userInfo, item) {
2742
return false;
2843
}
2944

30-
function checkWritePermission(tokenInfo, item) {
31-
if (tokenInfo.admin === true) {
32-
return true;
33-
}
34-
if (tokenInfo.username === item.author) {
35-
return true;
36-
}
37-
return false;
38-
}
39-
4045
const list = asyncHandler(async (req, res, next) => {
4146
try {
4247
const result = await MarketplaceItem.list(
@@ -96,7 +101,7 @@ const get = asyncHandler(async (req, res, next) => {
96101
if (isNil(result)) {
97102
return next(error.createNotFound());
98103
} else {
99-
if (checkReadPermission(req.userInfo, result)) {
104+
if (await checkReadPermission(req.userInfo, result)) {
100105
res.status(200).json(result);
101106
} else {
102107
return next(
@@ -114,7 +119,7 @@ const get = asyncHandler(async (req, res, next) => {
114119
const update = asyncHandler(async (req, res, next) => {
115120
try {
116121
let result = await MarketplaceItem.get(req.params.itemId);
117-
if (checkWritePermission(req.tokenInfo, result)) {
122+
if (await checkWritePermission(req.tokenInfo, result)) {
118123
result = await MarketplaceItem.update(req.params.itemId, req.body);
119124
if (isNil(result)) {
120125
return next(error.createNotFound());
@@ -136,7 +141,7 @@ const update = asyncHandler(async (req, res, next) => {
136141
const del = asyncHandler(async (req, res, next) => {
137142
try {
138143
let result = await MarketplaceItem.get(req.params.itemId);
139-
if (checkWritePermission(req.tokenInfo, result)) {
144+
if (await checkWritePermission(req.tokenInfo, result)) {
140145
result = await MarketplaceItem.del(req.params.itemId);
141146
if (isNil(result)) {
142147
return next(error.createNotFound());
@@ -158,7 +163,7 @@ const del = asyncHandler(async (req, res, next) => {
158163
const listTags = asyncHandler(async (req, res, next) => {
159164
try {
160165
let result = await MarketplaceItem.get(req.params.itemId);
161-
if (checkReadPermission(req.userInfo, result)) {
166+
if (await checkReadPermission(req.userInfo, result)) {
162167
result = await MarketplaceItem.getTags(result);
163168
if (isNil(result)) {
164169
return next(error.createNotFound());
@@ -180,7 +185,7 @@ const listTags = asyncHandler(async (req, res, next) => {
180185
const addTag = asyncHandler(async (req, res, next) => {
181186
try {
182187
let result = await MarketplaceItem.get(req.params.itemId);
183-
if (checkReadPermission(req.userInfo, result)) {
188+
if (await checkWritePermission(req.tokenInfo, result)) {
184189
result = await MarketplaceItem.addTag(result, req.params.tagId);
185190
if (isNil(result)) {
186191
return next(error.createNotFound());
@@ -202,7 +207,7 @@ const addTag = asyncHandler(async (req, res, next) => {
202207
const deleteTag = asyncHandler(async (req, res, next) => {
203208
try {
204209
let result = await MarketplaceItem.get(req.params.itemId);
205-
if (checkReadPermission(req.userInfo, result)) {
210+
if (await checkWritePermission(req.tokenInfo, result)) {
206211
result = await MarketplaceItem.deleteTag(result, req.params.tagId);
207212
if (isNil(result)) {
208213
return next(error.createNotFound());
@@ -223,14 +228,13 @@ const deleteTag = asyncHandler(async (req, res, next) => {
223228

224229
const listCategories = asyncHandler(async (req, res, next) => {
225230
try {
226-
let result = await MarketplaceItem.get(req.params.itemId);
227-
if (checkReadPermission(req.userInfo, result)) {
228-
result = await MarketplaceItem.getCategories(result);
229-
if (isNil(result)) {
230-
return next(error.createNotFound());
231-
} else {
232-
res.status(200).json(result);
233-
}
231+
const result = await MarketplaceItem.get(req.params.itemId);
232+
const categories = await MarketplaceItem.getCategories(result);
233+
if (isNil(result)) {
234+
return next(error.createNotFound());
235+
}
236+
if (await checkReadPermission(req.userInfo, result, categories)) {
237+
res.status(200).json(categories);
234238
} else {
235239
return next(
236240
error.createForbidden(
@@ -246,7 +250,18 @@ const listCategories = asyncHandler(async (req, res, next) => {
246250
const addCategory = asyncHandler(async (req, res, next) => {
247251
try {
248252
let result = await MarketplaceItem.get(req.params.itemId);
249-
if (checkReadPermission(req.userInfo, result)) {
253+
if (await checkWritePermission(req.tokenInfo, result)) {
254+
const category = await ItemCategory.get(req.params.categoryId);
255+
if (
256+
category.name === ItemCategory.OFFICIAL_EXAMPLE &&
257+
req.tokenInfo.admin !== true
258+
) {
259+
return next(
260+
error.createForbidden(
261+
`Only admin can set "${ItemCategory.OFFICIAL_EXAMPLE}" category.`,
262+
),
263+
);
264+
}
250265
result = await MarketplaceItem.addCategory(result, req.params.categoryId);
251266
if (isNil(result)) {
252267
return next(error.createNotFound());
@@ -268,7 +283,7 @@ const addCategory = asyncHandler(async (req, res, next) => {
268283
const deleteCategory = asyncHandler(async (req, res, next) => {
269284
try {
270285
let result = await MarketplaceItem.get(req.params.itemId);
271-
if (checkReadPermission(req.userInfo, result)) {
286+
if (await checkWritePermission(req.tokenInfo, result)) {
272287
result = await MarketplaceItem.deleteCategory(
273288
result,
274289
req.params.categoryId,

‎rest_server/src/models/item_category.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ const modelSyncHandler = require('./model_init_handler');
55

66
class ItemCategory {
77
constructor(sequelize, DataTypes) {
8+
this.OFFICIAL_EXAMPLE = 'official example';
9+
810
this.orm = sequelize.define('ItemCategory', {
911
id: {
1012
type: DataTypes.STRING,

‎rest_server/src/models/marketplace_item.js‎

Lines changed: 64 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
const { isNil, toLower } = require('lodash');
44
const { Op, fn, col, where, cast } = require('sequelize');
55
const modelSyncHandler = require('./model_init_handler');
6+
const ItemCategory = require('./item_category');
67

78
class MarketplaceItem {
89
constructor(sequelize, DataTypes) {
@@ -123,44 +124,6 @@ class MarketplaceItem {
123124
},
124125
];
125126
}
126-
if (!userInfo.admin) {
127-
filterStatement[Op.or] = [
128-
{
129-
isPublic: {
130-
[Op.eq]: true,
131-
},
132-
},
133-
{
134-
[Op.and]: [
135-
{
136-
isPrivate: {
137-
[Op.eq]: true,
138-
},
139-
},
140-
{
141-
author: {
142-
[Op.eq]: userInfo.username,
143-
},
144-
},
145-
],
146-
},
147-
{
148-
[Op.and]: [
149-
{
150-
isPublic: {
151-
[Op.eq]: false,
152-
},
153-
isPrivate: {
154-
[Op.eq]: false,
155-
},
156-
groupList: {
157-
[Op.overlap]: userInfo.groupList,
158-
},
159-
},
160-
],
161-
},
162-
];
163-
}
164127

165128
const havings = [];
166129
const havingArrayAgg = (queryParameter, colName) => {
@@ -177,6 +140,65 @@ class MarketplaceItem {
177140
havingArrayAgg(tags, 'ItemTags.name');
178141
havingArrayAgg(categories, 'ItemCategories.name');
179142

143+
if (!userInfo.admin) {
144+
if (filterStatement[Op.and] === undefined) {
145+
filterStatement[Op.and] = [];
146+
}
147+
filterStatement[Op.and].push({
148+
[Op.or]: [
149+
{
150+
isPublic: {
151+
[Op.eq]: true,
152+
},
153+
},
154+
{
155+
[Op.and]: [
156+
{
157+
isPrivate: {
158+
[Op.eq]: true,
159+
},
160+
},
161+
{
162+
author: {
163+
[Op.eq]: userInfo.username,
164+
},
165+
},
166+
],
167+
},
168+
{
169+
[Op.and]: [
170+
{
171+
isPublic: {
172+
[Op.eq]: false,
173+
},
174+
isPrivate: {
175+
[Op.eq]: false,
176+
},
177+
groupList: {
178+
[Op.overlap]: userInfo.groupList,
179+
},
180+
},
181+
],
182+
},
183+
],
184+
});
185+
186+
havings.push({
187+
[Op.not]: {
188+
[Op.and]: [
189+
{
190+
isPrivate: true,
191+
},
192+
where(
193+
fn('array_agg', col('ItemCategories.name')),
194+
Op.contains,
195+
cast([ItemCategory.OFFICIAL_EXAMPLE], 'varchar[]'),
196+
),
197+
],
198+
},
199+
});
200+
}
201+
180202
const items = await this.orm.findAll({
181203
where: filterStatement,
182204
having: havings,
@@ -373,17 +395,17 @@ class MarketplaceItem {
373395
return await handler(item, this.models);
374396
}
375397

376-
async addCategory(item, tagId) {
398+
async addCategory(item, categoryId) {
377399
const handler = modelSyncHandler(async item => {
378-
return await item.addItemCategory(tagId);
400+
return await item.addItemCategory(categoryId);
379401
});
380402

381403
return await handler(item, this.models);
382404
}
383405

384-
async deleteCategory(item, tagId) {
406+
async deleteCategory(item, categoryId) {
385407
const handler = modelSyncHandler(async item => {
386-
return await item.removeItemCategory(tagId);
408+
return await item.removeItemCategory(categoryId);
387409
});
388410

389411
return await handler(item, this.models);

‎rest_server/src/router.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,17 @@ router
2626

2727
router
2828
.route('/items/:itemId/tag/:tagId')
29-
.post(token.checkAuthAndGetUserInfo, itemController.addTag)
30-
.delete(token.checkAuthAndGetUserInfo, itemController.deleteTag);
29+
.post(token.checkAuthAndGetTokenInfo, itemController.addTag)
30+
.delete(token.checkAuthAndGetTokenInfo, itemController.deleteTag);
3131

3232
router
3333
.route('/items/:itemId/category')
3434
.get(token.checkAuthAndGetUserInfo, itemController.listCategories);
3535

3636
router
3737
.route('/items/:itemId/category/:categoryId')
38-
.post(token.checkAuthAndGetUserInfo, itemController.addCategory)
39-
.delete(token.checkAuthAndGetUserInfo, itemController.deleteCategory);
38+
.post(token.checkAuthAndGetTokenInfo, itemController.addCategory)
39+
.delete(token.checkAuthAndGetTokenInfo, itemController.deleteCategory);
4040

4141
router
4242
.route('/tags')

0 commit comments

Comments
 (0)