Skip to content

Conversation

@jasonsiders
Copy link
Owner

@jasonsiders jasonsiders commented Sep 10, 2025

Closes #128, where SoqlTest was throwing various errors when run by non-admin users. In this case, it was a CICD/integration user that had very little access to the data model. This caused queries which reference even standard fields like Contact.AccountId to fail.

Fixed by forcing all queries to run in system mode after asserting that the correct query was generated.

@claude
Copy link
Contributor

claude bot commented Sep 10, 2025

Claude finished @jasonsiders's task —— View job


PR Code Review

I've thoroughly reviewed this pull request that fixes CRUD/FLS errors in unit tests for non-admin users.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze the modified SoqlTest.cls file
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide comprehensive feedback

Code Review Findings

Code Quality & Best Practices

  • Clean Implementation: The fix is minimal and targeted, adding only one line of code at /source/classes/SoqlTest.cls:1637
  • Proper Placement: The setAccessLevel(System.AccessLevel.SYSTEM_MODE) call is correctly placed after query validation but before execution
  • Maintainable Design: The solution maintains the existing test structure while solving the permissions issue
  • Documentation: The code comment clearly explains the reasoning for running in system mode

Bug Prevention & Issues

  • No Logic Bugs: The fix correctly addresses the root cause of CRUD/FLS errors for restricted users
  • No Side Effects: The change only affects query execution mode, not the query validation or generation logic
  • Appropriate Timing: System mode is enabled after asserting that the correct query was generated, preserving the test's validation integrity

Performance Considerations

  • No Performance Impact: Adding setAccessLevel() has negligible performance overhead
  • No Query Changes: The underlying query structure remains unchanged, so execution plans are unaffected

Security Assessment

  • Appropriate Use of System Mode: This is a legitimate use case for SYSTEM_MODE in unit tests
  • Controlled Scope: System mode is only applied during test execution, not in production code paths
  • Test Isolation: The change is confined to the test validation helper method, maintaining security boundaries
  • Principle of Least Privilege: The system mode is only enabled for the minimal necessary operation (query execution)

Test Coverage

  • No Coverage Reduction: All existing test methods continue to validate query generation logic
  • Enhanced Reliability: Tests will now pass consistently regardless of the running user's permissions
  • CI/CD Friendly: Addresses the core issue with integration users having limited access to standard objects

Key Technical Details

Root Cause Analysis: The issue occurred because queries referencing standard fields like Contact.AccountId would fail when executed by users (like CI/CD integration users) lacking sufficient object-level permissions.

Solution Strategy: The fix follows the pattern:

  1. First validate that the correct SOQL query was generated
  2. Then enable system mode for query execution to bypass permission checks
  3. Execute the query to complete the test

File Modified: source/classes/SoqlTest.cls - specifically the validateQuery helper method at line 1637.

Recommendation: ✅ APPROVE

This is a well-implemented solution that:

  • Solves the immediate problem of test failures for non-admin users
  • Maintains test integrity by validating query generation before enabling system mode
  • Uses minimal, targeted changes with clear documentation
  • Follows Salesforce security best practices for test methods
  • Will improve CI/CD pipeline reliability

The implementation is clean, secure, and addresses the requirements described in issue #128 effectively.


Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

✅ Static Analysis: Check Passed

Analyzed changed files, and found 0 potential violations. 0 meets or exceeds the set severity threshold: Info

⚫ 0 Critical severity violation(s)
🔴 0 High severity violation(s)
🟠 0 Medium severity violation(s)
🟡 0 Low severity violation(s)
⚪ 0 Info severity violation(s)

@jasonsiders jasonsiders merged commit d0d96c8 into main Sep 10, 2025
3 checks passed
@jasonsiders jasonsiders deleted the 128-update-tests-to-work-regardless-of-running-user branch September 10, 2025 00:30
@jasonsiders jasonsiders mentioned this pull request Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Tests to work regardless of running user

2 participants