fix(access): require MANAGE_CONNECTIONS to create a connection; hide admin surfaces

POST /connections had no authorization at all — it went straight to
test-and-save, so any authenticated user could create, then edit and delete,
their own connection. Verified live: as DATA_ENGINEER the request returned 200
and the row persisted with owner_username = analyst. An earlier check reported
403 only because the payload was malformed, so validation rejected it before
authorization was ever reached; the endpoint was open.

Creation is not scoped to an existing connection id, so none of the
assertCanManage*Connection* helpers apply. assertCanManageConnections() is
permission-based rather than admin-only, so DBA — which holds MANAGE_CONNECTIONS
by design — keeps working, as does any custom role granted it. It honours
security.auth.enabled like every other guard here, so the dev-mode bypass stays
coherent.

UI: Settings and Connections are administrative surfaces and are hidden from
Developer and Data Engineer. Enforced inside SettingsModal and
ManageConnectionsModal, not only at the call sites — both are opened from several
places, and gating each entry point separately means the next new one silently
reopens the hole. Hiding Settings also removes MCP tokens from those roles, which
is the intended trade.

Nav gating moves from a minimum-role table to per-section permissions, so custom
roles and admin overrides take effect without a code change.

Author: notSumit25

CLAUDE.md

@@ -210,6 +210,99 @@ returns a number).
 4. **Tooltips**: Always use `HelpTooltip` component, never plain `title` attributes.
 5. **Design**: Minimal black/white/grey palette, Inter font, subtle transitions. See UX guidelines in full CLAUDE.md.
 
