FIX (GraphEngine): @W-13869734@: Fix crash on explicit access level specified in Database.query method #1154
+245
−38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MrEminent42
changed the title
MrEminent42
commented
Aug 18, 2023
sfge/src/main/java/com/salesforce/rules/fls/apex/operations/ValidationConverter.java
Outdated
Show resolved
Hide resolved
MrEminent42
force-pushed
the
d/W-13869734
branch
2 times, most recently
from
bfdf195
to
38f0d95
Compare
MrEminent42
commented
Aug 21, 2023
There was a problem hiding this comment.
Changes these tests to only use a subset of the methods in the Database class. They were taking way too long. Full coverage of all methods is in SharingPolicySimpleTest.java.
| "Database.getQueryLocator('SELECT Id, NumberOfEmployees FROM account WHERE NumberOfEmployees = 3 LIMIT 1')", | ||
| "Database.getQueryLocatorWithBinds('SELECT Id, NumberOfEmployees FROM account WHERE NumberOfEmployees = 3 LIMIT 1', new Map<String, Object>{'name' => 'KENSINGTON'})", | ||
| "Database.getQueryLocator('SELECT Name, NumberOfEmployees FROM account WHERE NumberOfEmployees = 3 LIMIT 1')", | ||
| "Database.getQueryLocatorWithBinds('SELECT Name, NumberOfEmployees FROM account WHERE NumberOfEmployees = :i LIMIT 1', new Map<String, Object>{'i' => 3})", |
There was a problem hiding this comment.
Updated these because Josh mentioned SELECT Id, .. is not good to have, possibly an error. Also, I made the original binds when I didn't really know what they were, and they didn't make sense.
jfeingold35
reviewed
Aug 29, 2023
sfge/src/main/java/com/salesforce/graph/symbols/apex/ApexSoqlValueFactory.java
Outdated
Show resolved
Hide resolved
...java/com/salesforce/rules/usewithsharingondatabaseoperation/SharingPolicySubclassesTest.java
Show resolved
Hide resolved
sfge/src/test/java/com/salesforce/rules/fls/apex/ReadFlsScenariosTest.java
Show resolved
Hide resolved
sfge/src/test/java/com/salesforce/rules/fls/apex/ReadFlsScenariosTest.java
Show resolved
Hide resolved
sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java
Outdated
Show resolved
Hide resolved
sfge/src/main/java/com/salesforce/rules/fls/apex/operations/ValidationConverter.java
Outdated
Show resolved
Hide resolved
sfge/src/test/java/com/salesforce/rules/fls/apex/ValidationConverterTest.java
Outdated
Show resolved
Hide resolved
sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java
Outdated
Show resolved
Hide resolved
sfge/src/main/java/com/salesforce/rules/fls/apex/operations/ValidationConverter.java
Outdated
Show resolved
Hide resolved
sfge/src/main/java/com/salesforce/rules/fls/apex/operations/ValidationConverter.java
Outdated
Show resolved
Hide resolved
jfeingold35
approved these changes
Aug 30, 2023
MrEminent42
force-pushed
the
d/W-13869734
branch
2 times, most recently
from
53ca55e
to
bd667f7
Compare
…h explicit AccessLevel @W-13869734@: clean up docs again @W-13869734@: clean up docs @W-13869734@: PR feedback, refactor boolean pass-down @W-13869734@: clean up @W-13869734@: fix unused test @W-13869734@: add disabled test for methods in Database class beyond basic 6 @W-13869734@: fix bug expecting validations & crashing when Database.query() and similar were executed in USER_MODE @W-13869734@: update UseWithSharingOnDatabaseOperation tests - limit subclasses tests to only a few select database operations, not all of them; significantly speeds up testing - update the list of DATABASE_METHODS to include Database.query(String, AccessLevel)
MrEminent42
force-pushed
the
d/W-13869734
branch
from
bd667f7
to
4dd9128
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Databaseclass that include aSystem.AccessLevelas their last parameter.AccessLevel.USER_MODEis specified, prevents expected validations from being created on the vertex.Notes:
ValidationConverter#getHolderthatchildApexValue instanceof ApexStringValueis true IF AND ONLY IF the vertex we are inspecting is one of theDatabase.whatevermethods that could contain an explicit access level.convertSoqlQueryInfo(ValidationHolder, ProcessFields, HashSet<SoqlQueryInfo>, boolean), the last boolean (representing an override that makes any SOQL operation safe to perform without FLS validations) should betrue?Additional notes:
UseWithSharingOnDatabaseOperationto test theDatabase.query(String, AccessLevel)method specifically when it has an access level specified.SharingPolicySubclassesTestto use a select subset of possible database operations, not all of them. The tests were taking quite a long time. Testing all operations was not necessary since the full breadth of operations is tested inSharingPolicySimpleTest.Known bugs/limiting behavior:
insert,merge,query,undelete,upsert, andupdatemethods of theDatabaseclass in Apex are checked for FLS violations. TODO: check all methods, likequeryWithBinds, etc.