Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from
Dec 19, 2022
Merged

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

merged 9 commits into from
Dec 19, 2022

Conversation

Tarush-Singh35
Copy link
Contributor

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

Describe the PR

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

Fixes

@Tarush-Singh35
Copy link
Contributor Author

@rsoesemann and @rbklaassen please review this

@rbklaassen
Copy link

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
Copy link
Member

@adangel adangel left a comment

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.

...pex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml Outdated Show resolved Hide resolved
pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java Show resolved Hide resolved
...pex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml Show resolved Hide resolved
@adangel adangel added this to the 6.53.0 milestone Dec 1, 2022
@Tarush-Singh35
Copy link
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
Copy link
Member

adangel commented Dec 1, 2022

@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
Copy link
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

@pmd-test
Copy link

pmd-test commented Dec 16, 2022 •
edited
Loading

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
5 participants
@Tarush-Singh35 @rbklaassen @adangel @pmd-test @rsoesemann