Skip to content

Conversation

subrata71
Copy link
Contributor

@subrata71 subrata71 commented Oct 9, 2025

Description

Slack Thread
EE Counterpart PR: https://github.com/appsmithorg/appsmith-ee/pull/8242

Fixes #Issue Number
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/18408366993
Commit: 698d879
Cypress dashboard.
Tags: @tag.All
Spec:


Fri, 10 Oct 2025 15:02:32 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Breaking Changes

    • Config REST endpoints for fetching/updating by name and ACL-guarded config update paths have been removed; clients relying on those endpoints or permissioned fetch/update should adjust.
  • Bug Fixes

    • Simplified config access surface to reduce permission-related complexity and potential inconsistencies.

@subrata71 subrata71 self-assigned this Oct 9, 2025
@subrata71 subrata71 added the ok-to-test Required label for CI label Oct 9, 2025
@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Oct 9, 2025
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Walkthrough

Removed ACL-guarded config API surface and REST controllers: three ACL-related methods were deleted from ConfigServiceCE and its implementation; the ConfigController and ConfigControllerCE REST classes were removed, eliminating GET/PUT endpoints and ACL-based update/get paths.

Changes

Cohort / File(s) Change Summary
Service implementation (ACL methods removed)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConfigServiceCEImpl.java
Removed ACL-aware methods: updateByName(Config), getByName(String, AclPermission), and getByNameAsUser(String, User, AclPermission). Implementation logic for permission-guarded lookup and update was deleted.
Service interface (API surface reduced)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConfigServiceCE.java
Removed corresponding method declarations from the ConfigServiceCE interface: updateByName(Config), getByName(String, AclPermission), and getByNameAsUser(String, User, AclPermission). Imports for AclPermission and User removed.
Controller removals
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConfigController.java, app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ConfigControllerCE.java
Deleted ConfigController and ConfigControllerCE classes, including their constructors and REST endpoint methods (GET/PUT by name). Routing under Url.CONFIG_URL for those endpoints removed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  rect rgba(200,230,255,0.4)
    Note right of Client: Previous flow (before removals)
    Client->>ConfigControllerCE: GET /config/name/{name}
    ConfigControllerCE->>ConfigServiceCEImpl: getByName(name, permission?)
    ConfigServiceCEImpl->>ConfigRepository: findByName(name[, permission])
    ConfigRepository-->>ConfigServiceCEImpl: Config
    ConfigServiceCEImpl-->>ConfigControllerCE: ResponseDTO<Config>
    ConfigControllerCE-->>Client: 200 OK
  end

  rect rgba(255,230,200,0.4)
    Note right of Client: New flow (after removals)
    Client->>Server: GET/PUT /config/name/{name}  (no controller)
    Note over Server: Endpoints removed — request will not be handled here
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A few ACL gates walked away,
Controllers closed for a cleaner day.
Service surface trimmed and light,
Calls once guarded lost their right.
Repo stays waiting, quiet and bright.

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title claims to enforce permission when updating instance config, but the changes actually remove ACL-based config retrieval and update methods along with their controllers, so it does not accurately reflect the main change. Please update the title to clearly summarize the removal of ACL-based config APIs and controllers, for example “chore: remove ACL-based config retrieval and update endpoints,” so it accurately reflects the primary change.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ⚠️ Warning The pull request description largely mirrors the template without providing required details such as a concrete “Fixes” issue reference, a TL;DR summary or clear motivation and context for the changes, and any listed dependencies, so it does not meet the repository’s pull request description requirements. Please update the description by replacing the placeholder “Fixes #Issue Number” or “Fixes Issue URL” with the actual issue reference, add a concise summary or TL;DR, include motivation and context for this change, list any dependencies, and link any relevant documents as outlined in the template.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch enforce-permission-in-instance-config

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@subrata71 subrata71 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Oct 9, 2025
@subrata71 subrata71 changed the title chore: Enforce permission while reading or updating instance-config chore: Enforce permission while updating instance-config Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants