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.

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

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

Closed
rsoesemann opened this issue Oct 7, 2022 · 24 comments · Fixed by #4244
Closed

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

rsoesemann opened this issue Oct 7, 2022 · 24 comments · Fixed by #4244
Assignees
Labels
a:false-negative PMD doesn't flag a problematic piece of code an:enhancement An improvement on existing features / rules
Milestone

Comments

@rsoesemann
Copy link
Member

rsoesemann commented Oct 7, 2022

Affects PMD Version:

Rule: ApexCRUDViolation

Description:
With the upcoming Winter '23 (API Version 56) Salesforce is going to add more native capabilities to enforce CRUD and FLS security in SOQL queries and DML statements as described here https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_classes_enforce_usermode.htm.

Currently, the usage of such new features is ignored by PMD and marked as a CRUD Violation. Therefor the rule needs to be extended to recognize and handle them correctly.

Code Sample demonstrating the issue:

public class UserMode {
	public void coverAllCasesWithTest() {
                // SOQL Queries cases
                
                Contact c;
                // Should be flagged a critical issue
                c = [SELECT Name FROM Contact];
                // Should be ignored
		c = [SELECT Name FROM Contact WITH USER_MODE];
                // Should be at best a warning because it ignores CRUD but explicitly
		c = [SELECT Name FROM Contact WITH SYSTEM_MODE];

                // DML cases

                // Should be flagged a critical issue
               insert contact;

                // Should be ignored
               insert as user contact;

                // Should be at best a warning because it ignores CRUD but explicitly
		insert as system contact;

               // ...and for ALL other occurrences of System.AccessLevel
	}
}

This issue should cover all cases with the optional accessLevel parameter. See Dynamic SOQL.
Database.getQueryLocator methods
Search.query methods
Database DML methods (insert, update, upsert, merge, delete, undelete, and convertLead)
Includes the *Immediate and *Async methods, such as insertImmediate and deleteAsync.

This issuers should be easy to contribute as we can just look at how the related earlier enhancements were done: #2210

@rsoesemann rsoesemann added a:false-negative PMD doesn't flag a problematic piece of code an:enhancement An improvement on existing features / rules labels Oct 7, 2022
@rsoesemann rsoesemann changed the title [apex] ApexCRUDViolation: Support SOQL and DML User Mode (GA in Winter '23) [apex] ApexCRUDViolation: Recognize User Mode in SOQL + DML Oct 7, 2022
@Tarush-Singh35
Copy link
Contributor

@rsoesemann I would like to contribute to this issue can you assign this issue to me?

@rsoesemann
Copy link
Member Author

rsoesemann commented Oct 21, 2022

@adangel was faster ;-)

Welcome @Tarush-Singh35. Thanks for your willingness to contribute to this very relevant change request.
Maybe you can join force with @rbklaassen from within Salesforce who also wanted to take this.

@Tarush-Singh35
Copy link
Contributor

yeah sure, I am getting started @rsoesemann we can work something out together. how can we connect @rbklaassen please let me know

@Tarush-Singh35
Copy link
Contributor

@rbklaassen I am getting started with the issue if you want to join do ping me.

Thanks & Regards

@rbklaassen
Copy link

Hi @Tarush-Singh35 , go ahead and start on this. I've been quite busy last week and preparing a go-live at customer this week, so I don't think I can help out this or next week. I'll check in after that!

@rbklaassen
Copy link

Also, how do you prefer to connect @Tarush-Singh35 ?

@Tarush-Singh35
Copy link
Contributor

@rbklaassen through slack would be great

@rbklaassen
Copy link

I'm on slack with rklaassen@salesforce.com @Tarush-Singh35

@Tarush-Singh35
Copy link
Contributor

I'm on slack with rklaassen@salesforce.com @Tarush-Singh35

yeah will connect with you through slack

@rsoesemann
Copy link
Member Author

Any progress here,guys?

@rbklaassen
Copy link

Sorry, not yet. Have had COVID and a pretty busy schedule at customer.

@Tarush-Singh35
Copy link
Contributor

Tarush-Singh35 commented Nov 10, 2022

I made some progress but I had an operation so I was on rest I will raise the PR soon also @rbklaassen sorry for not connecting with you

@adangel adangel added this to the 6.53.0 milestone Nov 17, 2022
@Tarush-Singh35
Copy link
Contributor

@rbklaassen you ready to contribute?

@rbklaassen
Copy link

Yes, I can do some work on this tomorrow. Do you want to collaborate tomorrow @Tarush-Singh35 ?

@Tarush-Singh35
Copy link
Contributor

Yeah @rbklaassen lets do this

@rbklaassen
Copy link

You need to connect with me through slack, I don't have enough details to connect to you @Tarush-Singh35

@Tarush-Singh35
Copy link
Contributor

You need to connect with me through slack, I don't have enough details to connect to you @Tarush-Singh35

I will create a workspace and I will ask you to join it

@Tarush-Singh35
Copy link
Contributor

Added you @rbklaassen in Slack

@rsoesemann
Copy link
Member Author

@rbklaassen and @Tarush-Singh35 any progress here? Or do you have any blockers I can help with?

@rbklaassen
Copy link

Hi @rsoesemann, today we had a meeting together and discussed the way forward. I came to the conclusion that I miss the Java knowledge to really work on the code, but I can be the support-desk for @Tarush-Singh35 as I know Apex very good. Also I will be able to test his solution when needed on a local project for instance.

@Tarush-Singh35
Copy link
Contributor

Yeah I am formulating a solution I think so I will be able to make a pull request soon @rsoesemann and @rbklaassen are mentoring me in the apex

@Tarush-Singh35
Copy link
Contributor

@rsoesemann @rbklaassen i need help to understand the issue in my PR can you guys help me out

@rsoesemann
Copy link
Member Author

@Tarush-Singh35 I see two comments from @adangel which both make sense to me:

  1. Explicit SYSTEM Mode should not detect an Issue. It's ok bahviour
  2. Your code https://github.com/pmd/pmd/pull/4244/files#diff-32e0573a4d25115d3334e8f69cd4db0628e035eaa3831a90fc5f00a0e4e272f1R668 changes the logic also for other cases that relate to SECURITY ENFORCED. That's most probably the reason why existing test fail. Please run all tests before you submit a PR.

@adangel
Copy link
Member

adangel commented Dec 16, 2022

@rsoesemann Be aware, that the rule currently doesn't support query: see #2628 - so I didn't add this either.

Also, the currently available Jorje library (last updated in Feb. 2022) doesn't support the syntax insert as user new Contact(LastName='foo'). It fails with the following exception:

Caused by: apex.jorje.services.exception.ParseException: Syntax(error = UnexpectedToken(loc = (10, 16, 53, 55), token = 'as'))

There doesn't seem to be a new version available yet: https://github.com/forcedotcom/salesforcedx-vscode/commits/develop/packages/salesforcedx-vscode-apex/out/apex-jorje-lsp.jar

Apart from these two points, the PR #4244 fixes this issues.

adangel added a commit that referenced this issue Dec 19, 2022
[apex] ApexCRUDViolation: user mode and system mode with test cases added #4244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-negative PMD doesn't flag a problematic piece of code an:enhancement An improvement on existing features / rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants