From ae25fe128d2cfc492eac7cb4530c80d47ba1be77 Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Thu, 30 Mar 2023 18:54:05 +0300 Subject: [PATCH 1/6] add type in esClient.delete --- .circleci/config.yml | 1 + src/services/ChallengeService.js | 1 + 2 files changed, 2 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 841d3910..138dfe69 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -84,6 +84,7 @@ workflows: only: - dev - fix/template + - fix/PLAT-2584 - "build-qa": context: org-global diff --git a/src/services/ChallengeService.js b/src/services/ChallengeService.js index 2b544007..17446bbb 100644 --- a/src/services/ChallengeService.js +++ b/src/services/ChallengeService.js @@ -2297,6 +2297,7 @@ async function deleteChallenge(currentUser, challengeId) { await esClient.delete({ index: config.get("ES.ES_INDEX"), refresh: config.get("ES.ES_REFRESH"), + type: config.get("ES.OPENSEARCH") == "false" ? config.get("ES.ES_TYPE") : undefined, id: challengeId, }); await helper.postBusEvent(constants.Topics.ChallengeDeleted, { From e9b50a7d8d885447218be8e9b1c5c6430dec26ac Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Thu, 30 Mar 2023 19:00:02 +0300 Subject: [PATCH 2/6] update ci configs --- .circleci/config.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 138dfe69..841d3910 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -84,7 +84,6 @@ workflows: only: - dev - fix/template - - fix/PLAT-2584 - "build-qa": context: org-global From c5cd0215eabcdb9898a36f98c63686a730192b86 Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Thu, 30 Mar 2023 19:15:52 +0300 Subject: [PATCH 3/6] check if challenge exists before trying to delete --- src/services/ChallengeService.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/services/ChallengeService.js b/src/services/ChallengeService.js index 17446bbb..c92b810e 100644 --- a/src/services/ChallengeService.js +++ b/src/services/ChallengeService.js @@ -2282,6 +2282,9 @@ function sanitizeData(data, challenge) { */ async function deleteChallenge(currentUser, challengeId) { const challenge = await challengeDomain.lookup(getLookupCriteria("id", challengeId)); + if (!challenge) { + throw new errors.NotFoundError(`Challenge with id: ${challengeId} doesn't exist`); + } if (challenge.status !== constants.challengeStatuses.New) { throw new errors.BadRequestError( `Challenge with status other than "${constants.challengeStatuses.New}" cannot be removed` @@ -2292,7 +2295,10 @@ async function deleteChallenge(currentUser, challengeId) { // check if user are allowed to delete the challenge await ensureAccessibleForChallenge(currentUser, challenge); // delete DB record - await challengeDomain.delete(getLookupCriteria("id", challengeId)); + const { items } = await challengeDomain.delete(getLookupCriteria("id", challengeId)); + if (!_.find(items, { id: challengeId })) { + throw new errors.Internal(`There was an error deleting the challenge with id: ${challengeId} from dynamo`); + } // delete ES document await esClient.delete({ index: config.get("ES.ES_INDEX"), From 39428ecff7c9e30663d3c2e172f6b84ab9acda47 Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Thu, 30 Mar 2023 19:33:57 +0300 Subject: [PATCH 4/6] refactor delete --- src/services/ChallengeService.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/services/ChallengeService.js b/src/services/ChallengeService.js index c92b810e..ded5bf63 100644 --- a/src/services/ChallengeService.js +++ b/src/services/ChallengeService.js @@ -2281,22 +2281,20 @@ function sanitizeData(data, challenge) { * @returns {Object} the deleted challenge */ async function deleteChallenge(currentUser, challengeId) { - const challenge = await challengeDomain.lookup(getLookupCriteria("id", challengeId)); + const { items } = await challengeDomain.scan({ + criteria: getScanCriteria({ id: challengeId, status: constants.challengeStatuses.New }), + }); + const challenge = _.first(items); if (!challenge) { - throw new errors.NotFoundError(`Challenge with id: ${challengeId} doesn't exist`); - } - if (challenge.status !== constants.challengeStatuses.New) { - throw new errors.BadRequestError( - `Challenge with status other than "${constants.challengeStatuses.New}" cannot be removed` - ); + throw new errors.NotFoundError(`Challenge with id: ${challengeId} doesn't exist or is not in New status`); } // check groups authorization await ensureAccessibleByGroupsAccess(currentUser, challenge); // check if user are allowed to delete the challenge await ensureAccessibleForChallenge(currentUser, challenge); // delete DB record - const { items } = await challengeDomain.delete(getLookupCriteria("id", challengeId)); - if (!_.find(items, { id: challengeId })) { + const { items: deletedItems } = await challengeDomain.delete(getLookupCriteria("id", challengeId)); + if (!_.find(deletedItems, { id: challengeId })) { throw new errors.Internal(`There was an error deleting the challenge with id: ${challengeId} from dynamo`); } // delete ES document From 131223357752572e607ee7e022c9df11e687b99f Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Thu, 30 Mar 2023 20:16:21 +0300 Subject: [PATCH 5/6] clean up access checks --- src/services/ChallengeService.js | 74 +------------------------------- 1 file changed, 2 insertions(+), 72 deletions(-) diff --git a/src/services/ChallengeService.js b/src/services/ChallengeService.js index ded5bf63..79dd39ff 100644 --- a/src/services/ChallengeService.js +++ b/src/services/ChallengeService.js @@ -67,74 +67,6 @@ async function ensureAccessibleForChallenge(user, challenge) { } } -/** - * Filter challenges by groups access - * @param {Object} currentUser the user who perform operation - * @param {Array} challenges the challenges to filter - * @returns {Array} the challenges that can be accessed by current user - */ -async function filterChallengesByGroupsAccess(currentUser, challenges) { - const res = []; - let userGroups; - const needToCheckForGroupAccess = !currentUser - ? true - : !currentUser.isMachine && !hasAdminRole(currentUser); - const subGroupsMap = {}; - for (const challenge of challenges) { - challenge.groups = _.filter( - challenge.groups, - (g) => !_.includes(["null", "undefined"], _.toString(g).toLowerCase()) - ); - let expandedGroups = []; - if ( - !challenge.groups || - _.get(challenge, "groups.length", 0) === 0 || - !needToCheckForGroupAccess - ) { - res.push(challenge); - } else if (currentUser) { - // get user groups if not yet - if (_.isNil(userGroups)) { - userGroups = await helper.getUserGroups(currentUser.userId); - } - // Expand challenge groups by subGroups - // results are being saved on a hashmap for efficiency - for (const group of challenge.groups) { - let subGroups; - if (subGroupsMap[group]) { - subGroups = subGroupsMap[group]; - } else { - subGroups = await helper.expandWithSubGroups(group); - subGroupsMap[group] = subGroups; - } - expandedGroups = [..._.concat(expandedGroups, subGroups)]; - } - // check if there is matched group - // logger.debug('Groups', challenge.groups, userGroups) - if (_.find(expandedGroups, (group) => !!_.find(userGroups, (ug) => ug.id === group))) { - res.push(challenge); - } - } - } - return res; -} - -/** - * Ensure the user can access the challenge by groups access - * @param {Object} currentUser the user who perform operation - * @param {Object} challenge the challenge to check - */ -async function ensureAccessibleByGroupsAccess(currentUser, challenge) { - const filtered = await filterChallengesByGroupsAccess(currentUser, [challenge]); - if (filtered.length === 0) { - throw new errors.ForbiddenError(`ensureAccessibleByGroupsAccess :: You don't have access to this group! - Current User: ${JSON.stringify(currentUser)} - Challenge: ${JSON.stringify(challenge)} - Filtered: ${JSON.stringify(filtered)} - `); - } -} - /** * Search challenges by legacyId * @param {Object} currentUser the user who perform operation @@ -2288,10 +2220,8 @@ async function deleteChallenge(currentUser, challengeId) { if (!challenge) { throw new errors.NotFoundError(`Challenge with id: ${challengeId} doesn't exist or is not in New status`); } - // check groups authorization - await ensureAccessibleByGroupsAccess(currentUser, challenge); - // check if user are allowed to delete the challenge - await ensureAccessibleForChallenge(currentUser, challenge); + // ensure user can modify challenge + await helper.ensureUserCanModifyChallenge(currentUser, challenge); // delete DB record const { items: deletedItems } = await challengeDomain.delete(getLookupCriteria("id", challengeId)); if (!_.find(deletedItems, { id: challengeId })) { From e70f51333b45b7f24683c9deba1f0e3a3dd0c397 Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Mon, 3 Apr 2023 13:46:10 +0300 Subject: [PATCH 6/6] change error message in delete challenge --- src/services/ChallengeService.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/ChallengeService.js b/src/services/ChallengeService.js index 79dd39ff..bd94144c 100644 --- a/src/services/ChallengeService.js +++ b/src/services/ChallengeService.js @@ -2225,7 +2225,7 @@ async function deleteChallenge(currentUser, challengeId) { // delete DB record const { items: deletedItems } = await challengeDomain.delete(getLookupCriteria("id", challengeId)); if (!_.find(deletedItems, { id: challengeId })) { - throw new errors.Internal(`There was an error deleting the challenge with id: ${challengeId} from dynamo`); + throw new errors.Internal(`There was an error deleting the challenge with id: ${challengeId}`); } // delete ES document await esClient.delete({