From 0bf807fa9f5723a9e2f49cd60a370f52b20842ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Fri, 13 Jul 2018 11:47:26 +0100 Subject: [PATCH] Remove SubscriptionLocator.getManagedSubscription It was used as a kind of access control check, but it's clearer if the check is in the only controller that actually needs it. --- .../SubscriptionGroupController.coffee | 20 ++++++++++++++----- .../Subscription/SubscriptionLocator.coffee | 9 --------- .../SubscriptionGroupControllerTests.coffee | 2 +- .../SubscriptionLocatorTests.coffee | 2 +- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee index 54c5827a20..0fb7b6e7c9 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee @@ -13,7 +13,7 @@ module.exports = adminUserId = AuthenticationController.getLoggedInUserId(req) newEmail = req.body?.email?.toLowerCase()?.trim() - SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) -> + getManagedSubscription adminUserId, (error, subscription) -> return next(error) if error? logger.log adminUserId:adminUserId, newEmail:newEmail, "adding user to group subscription" @@ -31,7 +31,7 @@ module.exports = removeUserFromGroup: (req, res, next)-> adminUserId = AuthenticationController.getLoggedInUserId(req) userToRemove_id = req.params.user_id - SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) -> + getManagedSubscription adminUserId, (error, subscription) -> return next(error) if error? logger.log adminUserId:adminUserId, userToRemove_id:userToRemove_id, "removing user from group subscription" SubscriptionGroupHandler.removeUserFromGroup subscription._id, userToRemove_id, (err)-> @@ -43,7 +43,7 @@ module.exports = removeSelfFromGroup: (req, res, next)-> adminUserId = req.query.admin_user_id userToRemove_id = AuthenticationController.getLoggedInUserId(req) - SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) -> + getManagedSubscription adminUserId, (error, subscription) -> return next(error) if error? logger.log adminUserId:adminUserId, userToRemove_id:userToRemove_id, "removing user from group subscription after self request" SubscriptionGroupHandler.removeUserFromGroup subscription._id, userToRemove_id, (err)-> @@ -54,7 +54,7 @@ module.exports = renderSubscriptionGroupAdminPage: (req, res, next)-> user_id = AuthenticationController.getLoggedInUserId(req) - SubscriptionLocator.getManagedSubscription user_id, (error, subscription)-> + getManagedSubscription user_id, (error, subscription)-> return next(error) if error? if !subscription?.groupPlan return res.redirect("/user/subscription") @@ -67,7 +67,7 @@ module.exports = exportGroupCsv: (req, res)-> user_id = AuthenticationController.getLoggedInUserId(req) logger.log user_id: user_id, "exporting group csv" - SubscriptionLocator.getManagedSubscription user_id, (err, subscription)-> + getManagedSubscription user_id, (err, subscription)-> return next(error) if error? if !subscription.groupPlan return res.redirect("/") @@ -81,3 +81,13 @@ module.exports = ) res.contentType('text/csv') res.send(groupCsv) + + +getManagedSubscription = (managerId, callback) -> + SubscriptionLocator.findManagedSubscription managerId, (err, subscription)-> + if subscription? + logger.log managerId: managerId, "got managed subscription" + else + err ||= new Error("No subscription found managed by user #{managerId}") + + return callback(err, subscription) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee index d348e2e829..3a8d29673b 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee @@ -14,15 +14,6 @@ module.exports = SubscriptionLocator = logger.log user_id:user_id, "got users subscription" callback(err, subscription) - getManagedSubscription: (managerId, callback)-> - SubscriptionLocator.findManagedSubscription managerId, (err, subscription)-> - if subscription? - logger.log managerId: managerId, "got managed subscription" - else - err ||= new Error("No subscription found managed by user #{managerId}") - - callback(err, subscription) - findManagedSubscription: (managerId, callback)-> logger.log managerId: managerId, "finding managed subscription" Subscription.findOne manager_ids: managerId, callback diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee index 5d8cb397c0..a399496399 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee @@ -29,7 +29,7 @@ describe "SubscriptionGroupController", -> isUserPartOfGroup: sinon.stub() getPopulatedListOfMembers: sinon.stub().callsArgWith(1, null, [@user]) @SubscriptionLocator = - getManagedSubscription: sinon.stub().callsArgWith(1, null, @subscription) + findManagedSubscription: sinon.stub().callsArgWith(1, null, @subscription) @AuthenticationController = getLoggedInUserId: (req) -> req.session.user._id diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionLocatorTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionLocatorTests.coffee index 6543ac0a8a..52dee42e84 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionLocatorTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionLocatorTests.coffee @@ -45,7 +45,7 @@ describe "Subscription Locator Tests", -> it "should query the database", (done)-> @Subscription.findOne.callsArgWith(1, null, @subscription) - @SubscriptionLocator.getManagedSubscription @user._id, (err, subscription)=> + @SubscriptionLocator.findManagedSubscription @user._id, (err, subscription)=> @Subscription.findOne.calledWith({"manager_ids":@user._id}).should.equal true subscription.should.equal @subscription done()