Skip to content

[apex] ApexCRUDViolation: user mode and system mode with test cases added#4244

Merged
adangel merged 9 commits into
pmd:masterfrom
Tarush-Singh35:#4146
Dec 19, 2022
Merged

[apex] ApexCRUDViolation: user mode and system mode with test cases added#4244
adangel merged 9 commits into
pmd:masterfrom
Tarush-Singh35:#4146

Conversation

@Tarush-Singh35

@Tarush-Singh35 Tarush-Singh35 commented Nov 29, 2022 •
edited by adangel
Loading

Copy link
Copy Markdown
Contributor

Describe the PR

Extending the Rules to handle the ApexCrudViolation using User Mode and System Mode

Fixes

@Tarush-Singh35

Tarush-Singh35 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

@rsoesemann and @rbklaassen please review this

@rbklaassen

rbklaassen commented Nov 29, 2022

Copy link
Copy Markdown

It looks pretty good @Tarush-Singh35. However, I can't tell from the code if it also covers the scenario where schema.isAccessible is used for the object/fieds. In that case it isn't needed to add a security mode to the SOQL query.

@adangel adangel changed the title user mode and System mode with test cases added [apex] ApexCRUDViolationRule: user mode and System mode with test cases added Dec 1, 2022
@adangel adangel linked an issue Dec 1, 2022 that may be closed by this pull request
[apex] ApexCRUDViolation: Recognize User Mode in SOQL + DML #4146
Closed
@adangel adangel changed the title [apex] ApexCRUDViolationRule: user mode and System mode with test cases added [apex] ApexCRUDViolation: user mode and system mode with test cases added Dec 1, 2022
adangel
adangel requested changes Dec 1, 2022
View reviewed changes

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Please have a look at my comments.

Comment thread ...pex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml Outdated
Comment thread pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
Comment thread ...pex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
@adangel adangel added this to the 6.53.0 milestone Dec 1, 2022
@Tarush-Singh35

Tarush-Singh35 commented Dec 1, 2022

Copy link
Copy Markdown
Contributor Author

It looks pretty good @Tarush-Singh35. However, I can't tell from the code if it also covers the scenario where schema.isAccessible is used for the object/fieds. In that case it isn't needed to add a security mode to the SOQL query.

@rbklaassen sorry I did not understand what you are trying to say can you elaborate this

@adangel

adangel commented Dec 1, 2022

Copy link
Copy Markdown
Member

@Tarush-Singh35 we have now a couple of test failures for some old tests. Can you look at them and decide, whether they are now wrong or whether the new rule changes do something wrong?

https://github.com/pmd/pmd/actions/runs/3592105214/jobs/6047623060#step:7:7763

  Run 16: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 17: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 18: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED in a List" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 19: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED in a List Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 20: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED with Case Insensitivity" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 21: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED with Case Insensitivity Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  [INFO]   Run 22: PASS
  Error:    Run 23: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED Secured" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 24: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED Secured Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  [INFO]   Run 25: PASS
  Error:    Run 26: ApexCRUDViolationTest>RuleTst.runTest:122 "User Mode" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 27: ApexCRUDViolationTest>RuleTst.runTest:124->RuleTst.assertMessages:166 "System Mode, gives warning because it ignores CRUD but explicitly" produced wrong message on violation number 1. expected:<[This CRUD statement uses explicit system mode]> but was:<[Validate CRUD permission before SOQL/DML operation]>

Run 71: ApexCRUDViolationTest>RuleTst.runTest:122 "Proper CRUD check in SOQL for-loop with security enforced" resulted in wrong number of failures, expected:<0> but was:<1>

@Tarush-Singh35

Tarush-Singh35 commented Dec 11, 2022

Copy link
Copy Markdown
Contributor Author

@Tarush-Singh35 we have now a couple of test failures for some old tests. Can you look at them and decide, whether they are now wrong or whether the new rule changes do something wrong?

https://github.com/pmd/pmd/actions/runs/3592105214/jobs/6047623060#step:7:7763

  Run 16: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 17: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 18: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED in a List" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 19: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED in a List Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 20: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED with Case Insensitivity" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 21: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED with Case Insensitivity Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  [INFO]   Run 22: PASS
  Error:    Run 23: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED Secured" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 24: ApexCRUDViolationTest>RuleTst.runTest:122 "Accepts Closure SECURITY ENFORCED Secured Line Break" resulted in wrong number of failures, expected:<0> but was:<1>
  [INFO]   Run 25: PASS
  Error:    Run 26: ApexCRUDViolationTest>RuleTst.runTest:122 "User Mode" resulted in wrong number of failures, expected:<0> but was:<1>
  Error:    Run 27: ApexCRUDViolationTest>RuleTst.runTest:124->RuleTst.assertMessages:166 "System Mode, gives warning because it ignores CRUD but explicitly" produced wrong message on violation number 1. expected:<[This CRUD statement uses explicit system mode]> but was:<[Validate CRUD permission before SOQL/DML operation]>

Run 71: ApexCRUDViolationTest>RuleTst.runTest:122 "Proper CRUD check in SOQL for-loop with security enforced" resulted in wrong number of failures, expected:<0> but was:<1>

hey, @adangel I had gone through the code but could not find any error because I did not make any changes to the security enforced. Any help would be appreciated

@ghost

ghost commented Dec 16, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 Compared to master:
This changeset changes 4 violations,
introduces 3 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 15 errors and 7 configuration errors.
Full report
✅ Compared to master:
This changeset changes 4 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 15 errors and 7 configuration errors.
Full report

Generated by 🚫 Danger

@adangel adangel mentioned this pull request Dec 16, 2022
Closed
adangel added a commit that referenced this pull request Dec 19, 2022
@adangel
[doc] Update release notes (#4146, #4244)
03db706
adangel added a commit that referenced this pull request Dec 19, 2022
@adangel
Merge pull request #4244 from Tarush-Singh35:#4146
a76603b 
[apex] ApexCRUDViolation: user mode and system mode with test cases added #4244
@adangel adangel merged commit 930b778 into pmd:master Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adangel adangel adangel requested changes

@rsoesemann rsoesemann rsoesemann left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.53.0

Development

Successfully merging this pull request may close these issues.

[apex] ApexCRUDViolation: Recognize User Mode in SOQL + DML

4 participants

@Tarush-Singh35 @rbklaassen @adangel @rsoesemann