chore: Introduce orgId param for non-reactive feature flagged methods to fetch correct org flags#39519
chore: Introduce orgId param for non-reactive feature flagged methods to fetch correct org flags#39519
Conversation
WalkthroughThis pull request propagates the current user’s organization context throughout the codebase. Most modifications update method signatures and internal logic to invoke Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant ReactiveContextUtils
participant PermissionProvider
Client->>Service: Request resource operation
Service->>ReactiveContextUtils: getCurrentUser()
ReactiveContextUtils-->>Service: User { organizationId }
Service->>PermissionProvider: getPermission(organizationId)
PermissionProvider-->>Service: Permission result
Service->>Client: Return operation outcome
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
Failed server tests
|
Failed server tests
|
Failed server tests
|
…m:appsmithorg/appsmith into chore/handle-sync-feature-flagged-methods
abhvsn
added
ok-to-test
abhvsn
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (1)
117-136: Organization-specific feature flag lookup improves context separation.The code now correctly extracts the organization ID from the trigger request and uses it to retrieve organization-specific feature flags. This ensures proper multi-tenant isolation for feature flags.
Consider extracting the feature flag retrieval logic into a separate method to improve readability:
- String organizationId = updatedTriggerRequestDTO.getOrganizationId(); - // TODO: Flags are needed here for google sheets integration to support shared - // drive behind a flag - // Once thoroughly tested, this flag can be removed - Map<String, Boolean> featureFlagMap = - featureFlagService.getCachedOrganizationFeatureFlags(organizationId) - != null - ? featureFlagService - .getCachedOrganizationFeatureFlags(organizationId) - .getFeatures() - : Collections.emptyMap(); + Map<String, Boolean> featureFlagMap = getFeatureFlagsForOrganization(updatedTriggerRequestDTO.getOrganizationId());With a separate helper method:
private Map<String, Boolean> getFeatureFlagsForOrganization(String organizationId) { // TODO: Flags are needed here for google sheets integration to support shared // drive behind a flag // Once thoroughly tested, this flag can be removed return featureFlagService.getCachedOrganizationFeatureFlags(organizationId) != null ? featureFlagService.getCachedOrganizationFeatureFlags(organizationId).getFeatures() : Collections.emptyMap(); }app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (1)
305-345: Added organization-scoped permission validation.The code now properly retrieves the current user and checks permissions using the organization ID when creating a datasource. This ensures proper multi-organization support.
Consider extracting the permission check logic to a separate method to improve readability:
- .switchIfEmpty( - Mono.defer(() -> ReactiveContextUtils.getCurrentUser().flatMap(user -> { - // check if user has permission to create datasource - if (!permissionProvider.canCreateDatasource(workspace, user.getOrganizationId())) { - log.error( - "Unauthorized to create datasource: {} in workspace: {}", - datasourceStorage.getName(), - workspace.getName()); - return Mono.error(new AppsmithException( - AppsmithError.ACL_NO_RESOURCE_FOUND, - FieldName.DATASOURCE, - datasourceStorage.getName())); - } + .switchIfEmpty( + Mono.defer(() -> ReactiveContextUtils.getCurrentUser().flatMap(user -> + checkDatasourceCreatePermission(user, workspace, datasourceStorage) + .then(Mono.defer(() -> {With a separate permission check method:
private Mono<Void> checkDatasourceCreatePermission(User user, Workspace workspace, DatasourceStorage datasourceStorage) { if (!permissionProvider.canCreateDatasource(workspace, user.getOrganizationId())) { log.error( "Unauthorized to create datasource: {} in workspace: {}", datasourceStorage.getName(), workspace.getName()); return Mono.error(new AppsmithException( AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.DATASOURCE, datasourceStorage.getName())); } return Mono.empty(); }app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (1)
4174-4176: Consider avoidingblock()in reactive code.While it's acceptable in tests, using
block()in reactive streams can lead to thread starvation issues in production code. For tests, consider using StepVerifier instead of blocking operations where possible.- String orgId = ReactiveContextUtils.getCurrentUser() - .map(User::getOrganizationId) - .block(); + // For tests, we could use a mock or get from test user + String orgId = ReactiveContextUtils.getCurrentUser() + .map(User::getOrganizationId) + .blockOptional() + .orElse("default-organization-id");app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (2)
136-140: Consistent use of TRUE constantThe change to use the statically imported TRUE constant makes the code more concise. Note that you're still using
Boolean.FALSEon line 139 - consider also importing FALSE statically for consistency.- } else { - forkWithConfig = Boolean.FALSE; + } else { + forkWithConfig = FALSE;
136-140: Simplified boolean comparisonThe code now consistently uses the static TRUE constant, improving readability. Note that only the TRUE cases were updated, and the FALSE case was left using the explicit Boolean.FALSE.
For consistency, consider also using a static import for FALSE:
- forkWithConfig = Boolean.FALSE; + forkWithConfig = FALSE;app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCEImpl.java (2)
21-21: Parameter 'organizationId' is unused.
Currently,organizationIdis not being utilized within this method. Consider removing it or leveraging it for additional permission granularity if needed.
36-36: Parameter 'organizationId' is unused.
The added parameter is never referenced in this method. Clarify its intended use or remove to reduce confusion and maintenance overhead.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java (1)
39-53: Forgot password flow using default organization.
Temporarily falling back to the default org ID is acceptable if the real mapping is forthcoming. Ensure a TODO or backlog item exists to revisit once multi-org support for new sessions is finalized.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCEImpl.java (3)
24-26: Unused parameter in getDeletePermission.
TheorganizationIdparameter is not being utilized inside the method, which could lead to confusion or future maintenance issues. Consider removing or implementing usage if needed.
29-31: Unused parameter in getApplicationCreatePermission.
Similar to above,organizationIdisn’t used. If it’s intended for future expansion, add a clarifying comment. Otherwise, remove it to keep signatures clean.
34-36: Unused parameter in getDatasourceCreatePermission.
Same observation here: theorganizationIdparameter is unreferenced. Update the method body if you plan to enforce organization-based checks.app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java (1)
128-142: Parameter-based org ID extraction.
The logic for scanning parameters named “organizationId” or “orgId” is a neat approach. It’s worth adding a quick note if multiple org ID parameters exist or if future param names might conflict.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCE.java (1)
10-22: Clarify howorganizationIdis used in these templates
All newly introduced parameters remain unused in the returned strings. If the intent is to customize templates per organization, please ensure the parameter is integrated or documented accordingly.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCEImpl.java (1)
39-71: Confirm usage oforganizationIdin email templates
All methods now includeorganizationIdbut return static templates. If the plan is to generate org-specific content, ensureorganizationIdis actually utilized.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (2)
42-42: Consider using ConcurrentHashMap for thread safetyThe HashMap implementation isn't thread-safe, which could lead to data corruption when multiple threads update the cache simultaneously.
- private Map<String, CachedFeatures> cachedOrganizationFeatureFlags = new HashMap<>(); + private Map<String, CachedFeatures> cachedOrganizationFeatureFlags = new ConcurrentHashMap<>();
201-203: Consider adding cache expiration mechanismThe current implementation could lead to memory leaks as organizations are added but never removed from the cache.
Consider adding a mechanism to evict stale entries, such as:
- Time-based expiration
- Size-limited cache with LRU eviction
- Weak references to allow garbage collection
This would prevent unbounded memory growth in long-running applications.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (85)
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ReactiveContextUtils.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/importable/artifactbased/ArtifactBasedImportableServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/artifactbased/ArtifactBasedImportServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java(8 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ContextPermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceStructureSolutionCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PagePermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PagePermissionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCE.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/fork/ApplicationForkingServiceTests.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/EmailServiceHelperCETest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderTest.java(5 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationSnapshotServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceStorageServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/MockDataServiceTest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ThemeServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ThemeImportableServiceCETest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceStructureSolutionTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceTriggerSolutionTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java(2 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java
🧰 Additional context used
🧠 Learnings (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#39171
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java:42-44
Timestamp: 2025-02-24T05:59:14.021Z
Learning: The reactive transition for helper methods consuming @FeatureFlagged annotations will be handled in a separate PR to maintain focused changes and proper separation of concerns.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: perform-test / ci-test-result
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (206)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java (2)
246-250: Add null check for current user - good defensive codingThis addition correctly handles the case where no user might be present in the reactive context, preventing potential
NullPointerExceptionwhen accessing the user's organization ID.
251-254: Properly propagate organization context to permission checkThe change now correctly passes the current user's organization ID to the
getDeletePermissionmethod, ensuring that permissions are checked in the context of the user's organization. This aligns with the multi-organization setup requirements.app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java (1)
156-159: Good implementation of organization context.The change correctly retrieves the organization ID from the current user and passes it to the
getDeletePermissionmethod, ensuring proper organization-specific permission checking during test cleanup.app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java (1)
258-261: Added organization context to permission checkThe change correctly retrieves the workspace first, then extracts its organization ID to pass to the permission check. This properly propagates the organization context through the chain of method calls, ensuring that permissions are evaluated in the correct organizational scope.
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java (3)
39-39: LGTM: Added import for ReactiveContextUtilsThe import is necessary for the updated cleanup method implementation.
296-299: Good addition of null check for current userThis properly handles the case where no user context is available during test cleanup, preventing potential NPEs.
301-302: Correctly propagated organization context to permission checkThis change aligns with the PR objective to ensure that organization context is properly passed to methods that need it for permission checks. The organization ID is now correctly retrieved from the current user and passed to the
getDeletePermissionmethod.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (4)
10-10: New import for User domain model introduced.The addition of the User domain model import is necessary to support the subsequent changes for organization ID access.
20-20: New ReactiveContextUtils import added for context access.This import provides access to utility methods for retrieving the current user from the reactive context, which is needed for the organization ID.
439-444: Good implementation of reactive user context retrieval.The implementation properly retrieves the current user context and caches it to avoid multiple retrievals. The cached user context is then zipped with each page being processed, providing the organization context needed for permission checks.
472-474: Permission check correctly uses organization ID from current user.The change ensures that page creation permissions are correctly checked within the context of the user's organization. This prevents permission issues in multi-organization setups.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PagePermissionCE.java (1)
5-6: Interface update correctly adds organization context for permission checksThe addition of an
organizationIdparameter to thegetDeletePermissionmethod aligns with the PR's goal of propagating organization context for feature flagged methods.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java (1)
45-45: Method signature properly updated to include organizationId parameterThis change ensures that the cached organization feature flags are retrieved with the correct organization context, which fixes the issue mentioned in the PR description where the wrong organization's flags might be identified.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE.java (1)
25-25: Method return type correctly changed to MonoConverting this method to return a reactive type allows it to be used in reactive chains, improving consistency with other reactive methods and enabling better integration with organization context propagation.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceStorageServiceTest.java (1)
82-83: Test updated to pass organizationId to getDeletePermission methodThe test has been correctly updated to pass the workspace's organization ID to the getDeletePermission method, maintaining compatibility with the interface changes.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/MockDataServiceTest.java (3)
106-106: Added organizationId field to support multi-org contextThe organizationId field is being added to properly test the updated permission methods that require organization context.
122-122: Initializing organizationId from workspaceCorrectly initializes the organizationId field from the workspace object to provide proper context for permission checks.
137-137: Updated getDeletePermission call to include organizationIdThe method call has been updated to pass the organization ID to the getDeletePermission method, aligning with the updated interface.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCE.java (1)
6-6:Details
✅ Verification successful
Updated interface method to require organizationId parameter
Method signature now requires an organizationId parameter, which is essential for determining the correct organization-specific permissions. This change directly addresses the PR objective for improving permission checks in multi-organization contexts.
🏁 Script executed:
#!/bin/bash # Check all implementations of this interface to ensure they've been updated # to match the new signature with organizationId parameter echo "Finding all implementations of ActionPermissionCE interface..." rg -l "implements\s+ActionPermissionCE" --type java echo "Finding all uses of getDeletePermission to ensure they pass organizationId..." rg "getDeletePermission\s*\(\s*\)" --type javaLength of output: 504
Updated getDeletePermission Signature Verified: Organization ID Parameter Correctly Enforced
- Confirmed that the implementation in
ActionPermissionCEImpl.javanow uses the updated signature.- No instances were found where
getDeletePermission()is invoked without passing an organizationId.The changes satisfy the multi-organization permission check requirements.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java (1)
90-91: Updated getDeletePermission call to include organizationIdModified the method call to pass the organization ID from the workspace to align with the updated interface method signature. This ensures that tests properly reflect the production code behavior with organization context.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCE.java (1)
12-12: Method signature updated to include organizationId parameter.The
getPageCreatePermissionmethod signature has been updated to accept an organization ID parameter, which aligns with the multi-organization setup mentioned in the PR objectives.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java (1)
9-9: Method signature updated to include organizationId parameter.The
getDeletePermissionmethod now requires an organization ID parameter, which enables proper permission checks in a multi-organization context.app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java (3)
148-148: Added organization ID field to support permission checks.Adding this static field is necessary to store the organization context retrieved from the current user, supporting the multi-organization enhancement.
158-158: Properly initializing organizationId field in setup.The field is properly initialized by extracting the organization ID from the current user during test setup.
254-254: Updated permission check to include organization context.The
getDeletePermissionmethod call now includes the organization ID parameter as required by the updated interface.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ReactiveContextUtils.java (1)
1-21: Well-implemented utility for retrieving the current user.This utility class provides a clean, reactive approach to accessing the current user from the security context. It properly handles the case when no user is found with appropriate error logging.
The implementation follows reactive patterns and provides a consistent way to obtain the current user's context, which is essential for the organization-aware permission checks being implemented across the application.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.java (1)
10-12: Constructor updated to handle organization context.The constructor is properly modified to accept and pass the
OrganizationServiceto the parent class, ensuring organization-specific feature flag checks can be performed.app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java (1)
211-212: Updated permission check to include organization context.The deletion permission check now correctly uses the current user's organization ID, aligning with the multi-organization setup.
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java (2)
106-109: Added organization-aware permission handling.The cleanup method now correctly retrieves the current user and includes appropriate null checks before proceeding with application deletion. This ensures proper organization context throughout the test lifecycle.
111-112: Updated permission check with organization ID.The call to
getDeletePermission()now includes the organization ID parameter, ensuring correct permission checks in multi-organization environments.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ThemeImportableServiceCETest.java (2)
93-96: Added organization-aware permission handling.The cleanup method correctly retrieves the current user context and includes a null check to prevent NPEs, ensuring the test cleanup process is organization-aware.
98-99: Updated permission check with organization context.The
getDeletePermission()call now properly includes the organization ID parameter, ensuring the correct permissions are retrieved in a multi-organization environment.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java (4)
10-10: Appropriate addition of ReactiveContextUtils importThis import is needed for the getCurrentUser functionality now being used in the test to obtain organization context.
292-295: Good implementation of organization context retrievalThe code correctly retrieves the current user's organization ID for proper context in feature flag retrieval. This is consistent with the PR objective of ensuring correct organization flags in a multi-organization setup.
296-296: Appropriate update to method call with organizationId parameterThe method signature change from
getCachedOrganizationFeatureFlags()togetCachedOrganizationFeatureFlags(orgId)provides the necessary organization context for the test.
312-312: Correct propagation of organization ID parameterThe second call to
getCachedOrganizationFeatureFlagscorrectly passes the organization ID, maintaining consistency in the test.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceStructureSolutionTest.java (3)
22-22: Appropriate addition of ReactiveContextUtils importThis import is needed for retrieving the current user context in the test cleanup.
130-133: Good null check implementation for current userThe code correctly handles the case where no current user exists, preventing potential NullPointerException in the cleanup method.
135-137: Proper usage of organization context for permission checkThe code correctly passes the current user's organization ID to the permission check, ensuring that the right organization context is used for application deletion.
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java (2)
92-94: Appropriate method signature updateThe
createNewResourcemethod signature has been properly updated to include the organization ID parameter, which is consistent with the PR objective of propagating organization context.
94-94: Correct usage of organization ID in permission checkThe code properly passes the organization ID to the
canCreateActionmethod, ensuring that the permission check considers the correct organization context.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ThemeServiceTest.java (3)
16-16: Appropriate addition of ReactiveContextUtils importThis import is needed for retrieving the current user in the test cleanup method.
111-114: Good null check implementation for current userThe code correctly handles the case where no current user exists, preventing potential NullPointerException in the cleanup method.
116-118: Proper usage of organization context for permission checkThe code correctly passes the current user's organization ID to the permission check, ensuring proper organization context for application deletion during cleanup.
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java (3)
124-124: Added organization ID field for tests.Good addition of a static field to store the organization ID, which will be used across test methods.
138-138: Properly initialized the organization ID in setup.The initialization is correctly placed in the setup method, extracting the ID from the api_user.
235-235: Updated permission check to include organization context.The change correctly passes the organization ID to the permission check method, aligning with the multi-organization setup requirements.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCEImpl.java (2)
4-4: Added NotNull annotation import.This import is required for the method parameter annotation.
32-32: Updated method signature to include organization ID parameter.Added the required
organizationIdparameter with@NotNullannotation to ensure proper validation. This change is central to the PR objective of propagating organization context to permission checks.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (2)
287-290: Added null check for current user in cleanup method.Good defensive programming - adding a null check for the current user and early return prevents potential NullPointerExceptions during cleanup operations.
292-293: Updated permission check to use the current user's organization ID.The change correctly passes the current user's organization ID to the permission check method, ensuring the proper context for deletion operations.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (3)
38-38: Added ReactiveContextUtils import.Required import for accessing the current user in a reactive context.
159-162: Added null check for current user in cleanup method.Similar to the pattern in ActionServiceCE_Test, this adds proper defensive programming by checking for null user and early return to prevent exceptions.
164-165: Updated permission check to use organization context.The modification correctly passes the current user's organization ID to the permission check, maintaining consistency with the PR's objective.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java (4)
124-124: Added organization ID for permission checks.This new static variable will store the organization ID needed for permission operations throughout the test class. This aligns with the PR objective of making non-reactive methods organization-aware.
135-135: Organization ID correctly initialized from workspace.Properly extracting the organization ID from the workspace during test setup ensures consistent access to the same organization context in subsequent operations.
144-144: Updated permission check to use organization ID.This change enhances the application permission check by providing the organization context, ensuring that permissions are properly scoped to the relevant organization.
193-193: Fixed datasource permission check with organization ID.Updated the delete permission check to include the organization ID parameter, ensuring datasource deletion operations respect organization-specific permissions.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java (3)
128-128: Added organization ID for permission checks.Introduced static variable to maintain organization context across test methods, aligning with the multi-organization permission model.
140-140: Organization ID correctly initialized from workspace.Properly capturing the organization ID during test setup ensures consistent organization context is available for permission checks throughout the test lifecycle.
149-149: Updated permission check to use organization ID.Enhanced the application delete permission check to include organization context, ensuring that test cleanup operations respect organization boundaries.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (2)
86-86: Storing organization ID in context store.Added organization ID to the extension context store to make it available throughout the test lifecycle, supporting organization-aware permission checks.
95-100: Using organization ID for delete permission in cleanup.Properly retrieving and using the organization ID for application delete permissions during test cleanup ensures that permissions are correctly scoped to the organization.
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java (3)
15-15: Added ReactiveContextUtils for accessing current user.Added import for ReactiveContextUtils to access the current user information, which is needed to obtain organization context.
166-173: Fetching current user to get organization ID.Enhanced the Mono.zip operation to include current user retrieval and extract the organization ID. This ensures that resource operations are performed in the correct organizational context.
236-236: Updated createNewResource call with organization ID.Added organization ID parameter to the createNewResource method call, ensuring that new resource creation respects organization-specific permissions and context.
app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (3)
238-238: Good addition of organization context.The introduction of the
organizationIdstatic variable helps maintain consistent organization context throughout test operations.
253-253: Appropriate initialization of organizationId.Correctly capturing the organization ID from workspace during setup ensures tests operate with proper context.
273-273: Proper propagation of organizationId parameter.This change correctly passes the organization ID to the permission check, which aligns with the PR objective to propagate organization context for non-reactive feature-flagged methods.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationSnapshotServiceTest.java (1)
81-82: Added organization context to permission checksThe change correctly enhances the cleanup method to include the organization ID when retrieving delete permissions, ensuring proper organization-specific permission validation during test cleanup.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (3)
155-155: Added organizationId field to store contextNew static field to store the organization ID for consistent use throughout test methods.
162-162: Captured organization context from test userProperly initializes the static organizationId field from the test user in the setup method.
222-222: Added organization context to permission checksThe change correctly enhances the cleanup method to include the organization ID when retrieving delete permissions, ensuring proper organization-specific permission validation during test cleanup.
app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (3)
156-156: Adding static organizationId field to store context.This addition correctly adds storage for the organization ID which will be needed for permission checks.
262-262: Setting organizationId from workspace during setup.Good implementation to initialize the organization ID from the saved workspace, ensuring it's available for all tests.
944-944: Updated permission check to include organizationId parameter.The
getPageCreatePermission()method call has been correctly updated to include the organizationId parameter, aligning with the updated method signature in the application permission provider.app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (1)
89-97: Properly propagates organization context in feature flag retrievalThe change correctly retrieves feature flags based on the organization ID from the trigger request, ensuring proper multi-organization support. This is critical for features like Google Sheets integration that may be enabled differently across organizations.
app/server/appsmith-server/src/test/java/com/appsmith/server/fork/ApplicationForkingServiceTests.java (1)
603-614: Added organization-aware permission checkThe change properly propagates organization ID to the permission check, which is necessary for the multi-organization setup. This ensures policies are filtered correctly based on organization context.
app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (2)
19-19: Appropriate import for retrieving user contextAdded import for ReactiveContextUtils which will be used to get the current user's organization context.
481-482: Updated to use organization-specific permission checkThe method now correctly retrieves the current user to obtain organization-specific delete permissions. This change is essential for proper access control in a multi-organization environment.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java (1)
122-123: Updated test to use organization-specific permissionTest cleanup now properly uses the organization context when checking delete permissions, maintaining consistency with the implementation changes.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (2)
170-178: Improved variable naming for better code clarity.The variable rename from
orgtovalidatedWorkspacemakes the code more self-documenting by clearly indicating that this is a validated workspace object.
575-577: Enhanced permission check with organizational context.The modification correctly leverages the current user's organization ID for permission validation when archiving workspaces. This ensures permissions are scoped to the appropriate organization context.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceStructureSolutionCEImpl.java (1)
186-188: Improved permission check with organizational context.The change correctly retrieves the current user and uses their organization ID for permission validation. This ensures that permissions are properly scoped when fetching schema preview data.
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/importable/artifactbased/ArtifactBasedImportableServiceCE.java (1)
43-44: Added organization context to resource creation method.The interface now requires implementations to handle organization ID during resource creation, ensuring proper permission scoping. This is consistent with the multi-organization architecture requirements.
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceTriggerSolutionTest.java (1)
126-133: Updated test to handle organization-specific permissions with null safety.The test cleanup now properly checks for null user and uses organization context for permission validation, consistent with implementation changes. The null check prevents potential NullPointerExceptions.
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java (2)
91-95: Correctly incorporated organizationId parameterThe method signature change properly adds the organizationId parameter required for multi-organization setups.
96-96: Organization context correctly utilized for permission checkThe organizationId parameter is properly utilized in the permission check, aligning with the multi-organization requirements.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (3)
26-26: Appropriate import for ReactiveContextUtilsAdded the necessary import for ReactiveContextUtils to retrieve the current user context.
155-162: User organization context properly retrieved for permissionsGood implementation that retrieves the current user context to obtain the organization ID for permission checking. This follows the pattern established throughout the codebase.
889-892: Consistent pattern used for retrieving organization contextThe same pattern is correctly applied to the archive method, maintaining consistency across the codebase.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ContextPermissionCE.java (1)
9-9: Interface method signature updated for organization contextThe method signature has been appropriately updated to include the organizationId parameter. Making it a default method with a null return ensures backward compatibility.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (3)
35-35: Added required import for user contextThe import for ReactiveContextUtils is correctly added to support retrieving the current user's organization ID.
232-234: Organization context incorporated for datasource accessProperly retrieves the current user to obtain their organization ID for datasource permission checks, following the established pattern.
505-507: Consistent organization context usage for application accessThe same pattern is correctly applied for application permissions, maintaining consistency in how organization context is utilized.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCE.java (3)
6-6: Method signature updated to include organizationId parameterThis change adds the
organizationIdparameter to thegetDeletePermissionmethod, which is necessary for proper permission checking in a multi-organization setup.
8-8: Method signature updated to include organizationId parameterThe
getApplicationCreatePermissionmethod now accepts anorganizationIdparameter to ensure permissions are checked against the correct organization context.
10-10: Method signature updated to include organizationId parameterThe
getDatasourceCreatePermissionmethod has been modified to accept anorganizationIdparameter, ensuring consistent organization-specific permission checking.app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (3)
77-77: Added organizationId field to support permission checksIntroduced the
organizationIdfield to store the organization context needed for permission checks in tests.
84-86: Updated to capture workspace and its organizationIdModified the setup to capture the full workspace object and extract its organization ID, which will be needed for permission checks.
92-92: Updated method call to include organizationId parameterThe call to
applicationPermission.getDeletePermission()now includes theorganizationIdparameter, aligning with the updated interface method signature.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (1)
15-15: Added necessary imports for User context handlingAdded imports for the
Userdomain object andReactiveContextUtilshelper class to support the organization context in tests.Also applies to: 19-19
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (4)
14-14: Added imports for user context accessAdded imports for the
Userdomain class andReactiveContextUtilshelper to support retrieving the current user's organization ID.Also applies to: 24-24
310-311: Added user context retrieval for organization IDAdded code to retrieve the current user via
ReactiveContextUtils.getCurrentUser()and include it in theMono.zipoperation to provide organization context.
315-315: Extract organization ID from current userExtracted the organization ID from the current user to pass to the resource creation method.
Consider adding a null check for the organization ID to handle edge cases where a user might not have an organization association:
String currentUserOrgId = objects.getT3().getOrganizationId(); if (currentUserOrgId == null) { return Mono.error(new AppsmithException(AppsmithError.ORGANIZATION_ID_NOT_FOUND)); }
376-376: Updated method call to include organization ID parameterAdded the
currentUserOrgIdparameter to thecreateNewResourcemethod call to ensure actions are created with the correct organization context.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
799-801: Enhanced permission check with organization context.The code now correctly retrieves the current user's organization ID for permission checks when deleting unpublished actions. This ensures that permissions are appropriately scoped by organization.
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/EmailServiceHelperCETest.java (3)
32-33: Added organization ID field for testing.This static field stores the organization ID for use in tests that need to verify organization-scoped functionality.
47-48: Properly initializes organization ID from test data.The test now captures the organization ID from the default organization configuration, ensuring that subsequent tests use a valid ID.
52-54: Updated test assertions to include organization context.The tests now properly pass the organization ID parameter to the methods being tested, ensuring they validate the correct behavior with organization context.
Also applies to: 58-61, 66-68, 72-73
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (2)
27-27: LGTM - Necessary import for the ReactiveContextUtils.The import is correctly added to support the new reactive context retrieval in updated methods.
94-110: Well-implemented reactive conversion.The method signature has been correctly updated to return a
Mono<ImportArtifactPermissionProvider>instead of a synchronous result. The implementation properly retrieves the current user's organization ID and uses it for the permission check.app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCE.java (1)
16-22: Method signatures properly updated with organizationId parameter.All existing methods have been appropriately updated to include the organization ID parameter. The new
ceEeSyncMethodWithoutOrgIdmethod provides a variant without the organization ID, which helps maintain backward compatibility where needed.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4)
15-15: Required imports added for reactive context support.These imports are necessary to support the user context retrieval in this service.
Also applies to: 21-21
150-156: Good implementation of organization-aware permission check.The code now correctly retrieves the current user context and uses the organization ID to determine the appropriate permission. This ensures that permissions are enforced in the correct organizational context.
348-356: Properly updated permission check in createSingleAction.The implementation now correctly retrieves the current user and determines the appropriate permission based on the user's organization ID and action type.
439-449: Improved context-aware permission validation.The code now properly handles different permission types based on both the action type and the user's organization context, enhancing security and multi-organization support.
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderTest.java (5)
11-14: Required imports added for test updates.The imports are correctly added to support reactive context retrieval in the test.
53-60: Good test setup to initialize organizationId.The
@BeforeEachsetup correctly initializes the organization ID by retrieving it from the current user context. This ensures all tests have access to the required context.
76-78: Tests updated to use organizationId parameter.The assertions now correctly pass the organization ID to the permission check methods, ensuring they match the updated method signatures.
116-120: Permission tests appropriately updated with organization context.The domain and permission associations are now correctly constructed with organization-specific permissions.
129-133: Assertions updated to include organizationId.All test assertions now correctly include the organization ID when calling permission check methods.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PagePermissionCEImpl.java (2)
25-27: Method signature updated to include organization contextThe method signature now includes the required
organizationIdparameter to align with the multi-organization architecture.
30-32: Method signature updated to include organization contextSimilarly, this method signature has been updated to include the
organizationIdparameter for proper permission handling in a multi-organization setup.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCE.java (2)
6-6: Interface method signature updated for multi-organization supportThe
getDeletePermissionmethod signature has been updated to accept an organization ID parameter, maintaining consistency with the implementation class.
10-10: Interface method signature updated for multi-organization supportThe
getActionCreatePermissionmethod signature has been updated to accept an organization ID parameter, maintaining consistency with implementation classes.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (2)
41-41: Added import for reactive context utilitiesAdded import for
ReactiveContextUtilsto support retrieving current user context for organization-specific operations.
758-772:Details
❓ Verification inconclusive
Updated method to retrieve organization-specific feature flags
The method has been enhanced to retrieve the current user's organization context and use it for feature flag checks, replacing the previous implementation that didn't consider organization context.
The changes:
- Changed from
flatMaptozipWithto combine datasource validation with user context- Extract organization ID from the current user
- Use this ID to fetch organization-specific feature flags
Ensure that
ReactiveContextUtils.getCurrentUser()always returns a valid user in the execution context. Consider what happens if the user object is null or lacks an organization ID - are there fallbacks in place?
🏁 Script executed:
#!/bin/bash # Check how ReactiveContextUtils.getCurrentUser() is implemented and if it handles edge cases ast-grep --pattern 'ReactiveContextUtils.getCurrentUser()' -A 5Length of output: 43151
Action: Validate User Retrieval for Organization-Specific Feature Flags
The updated implementation now leverages
zipWith(ReactiveContextUtils.getCurrentUser())to access the current user and extract the organization ID for fetching feature flags. This change ensures that organization-specific feature flags are correctly utilized. However, please verify thatReactiveContextUtils.getCurrentUser()reliably returns a non-null user and that the returned user always contains a valid organization ID. If there's any chance of a null value, consider adding a fallback (such as emitting an error or providing a default value) to gracefully handle such cases in the reactive chain.
- Confirm that all reactive flows expecting a non-null user have appropriate error handling or default behavior.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (3)
53-53: New import for reactive context utilities.The addition of
ReactiveContextUtilsis necessary for accessing the current user's context in a reactive manner, which is needed for the organization ID extraction changes.
420-421: Updated permission check to include organization context.The modification properly propagates the current user's organization ID to the
getDeletePermission()method, ensuring that permission checks are correctly scoped to the user's organization context.
4179-4181: Properly scoped permission check with organization ID.The change to include the organization ID in the permission check is essential for multi-organization setups where permissions need to be evaluated in the context of a specific organization.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (4)
42-42: Good restructuring to support multiple organizationsThe change from a single
CachedFeaturesto aMap<String, CachedFeatures>enables proper organization-specific feature flags storage, which aligns with your multi-organization setup requirements.
185-185: Appropriately updated map storage logicThe storage implementation correctly uses the organizationId as a key when caching feature flags, ensuring each organization's flags are isolated.
201-203: Well-implemented accessor method with safe fallbackThis getter method correctly retrieves organization-specific feature flags with a proper default value when an organization has no cached flags.
185-185: Implementation now correctly stores per-organization feature flagsThis change properly associates cached features with their respective organizationId, fixing the issue mentioned in the PR description.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCEImpl.java (4)
35-35: Method signature updated but implementation doesn't utilize the parameterThe method signature now accepts an organization ID parameter, but the implementation doesn't use it yet. This may be intended as a first step toward organization-specific permissions.
Is this intentional as part of a phased approach? Consider adding a comment explaining why the parameter isn't used if this is by design.
50-50: Method signature updated but implementation doesn't utilize the parameterSimilar to the
getDeletePermissionmethod, this method now accepts an organization ID parameter but doesn't use it in the implementation.If this is intended as preparation for future organization-specific logic, consider adding a TODO comment for clarity.
35-35: Method signature updated to support organization-specific permissionsThe parameter is correctly added to match the interface requirement, allowing for future organization-specific permission logic.
50-50: Method signature updated to support organization-specific permissionsThe parameter is correctly added to match the interface requirement, allowing for future organization-specific permission logic.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java (4)
168-176: Improved error handling with reactive patternThe refactoring from
flatMaptozipWhen+mapcorrectly improves the reactive flow by combining the permission group with the last admin role check into a tuple. Using a direct exception throw here is appropriate since this represents a validation error in the business logic.
389-391: Properly updated method to reactive patternGood conversion of the
isLastAdminRoleEntitymethod to returnMono<Boolean>instead ofBoolean, making it consistent with reactive programming patterns used throughout the codebase.
168-176: Improved error handling when checking for last admin roleThe code now properly uses
zipWhento combine permission group with admin check result, making the flow more readable and structured.
389-391: Method now returns Mono for reactive consistencyThis change improves the reactive programming model consistency by returning a reactive type. However, the implementation is simple enough that it could have used
Mono.just(...)directly instead of building a new instance.app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (7)
68-68: Good use of static importUsing static import for commonly used constants improves code readability.
439-442: Correctly propagating organization context in permission checksThe change appropriately includes the user's organization ID in the permission check for application creation, adhering to the multi-organization support requirement.
573-673: Well-restructured permission checking logicThe extensive refactoring correctly incorporates organization ID in all relevant permission checks, including application creation and datasource creation. The code structure with the early return for forking-enabled applications is also cleaner.
676-680: Updated to use TRUE constant consistentlyThe method now uses the static TRUE constant, making the code more consistent with other similar checks in the class.
68-68: Code readability improved with static importUsing the static import for TRUE makes the code more concise and readable.
439-442: Now correctly passing organization context for permission checksThis change ensures the workspace permission check has the correct organization context, fixing the issue described in the PR.
574-673: Comprehensive update to include organization context in permission checksThis section has been thoroughly updated to include the organization context from the current user in all relevant permission checks.
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java (8)
5-17: Imports and annotations look fine.
The added imports forUser,ReactiveContextUtils, and@WithUserDetailshelp manage user contexts in tests. The static import forArgumentMatchers.any()aligns with Mockito usage.Also applies to: 26-26
42-42: Static field naming clarity.
Usingprivate static String organizationId;is acceptable. Ensure that the test setup consistently initializes this field before each relevant test to avoid null references.
51-53: Mock usage for organization feature flags.
You’re correctly mockinggetCachedOrganizationFeatureFlags(any())to return a fixed set of flags. Ensure this covers edge cases (e.g., missing flags or null checks) in separate tests if needed.
120-126: CE/EE method test with organization context.
PassingorganizationIdwhile mocking feature flags is consistent and verifies the correct code path for the EE implementation.
132-133: CE default behavior confirmation.
Ensuring the method falls back to CE implementation when flags are disabled or default is crucial. Good test coverage.
139-148: Exception testing with organization context.
Verifying the flow when both CE-compatible and EE code paths are present, including the thrown exception, is handled well. Continue adding coverage for unexpected runtime conditions if needed.
153-160: Non-Appsmith exception scenario.
It’s good to check that any unknown exception triggers the expected fallback or rethrows as anAppsmithException. This ensures robust error handling.
167-184: Test for missing organizationId in non-reactive method.
Catching and validating theAppsmithExceptionensures the method correctly enforces the organization context. This aligns with the new requirement to always provideorganizationId.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java (3)
8-10: Constructor injection for organization context.
InjectingOrganizationServiceandReactiveContextUtilsusage is consistent with the broader requirement to handle multi-organization logic.Also applies to: 26-33
74-85: Workspace invite emails linked to user’s organization.
IncludingorganizationIdfrom the current user ensures accurate brand context in the invitation. This improves multi-tenant correctness.
113-132: Instance admin invite with organization context.
Enriching the email parameters with the correct organization ID personalizes the email content. Continue verifying that newly introduced fields are always non-null to prevent runtime errors.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (3)
285-287: Adding organization context to feature flag checksThe addition of ReactiveContextUtils to retrieve current user and use their organization ID for permission checks is a good improvement. This ensures feature flags are evaluated in the correct organizational context.
292-296: Consistent permission handling with organization contextSimilar to the previous change, this correctly retrieves the current user's organization ID for both permission parameters, ensuring consistent permission enforcement within the correct organizational context.
427-438: Improved permission handling in archive functionalityThe changes properly implement organization-aware permission checks in the archiveGivenActionCollection method. Good use of caching the user Mono to avoid multiple retrievals, and correctly passing organization context to both unpublished and published action flux pipelines.
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java (4)
43-43: Updated method signature to include organizationId parameterMethod signature properly updated to include the organizationId parameter while maintaining the original functionality.
49-49: Updated exception throwing method to include organizationIdCorrectly added the organizationId parameter to maintain consistency with other method signature changes.
55-55: Consistent signature update for non-Appsmith exception methodThe signature has been properly updated to include the organizationId parameter, aligning with the feature flag context requirements.
59-63: Added new method variation without organization IDGood addition of a backward-compatible method that doesn't require an organization ID. This provides flexibility while still allowing the method to be feature flagged.
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCEImpl.java (4)
30-30: Updated CE implementation signature to include organizationIdMethod signature change properly aligns with the EE implementation, maintaining compatibility.
35-35: Empty implementation updated with organizationId parameterThe method signature is correctly updated while maintaining the empty implementation for the CE version.
38-38: Consistent parameter update for non-Appsmith exception methodMethod signature properly updated to match the interface change.
40-43: Added CE implementation for non-organizationId methodAppropriate implementation of the new method that maintains consistency with the CE implementation pattern.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (4)
504-505: Updated variable name to reflect new Mono-wrapped return typeGood change to rename the variable to clearly indicate it now contains a Mono of list instead of just a list.
508-511: Modified processing of inherited domains listThe code now correctly handles the Mono-wrapped list by using flatMapMany to extract and process the individual Mono items.
608-608: Enhanced return type for better reactive flowChanged the method signature to return
Mono<List<Mono<Void>>>instead of justList<Mono<Void>>, which improves the reactive nature of the code by encapsulating the entire list in a Mono.
639-639: Updated return statement to match new method signatureCorrectly modified to return a Mono-wrapped list to match the updated method signature.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (7)
11-11: No concerns with new import statement.
The inclusion of theUserdomain import is consistent with usage in subsequent lines.
17-17: Import addition looks good.
Bringing inReactiveContextUtilsis necessary for user context retrieval. No issues here.
123-125: Validate user organization ID.
While fetching the organization ID from the current user, consider sanity checks for cases whereuser.getOrganizationId()might be null or unavailable to prevent potential null pointer issues.
162-165: Consistent approach to user retrieval.
This mirrors the logic above for retrieving the current user’s organization ID. Verify presence of a valid organization ID if not implicitly guaranteed upstream.
186-186: Good caching of current user.
Caching the Mono here helps avoid repeated calls toReactiveContextUtils.getCurrentUser()in subsequent flatMap operations.
188-191: Appropriate user-based permission retrieval.
This code is aligned with the approach used elsewhere, ensuring that thepagePermissioncall is scoped to the user's org. Just be mindful of error handling if organization ID is ever missing or invalid.
251-278: Ensure non-null org ID in subsequent calls.
The chained usage ofcurrentUserMonoto fetch pages withpagePermission.getActionCreatePermission(user.getOrganizationId())can cause issues if the organization ID is null. Confirm that all calling contexts guarantee a valid org.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.java (5)
29-29: No issues with the new import.
TheUserdomain class is now required for user context. This aligns with changes below.
37-37: ReactiveContextUtils import confirmed.
The import is appropriate for user context retrieval logic added below.
427-429: Zipping in user context.
Including the current user in the zipped Mono pipeline allows for more contextual checks (e.g., retrieving organization ID). Make sure you have fallback handling for null user scenarios if needed.
433-433: Extracting user from tuple.
This is a reasonable approach, but consider verifying thattuple.getT3()is always non-null to avoid NPE.
452-455: Passing organization info to integrationDTO.
Ensuring the user’s org ID is injected here provides better scoping. Double-check thatcurrentUser.getOrganizationId()cannot be null to avoid feature-flag or permission-check anomalies.app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java (5)
47-47: Method access without checked exception.
Removing thethrows IllegalAccessExceptionclause keeps your signature clean, but confirm that the underlying reflection calls won't produce other unchecked scenarios.
69-70: Comment clarifies new org ID extraction.
Explicitly noting that you’re retrievingorganizationIdfrom method arguments improves readability. No functional concerns here.
72-78: Mandatory org ID check.
Enforcing a non-reactive method to have a validorganizationIdis prudent. Just ensure upstream logic will always pass a non-empty org ID or handle thrown exceptions gracefully.
81-81: Feature flag invocation with org ID.
CallingisFeatureFlagEnabled(flagName, organizationId)ensures that the correct org-based feature flags are used. This is a solid approach.
121-122: Check cached org-based flags.
Grabbing feature flags fromfeatureFlagService.getCachedOrganizationFeatureFlags(organizationId)is standard. Consider verifying thatorganizationIdis valid or raising an explicit error if it’s missing in the cache.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java (3)
119-125: ValidateorganizationIdbefore retrieving page creation permission
IforganizationIdis invalid or null,getPageCreatePermission(organizationId)may allow unintended access. Consider adding a null or empty check to safeguard.
127-132: Check organization scope for action creation
Make sureorganizationIdcorresponds to the correct workspace context and that null/empty values are handled to avoid misapplied permissions.
134-139: Ensure datasource creation aligns with correct organization
Confirm that the providedorganizationIdis valid and relevant, so users do not accidentally create datasources in an unintended organization.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (10)
40-40: New import forReactiveContextUtils
No issues. Ensure usage remains consistent with existing session retrieval patterns.
162-166: Validateuser.getOrganizationId()is non-null before permission checks
UsingapplicationPermission.getPageCreatePermission(user.getOrganizationId())assumes a single valid org ID. Consider adding checks for multi-org or null edge cases.
479-483: Confirm correctorganizationIdfor workspace creation permissions
.findById(workspaceId, workspacePermission.getApplicationCreatePermission(user.getOrganizationId()))might be mismatched if a user belongs to multiple orgs.
513-516: Check default org usage for application deletion
Relying oncurrentUser.getOrganizationId()can be tricky if multiple orgs are possible. Confirm the application truly belongs to that org.
527-529: Review multi-org scenario with applicationzipWithuser context
Ensure the org ID fromuser.getOrganizationId()matches the application’s actual workspace, preventing cross-organization deletion issues.
534-535: Prevent cross-org repository lookups
PassingapplicationPermission.getDeletePermission(user.getOrganizationId())tofindAllApplicationsByBaseApplicationIdcould fetch out-of-scope apps if the org reference differs.
567-578: Unify organization checks during archiving
Repeated usage oforgIdfromcurrentUserOrgIdMonomight fail if no default org is set. Add fallback or error-handling to avoid partial resource archivals.
923-931: Confirm correct org ID for page deletion
deleteUnpublishedPagecallspagePermission.getDeletePermission(organizationId). Validate that the app you’re deleting indeed belongs to this organization.
1000-1001: Minor change: reformat or indentation
No functional changes appear to be introduced. Nothing further required.
1500-1506: Check organization alignment for datasource creation
datasourcePermission.getActionCreatePermission(user.getOrganizationId())could incorrectly allow creation if the org context is not carefully managed.
...in/java/com/appsmith/server/imports/internal/artifactbased/ArtifactBasedImportServiceCE.java
Show resolved
Hide resolved
...smith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java
Show resolved
Hide resolved
Failed server tests
|
|
Closing this as we have decided to opt for another solution #39594 |
1 participant
Description
PR to add organizationId param for the non-reactive methods where @FeatureFlagged is used. With the multi org setup as we can't rely on default organization, we are now checking the organizationId in FeatureFlagInvokerAspect to correctly figure out the organization flags.
/test all
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13662083853
Commit: 2b26509
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 04 Mar 2025 21:31:18 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Tests