+### Roles, Permissions & Custom Roles
+
+Roles are **not a hierarchy**. The old model ranked `DEVELOPER < ADMIN` and compared
+`ordinal()`; the shipped roles deliberately overlap without nesting, so an ordering
+comparison has no meaning and `Role.isAtLeast` is gone.
+
+| Role | Sections | Notes |
+|---|---|---|
+| `ADMIN` | everything | Fixed point: holds **every** permission; overrides against it are refused, so the last admin cannot be locked out of user management. |
+| `DBA` | all menus + connection settings | **No** user creation / invite codes / role management. |
+| `DATA_ENGINEER` | Agent, Dashboards, Editor | No Digest, no Performance. |
+| `DEVELOPER` | Agent, Digest, Dashboards, Performance, Editor | No connection settings. |
+| custom | whatever an admin ticks | `custom_roles` rows; the `code` is written to `users.role`. |
+
+- **Permissions are the unit of authorization.** `Permission` carries the built-in roles
+  that hold it by default (`defaultRoles`); one `VIEW_*` permission per sidebar section
+  (`VIEW_AGENT`, `VIEW_DASHBOARDS`, `VIEW_DIGEST`, `VIEW_BRAIN`, `VIEW_PERFORMANCE`,
+  `VIEW_EDITOR`). The frontend gates nav on those codes (`SECTION_PERMISSION` in
+  `src/lib/features.js`), not on a minimum role.
+- **A "role code" is either a built-in `Role` name or a `CustomRole.code`** — they share
+  the `users.role` namespace, so `CustomRoleService` refuses a code colliding with a
+  built-in one. `Role.fromString` returns **null** for anything unrecognised instead of
+  collapsing to DEVELOPER: mapping a custom role onto a built-in one would hand its
+  holders the wrong permissions. Use `PermissionService.getEffectivePermissions(roleCode)`
+  — `User.getRoleEnum()` is null for a custom role and `Role.getPermissions()` skips
+  overrides.
+- **Every token-minting path must resolve by role code.** `AuthSessionService`,
+  `PasswordlessAuthService`, `AuthInternalController`, `CustomUserDetailsService` and the
+  `/auth/me` payload all use `user.getRoleCode()` + `PermissionService`; `JwtUtil` gained
+  a `String roleCode` overload for exactly this. A `Role`-typed path cannot represent a
+  custom role, so a custom-role user would silently get the wrong claim.
+- **An unknown role code grants nothing** rather than falling back — a deleted custom role
+  must not become silent Developer access. Deleting a custom role is refused while any
+  user still holds it.
+- `RolePermissionOverride.role` is now a role-code **string** (same column), so overrides
+  work for custom roles too. Built-in role permission sets are code, not data: the API
+  refuses to edit them directly and points at overrides instead, so an admin's change
+  survives an upgrade.
+
+### Connection access levels & the create-connection guard
+
+- **There is one access level.** `ConnectionAccessLevel.CHAT_EDITOR` is `@Deprecated` and
+  retained only so pre-existing rows parse; `fromString` folds it (and a blank value) into
+  `FULL_CONTENT`, and `ConnectionAccessService.resolveAccess` returns `FULL_CONTENT` for
+  **every** grant. Assigning a connection therefore implies content access — no migration
+  was needed, legacy rows upgrade themselves on read. The "Full Access" / "Chat + Editor"
+  badges are gone; only Owner/Admin are surfaced.
+- **`AccessControlServiceTest` cannot prove anything about this.** It stubs
+  `resolveAccess` to return a fixed `EffectiveConnectionAccess`, so its CHAT_EDITOR case
+  passes vacuously no matter what the resolver does. `ConnectionAccessLevelCollapseTest`
+  exercises the real path — add coverage there, not to the stubbed test.
+- **`POST /connections` had no authorization at all.** It went straight to test-and-save,
+  so any authenticated user could create — then edit and delete — their own connection
+  (verified live: the row persisted with `owner_username = analyst` for a DATA_ENGINEER).
+  Hiding the sidebar button is not a control. It now calls
+  `accessControlService.assertCanManageConnections()`, which is **permission-based, not
+  admin-only**, so DBA and any custom role holding `MANAGE_CONNECTIONS` still work.
+  Creation is not scoped to a connection id, so none of the `assertCanManage*Connection*`
+  helpers apply — a new unscoped endpoint needs this guard explicitly.
+- **Settings and Connections are admin surfaces in the UI.** `SettingsModal` and
+  `ManageConnectionsModal` each refuse to render without the relevant permission, enforced
+  *inside* the component rather than only at the call site: both are opened from several
+  places, and gating each entry point separately means the next one silently reopens the
+  hole. Hiding Settings also removes MCP tokens from those roles — that is intended.
+
+### Dashboard workspaces
+
+`DashboardWorkspace` groups dashboards within one connection and carries its own member
+list (`DashboardWorkspaceMember`, keyed by **username** to match `connection_access_grant`
+so "View as" resolves membership as the target user).
+
+- **The rule is an AND, and it only ever narrows.** Connection access is checked first and
+  unchanged (`assertCanReadConnectionContent`); workspace membership is an *additional*
+  gate. Adding someone to a workspace can never grant them a connection they were not
+  already given. `saved_dashboards.workspace_id` is nullable — NULL means "not grouped",
+  governed purely by the connection ACL exactly as before.
+- Admins bypass the membership half, matching how they already bypass connection grants.
+- **Non-membership reports 404, not 403** — a user outside the workspace must not learn
+  the dashboard exists.
+- **Deleting a workspace detaches its dashboards, never deletes them** (the FK is
+  deliberately non-cascading). Removing the last MANAGER is refused, otherwise the
+  workspace could never be changed again by anyone but an admin.
+- `DashboardWorkspaceService.filterReadable` resolves a whole list in one membership
+  query; use it for any new dashboard-list endpoint rather than checking per row.
+- **`/saved-dashboards` had no connection authorization at all** before this change —
+  create, list, get, update and delete took a caller-supplied `connectionId`/id and
+  checked nothing, so any authenticated user could read every dashboard on every
+  connection (verified live against a running install, not inferred). All of them now
+  assert connection access *and* the workspace gate; `DashboardAlertController` does the
+  same through its single `requireDashboard` choke point. This is the same
+  "authentication is not authorization" trap `BrainController` documents — there is still
+  no filter doing it for you.
+
 ### Admin profile switch
 Admins can **View as** a sub-user from the top-right of the home layout (`ProfileSwitch`) to verify connection ACLs, chat/editor policies, and role-gated nav.
 

backend/src/main/java/com/dbaagent/controller/ConnectionController.java

@@ -189,6 +189,12 @@ private ConnectionRequest mergeTestRequest(ConnectionRequest saved, ConnectionRe
     public ResponseEntity<Map<String, Object>> saveConnection(@RequestBody ConnectionRequest request) {
         Map<String, Object> response = new HashMap<>();
         try {
+            // Creating a connection is not scoped to an existing connection id, so none of
+            // the assertCanManage*Connection* checks apply here — this endpoint had no
+            // authorization at all, and any authenticated user could add (then edit and
+            // delete) their own connection. Hiding the Connections button did not stop it.
+            accessControlService.assertCanManageConnections();
+
             // Test connection with privilege checks
             ConnectionTestResult result = connectionService.testConnectionWithPrivileges(request);
 

backend/src/main/java/com/dbaagent/service/security/AccessControlService.java

@@ -4,6 +4,7 @@
 import com.dbaagent.model.Chat;
 import com.dbaagent.model.ChatFeedback;
 import com.dbaagent.model.EffectiveConnectionAccess;
+import com.dbaagent.model.Permission;
 import com.dbaagent.repository.AnalysisHistoryRepository;
 import com.dbaagent.repository.ChatFeedbackRepository;
 import com.dbaagent.repository.ChatRepository;
@@ -181,6 +182,52 @@ public String requireCurrentUsername() {
         return username;
     }
 
+    /**
+     * Assert the caller may create a database connection.
+     *
+     * <p>Creating a connection is not scoped to an existing connection, so none of the
+     * {@code assertCanManage*Connection*} checks apply — there is no id to resolve
+     * access against yet. Without this, {@code POST /connections} had no authorization
+     * at all: a Developer or Data Engineer could create, then edit and delete, their own
+     * connection (verified live against a running install — the row persisted with
+     * {@code owner_username = analyst}). Hiding the Connections button only hid the
+     * button.
+     *
+     * <p>Permission-based rather than {@code isCurrentUserAdmin()} so DBA — which holds
+     * MANAGE_CONNECTIONS by design — keeps working, and so an admin-defined custom role
+     * granting that permission behaves consistently.
+     */
+    public void assertCanManageConnections() {
+        if (!authEnabled) {
+            return;
+        }
+        if (!hasPermission(Permission.MANAGE_CONNECTIONS)) {
+            throw new ResponseStatusException(FORBIDDEN, "You do not have permission to manage connections");
+        }
+    }
+
+    /**
+     * Whether the current principal carries a permission authority.
+     *
+     * <p>{@code CustomUserDetailsService} stamps every effective permission onto the
+     * authentication as a plain authority alongside {@code ROLE_<code>}, so this reads
+     * the already-resolved set (overrides and custom roles included) without a lookup.
+     */
+    public boolean hasPermission(Permission permission) {
+        if (permission == null) {
+            return false;
+        }
+        if (isCurrentUserAdmin()) {
+            return true;
+        }
+        Authentication authentication = currentAuthentication();
+        if (authentication == null || !authentication.isAuthenticated()) {
+            return false;
+        }
+        return authentication.getAuthorities().stream()
+            .anyMatch(authority -> permission.name().equals(authority.getAuthority()));
+    }
+
     public boolean isCurrentUserAdmin() {
         if (ImpersonationContext.isActive()) {
             return ImpersonationContext.current()

backend/src/test/java/com/dbaagent/service/security/ConnectionCreateAuthorizationTest.java

@@ -0,0 +1,142 @@
+package com.dbaagent.service.security;
+
+import com.dbaagent.model.Permission;
+import com.dbaagent.repository.AnalysisHistoryRepository;
+import com.dbaagent.repository.ChatFeedbackRepository;
+import com.dbaagent.repository.ChatRepository;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
+import org.springframework.security.core.authority.SimpleGrantedAuthority;
+import org.springframework.security.core.context.SecurityContextHolder;
+import org.springframework.test.util.ReflectionTestUtils;
+import org.springframework.web.server.ResponseStatusException;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.mockito.Mockito.mock;
+
+/**
+ * Creating a connection requires MANAGE_CONNECTIONS.
+ *
+ * <p>{@code POST /connections} previously had no authorization whatsoever: it went
+ * straight to test-and-save. A Data Engineer could create a connection and then edit and
+ * delete it — verified against a running install, where the row persisted with
+ * {@code owner_username = analyst}. Only the sidebar button was hidden, which is not a
+ * control.
+ *
+ * <p>The permission is read from the principal's granted authorities, which
+ * {@code CustomUserDetailsService} stamps from the fully-resolved effective permission
+ * set — so overrides and custom roles are honoured here without a second lookup.
+ */
+class ConnectionCreateAuthorizationTest {
+
+    private AccessControlService accessControlService;
+
+    @BeforeEach
+    void setUp() {
+        accessControlService = new AccessControlService(
+            mock(ConnectionAccessService.class),
+            mock(ChatRepository.class),
+            mock(ChatFeedbackRepository.class),
+            mock(AnalysisHistoryRepository.class)
+        );
+        // Every real deployment runs with auth on; the dev-mode bypass is covered below.
+        ReflectionTestUtils.setField(accessControlService, "authEnabled", true);
+    }
+
+    @AfterEach
+    void tearDown() {
+        SecurityContextHolder.clearContext();
+    }
+
+    private void authenticateAs(String username, String roleCode, String... permissions) {
+        var authorities = new java.util.ArrayList<SimpleGrantedAuthority>();
+        authorities.add(new SimpleGrantedAuthority("ROLE_" + roleCode));
+        for (String p : permissions) {
+            authorities.add(new SimpleGrantedAuthority(p));
+        }
+        SecurityContextHolder.getContext().setAuthentication(
+            new UsernamePasswordAuthenticationToken(username, null, authorities));
+    }
+
+    @Test
+    @DisplayName("A Developer cannot create a connection")
+    void developerRefused() {
+        // Developer's effective set has no MANAGE_CONNECTIONS (see PermissionResolutionTest).
+        authenticateAs("analyst", "DEVELOPER",
+            Permission.VIEW_AGENT.name(), Permission.VIEW_DASHBOARDS.name(),
+            Permission.VIEW_EDITOR.name(), Permission.EXECUTE_QUERIES.name());
+
+        assertThatThrownBy(() -> accessControlService.assertCanManageConnections())
+            .isInstanceOf(ResponseStatusException.class)
+            .hasMessageContaining("403");
+        assertThat(accessControlService.hasPermission(Permission.MANAGE_CONNECTIONS)).isFalse();
+    }
+
+    @Test
+    @DisplayName("A Data Engineer cannot create a connection")
+    void dataEngineerRefused() {
+        authenticateAs("analyst", "DATA_ENGINEER",
+            Permission.VIEW_AGENT.name(), Permission.VIEW_DASHBOARDS.name(),
+            Permission.VIEW_EDITOR.name());
+
+        assertThatThrownBy(() -> accessControlService.assertCanManageConnections())
+            .isInstanceOf(ResponseStatusException.class)
+            .hasMessageContaining("403");
+    }
+
+    @Test
+    @DisplayName("A DBA can create a connection — the check is permission-based, not admin-only")
+    void dbaAllowed() {
+        // DBA holds MANAGE_CONNECTIONS by design but is not an admin, so an
+        // isCurrentUserAdmin() check here would have broken it.
+        authenticateAs("dba-user", "DBA", Permission.MANAGE_CONNECTIONS.name());
+
+        assertDoesNotThrow(() -> accessControlService.assertCanManageConnections());
+    }
+
+    @Test
+    @DisplayName("An admin can create a connection")
+    void adminAllowed() {
+        authenticateAs("admin", "ADMIN");
+
+        assertDoesNotThrow(() -> accessControlService.assertCanManageConnections());
+        assertThat(accessControlService.hasPermission(Permission.MANAGE_CONNECTIONS)).isTrue();
+    }
+
+    @Test
+    @DisplayName("A custom role granted MANAGE_CONNECTIONS can create a connection")
+    void customRoleWithPermissionAllowed() {
+        authenticateAs("analyst", "PLATFORM_ENG", Permission.MANAGE_CONNECTIONS.name());
+
+        assertDoesNotThrow(() -> accessControlService.assertCanManageConnections());
+    }
+
+    @Test
+    @DisplayName("An unauthenticated caller cannot create a connection")
+    void anonymousRefused() {
+        SecurityContextHolder.clearContext();
+
+        assertThatThrownBy(() -> accessControlService.assertCanManageConnections())
+            .isInstanceOf(ResponseStatusException.class)
+            .hasMessageContaining("403");
+    }
+
+    @Test
+    @DisplayName("With auth disabled the check is a no-op, matching every other guard here")
+    void devModeBypass() {
+        // Consistency matters: requireCurrentUsername/isCurrentUserAdmin both honour this
+        // flag, and a guard that ignored it would switch connection creation off in the
+        // documented dev-mode bypass instead of opening it.
+        ReflectionTestUtils.setField(accessControlService, "authEnabled", false);
+        SecurityContextHolder.clearContext();
+
+        assertDoesNotThrow(() -> accessControlService.assertCanManageConnections());
+    }
+}

src/components/ManageConnectionsModal.js

@@ -17,6 +17,7 @@ import ConnectionSlowQueryConfig from "./ConnectionSlowQueryConfig";
 import SlowQuerySourceModal from "./SlowQuerySourceModal";
 import { connectionAPI, brainAPI } from "@/lib/api/client";
 import { useAuth } from "@/hooks/useAuth";
+import { PERMISSIONS } from "@/lib/permissions";
 import { getConnectionAccessBadge, getConnectionAccessLabel } from "@/lib/features";
 
 export default function ManageConnectionsModal({
@@ -25,7 +26,7 @@ export default function ManageConnectionsModal({
   onConnectionDeleted,
   onConnectionSaved,
 }) {
-  const { isAdmin } = useAuth();
+  const { isAdmin, hasPermission } = useAuth();
   const [connections, setConnections] = useState([]);
   const [loading, setLoading] = useState(false);
   const [deleteConfirmId, setDeleteConfirmId] = useState(null);
@@ -118,6 +119,12 @@ export default function ManageConnectionsModal({
 
   if (!isOpen) return null;
 
+  // Enforced here rather than only at the call sites: this modal adds, edits and deletes
+  // database connections, and it is opened from the sidebar, the Agent view and the user
+  // menu. Gating each entry point separately means the next new one silently reopens the
+  // hole. The backend already 403s these writes; this keeps the UI honest about it.
+  if (!hasPermission(PERMISSIONS.MANAGE_CONNECTIONS)) return null;
+
   return (
     <div className={styles.overlay} onClick={onClose}>
       <div className={styles.modal} onClick={(e) => e.stopPropagation()}>