Fix dwc access control #11278

AlanWierzchonCA · 2022-06-22T15:05:30Z

Q	A
Bug fix? (use the a.b branch)	[Yes]
New feature/enhancement? (use the a.x branch)	[No]
Deprecations?	[No]
BC breaks? (use the c.x branch)	[No]
Automated tests included?	[Yes]
Related user documentation PR URL	mautic/mautic-documentation#...
Related developer documentation PR URL	mautic/developer-documentation#...
Issue(s) addressed	Fixes #...

Description:

User without privileges can create and delete ’Dynamic Content’

Steps to test this PR:

Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

Create new Role with only Dynamic Content Permissions:
Dynamic Content - User has access to View Own and View Others
Create new User with created Role
Login with created User
request GET https://8080-alanwierzchonca-mautic-h31qcblosf3.ws-eu47.gitpod.io/s/dwc/new
request POST https://8080-alanwierzchonca-mautic-h31qcblosf3.ws-eu47.gitpod.io/s/dwc/delete/1

RCheesley

Thanks for the PR @AlanWierzchonCA - please could you reach out to me on Slack at https://mautic.org/slack as I need to chat with you about this issue.

Works as expected, after applying the PR I am correctly getting a 403 if I try to hit the create new / delete.

Please also see the PHPSTAN issues which need some attention :)

codecov · 2022-06-28T16:41:49Z

Codecov Report

Merging #11278 (e1f941d) into 4.4 (acf3393) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                4.4   #11278      +/-   ##
============================================
+ Coverage     49.37%   49.54%   +0.17%     
  Complexity    35401    35401              
============================================
  Files          2144     2144              
  Lines        105532   105533       +1     
============================================
+ Hits          52102    52283     +181     
+ Misses        53430    53250     -180

Impacted Files	Coverage Δ
...tentBundle/Controller/DynamicContentController.php	`28.35% <100.00%> (+28.35%)`	⬆️
...ndles/CampaignBundle/Entity/CampaignRepository.php	`62.32% <0.00%> (+0.70%)`	⬆️
app/bundles/EmailBundle/Entity/EmailRepository.php	`69.25% <0.00%> (+0.74%)`	⬆️
...undle/Security/Permissions/AbstractPermissions.php	`50.00% <0.00%> (+2.50%)`	⬆️
app/bundles/AssetBundle/Entity/AssetRepository.php	`25.97% <0.00%> (+2.59%)`	⬆️
...les/DynamicContentBundle/Entity/DynamicContent.php	`50.95% <0.00%> (+6.36%)`	⬆️
...amicContentBundle/Form/Type/DynamicContentType.php	`92.92% <0.00%> (+16.16%)`	⬆️
...DynamicContentBundle/Model/DynamicContentModel.php	`21.31% <0.00%> (+18.03%)`	⬆️
...cContentBundle/Entity/DynamicContentRepository.php	`29.67% <0.00%> (+27.47%)`	⬆️
... and 2 more

escopecz

Thanks for this! Please check my comments if it makes sense. I also suggested the fixes for PHPSTAN issues.

escopecz · 2022-06-29T08:36:35Z

app/bundles/DynamicContentBundle/Controller/DynamicContentController.php

+            if (!$this->get('mautic.security')->isGranted('dynamiccontent:dynamiccontents:deleteown')
+                && !$this->get('mautic.security')->isGranted('dynamiccontent:dynamiccontents:deleteother')) {


I'm wondering how the code can know whether the entity can be deleted based on "own" or "other" permissions when the security service doesn't know who created the entity. Shouldn't we use the hasEntityAccess method instead?

Isn't that exact check slightly bellow this one on line 493?

Hi @escopecz,
on lines 482, 483 the code checks if the logged in user has general permission to delete their own or other dynamic content.
If not, the user should not know if the entity exists or not. It should get 403, not 401.

In line 493 there is a checks base on existting entity Id.

Is it necessary to check it twice? It seems like a code duplication to me.

Ok, so how do we check if we can return 404 based on validation with the hasEntityAccess at line 493?

It should get 403, not 401.

I mean "...not 404" (notfound).

Here is an example how "not found" is implemented for landing pages:

https://github.com/mautic/mautic/blob/5.x/app/bundles/PageBundle/Controller/PageController.php#L496-L507

But this example does not check whether the user has permission to find out whether the entity exists or not.

No entity in Mautic checks whether the user has permission to find out whether the entity exists or not. I wouldn't over-complicated this :)

Yes, secuirity issues are sometimes overcomplicated and the Department of Security gives many absurd points.
As you wish, I have removed this access checking :)

app/bundles/DynamicContentBundle/Tests/Controller/DynamicContentControllerFunctionalTest.php

AlanWierzchonCA · 2022-08-29T09:18:45Z

Hi @escopecz, @RCheesley
thanks for review and tests.

I just made fixes and answered questions.

escopecz

Amazing! The code is good to go and the tests are amazing! Thank you for taking care of this and be patient with me. 👍

One last thing so this can be tested and merged. Can you please rebase it on top of 5.x branch and if you want to back-port it to Mautic 4 as well then create a duplicate PR against the 4.4 branch?

AlanWierzchonCA · 2022-09-16T12:40:47Z

Hi @escopecz,

I made rebase these changes to 4.4
and created a duplicate PR against the 5.x #11480

cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Jun 22, 2022

AlanWierzchonCA marked this pull request as ready for review June 23, 2022 13:50

RCheesley added roles Anything related to users and roles dynamic-content bug Issues or PR's relating to bugs labels Jun 28, 2022

RCheesley previously approved these changes Jun 28, 2022

View reviewed changes

escopecz requested changes Jun 29, 2022

View reviewed changes

escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Jun 29, 2022

AlanWierzchonCA dismissed RCheesley’s stale review via 83a25da August 24, 2022 14:37

AlanWierzchonCA force-pushed the fix_dwc_access_control branch from 83a25da to f992a95 Compare August 29, 2022 08:48

escopecz added needs-rebase PR's that need to be rebased and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Sep 15, 2022

escopecz approved these changes Sep 15, 2022

View reviewed changes

escopecz added the pending-test-confirmation PR's that require one test before they can be merged label Sep 15, 2022

fix of permmission validation and functional tests

e1f941d

AlanWierzchonCA changed the base branch from 4.x to 4.4 September 16, 2022 12:11

AlanWierzchonCA force-pushed the fix_dwc_access_control branch from f05adbc to e1f941d Compare September 16, 2022 12:13

AlanWierzchonCA mentioned this pull request Sep 16, 2022

Fix of permmission validation and functional tests PR for 5.x #11480

Merged

escopecz removed the needs-rebase PR's that need to be rebased label Sep 16, 2022

RCheesley mentioned this pull request Sep 23, 2022

Open Source Friday 23/09/2022 #11501

Closed

21 tasks

RCheesley added this to the 4.4.3 milestone Sep 24, 2022

RCheesley merged commit 04d9b48 into mautic:4.4 Sep 24, 2022

escopecz added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Sep 27, 2022

RCheesley mentioned this pull request Sep 30, 2022

Open Source Friday 30/09/2022 #11531

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dwc access control #11278

Fix dwc access control #11278

AlanWierzchonCA commented Jun 22, 2022 •

edited

RCheesley left a comment •

edited

codecov bot commented Jun 28, 2022 •

edited

escopecz left a comment

escopecz Jun 29, 2022

AlanWierzchonCA Aug 29, 2022

escopecz Aug 29, 2022

AlanWierzchonCA Aug 29, 2022 •

edited

AlanWierzchonCA Aug 29, 2022 •

edited

escopecz Aug 30, 2022

AlanWierzchonCA Aug 30, 2022

escopecz Aug 30, 2022

AlanWierzchonCA Sep 15, 2022

AlanWierzchonCA commented Aug 29, 2022

escopecz left a comment

AlanWierzchonCA commented Sep 16, 2022 •

edited

		if (!$this->get('mautic.security')->isGranted('dynamiccontent:dynamiccontents:deleteown')
		&& !$this->get('mautic.security')->isGranted('dynamiccontent:dynamiccontents:deleteother')) {

Fix dwc access control #11278

Fix dwc access control #11278

Conversation

AlanWierzchonCA commented Jun 22, 2022 • edited

Description:

Steps to test this PR:

RCheesley left a comment • edited

Choose a reason for hiding this comment

codecov bot commented Jun 28, 2022 • edited

Codecov Report

escopecz left a comment

Choose a reason for hiding this comment

escopecz Jun 29, 2022

Choose a reason for hiding this comment

AlanWierzchonCA Aug 29, 2022

Choose a reason for hiding this comment

escopecz Aug 29, 2022

Choose a reason for hiding this comment

AlanWierzchonCA Aug 29, 2022 • edited

Choose a reason for hiding this comment

AlanWierzchonCA Aug 29, 2022 • edited

Choose a reason for hiding this comment

escopecz Aug 30, 2022

Choose a reason for hiding this comment

AlanWierzchonCA Aug 30, 2022

Choose a reason for hiding this comment

escopecz Aug 30, 2022

Choose a reason for hiding this comment

AlanWierzchonCA Sep 15, 2022

Choose a reason for hiding this comment

AlanWierzchonCA commented Aug 29, 2022

escopecz left a comment

Choose a reason for hiding this comment

AlanWierzchonCA commented Sep 16, 2022 • edited

AlanWierzchonCA commented Jun 22, 2022 •

edited

RCheesley left a comment •

edited

codecov bot commented Jun 28, 2022 •

edited

AlanWierzchonCA Aug 29, 2022 •

edited

AlanWierzchonCA Aug 29, 2022 •

edited

AlanWierzchonCA commented Sep 16, 2022 •

edited