From 9577e9d38537788d30a3d04249851f18419679bf Mon Sep 17 00:00:00 2001 From: Venkat SF Date: Wed, 9 Sep 2026 06:22:44 +0000 Subject: [PATCH 1/2] feat: pin a connection as your per-user default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a pin toggle in Manage Connections and in the sidebar connection switcher. A pinned connection is the one DeepSQL opens on every load. The pin is per user, not a flag on the connection. A connection can be shared through connection_access_grant, so a column on database_connection would let one person's choice decide what everyone else opens on — and shared connections are canManageConfig=false for their recipients, so exactly the people who most want a default could not set one. One pin per user, enforced by a unique constraint on connection_pin.username; pinning a second connection moves the row rather than adding one. PUT|DELETE /connections/{id}/pin are gated on assertCanUseConnection, not assertCanManageConnectionConfig: choosing where you land is a preference, not a change to the connection. GET /connections carries the pinned flag per caller, so no surface needs a second request, and deleteConnection clears every pin on the connection alongside its grants. The pin has to beat an already-selected connection, not just an empty one. useDashboardStore persists connectionId, so after a reload something is always selected and the existing auto-select never ran. useConnectionManager now applies the pin once per page load, tracked at module scope rather than in a ref — the hook is called from a dozen sections, and a per-instance guard would let a later-mounted section yank the user back to the pin after they deliberately switched. The sidebar toggle is there because Manage Connections needs MANAGE_CONNECTIONS to open at all; without it a Developer or Data Engineer, who typically holds exactly one granted connection, would have no way to set a default. ConnectionScopedAuthorizationSafetyTest now flags GET /connections, because the handler resolves the caller's pinned connection id and the scanner matches (?i)connection_?id anywhere in a handler body. That endpoint takes no arguments at all, so it is exempted in AUTHORIZED_ELSEWHERE and connectionListingTakesNoCallerSuppliedId re-derives the claim, so the exemption cannot rot into cover for a real gap. The scanner was not weakened and no meaningless assert was added. Verified against a live backend on a throwaway stack: ddl-auto creates connection_pin with its unique index; pinning a second connection flips the first; unpinning a non-pinned connection leaves the real pin alone; deleting a connection clears its pin; a DEVELOPER with no grant gets 403 (not 500); a DEVELOPER holding only a grant pins successfully and their pin does not appear on the admin's list. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 45 ++++++ .../controller/ConnectionController.java | 58 +++++++- .../dto/ConnectionSummaryResponse.java | 9 ++ .../com/dbaagent/model/ConnectionPin.java | 70 ++++++++++ .../repository/ConnectionPinRepository.java | 27 ++++ .../service/ConnectionPinService.java | 96 +++++++++++++ .../migration/V120__create_connection_pin.sql | 24 ++++ .../controller/ConnectionControllerTest.java | 73 +++++++++- ...nnectionScopedAuthorizationSafetyTest.java | 32 ++++- .../service/ConnectionPinServiceTest.java | 131 ++++++++++++++++++ src/components/ManageConnectionsModal.js | 64 +++++++++ .../ManageConnectionsModal.module.css | 53 +++++++ src/components/layout/AppSidebar.jsx | 54 ++++++-- src/components/layout/AppSidebar.module.css | 51 ++++++- src/hooks/useAuth.jsx | 5 + src/lib/api/client.js | 15 ++ src/lib/hooks/queries/index.js | 1 + src/lib/hooks/queries/useConnections.js | 21 +++ src/lib/hooks/useConnectionManager.js | 73 ++++++++-- 19 files changed, 864 insertions(+), 38 deletions(-) create mode 100644 backend/src/main/java/com/dbaagent/model/ConnectionPin.java create mode 100644 backend/src/main/java/com/dbaagent/repository/ConnectionPinRepository.java create mode 100644 backend/src/main/java/com/dbaagent/service/ConnectionPinService.java create mode 100644 backend/src/main/resources/db/migration/V120__create_connection_pin.sql create mode 100644 backend/src/test/java/com/dbaagent/service/ConnectionPinServiceTest.java diff --git a/CLAUDE.md b/CLAUDE.md index b6b1011..2b4eb0b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -498,6 +498,51 @@ so "View as" resolves membership as the target user). "authentication is not authorization" trap `BrainController` documents — there is still no filter doing it for you. +### Default connection (pinning) + +A user can pin one connection as their default, from the pin column in **Manage +Connections** or the pin toggle in the sidebar connection switcher. + +- **The pin is per user, not per connection.** `connection_pin` keys on username with a + unique constraint (`V120__create_connection_pin.sql`; applied by `ddl-auto` from the + `ConnectionPin` entity — verified on a scratch database, the table and its unique index + are created on boot). A column on `database_connection` would have been wrong twice + over: a connection shared through `connection_access_grant` would let one user's choice + decide what everyone else opens on, and a shared connection is `canManageConfig=false` + for its recipients, so exactly the people who most want a default could not set one. +- **One pin per user is the point.** `ConnectionPinService.pin` moves the existing row + rather than inserting a second; the unique constraint is the backstop, and a losing + concurrent insert re-reads and updates instead of surfacing a 500. +- **`PUT|DELETE /connections/{id}/pin` are gated on `assertCanUseConnection`**, not + `assertCanManageConnectionConfig` — choosing where you land is a preference, not a + change to the connection. Verified live: a DEVELOPER holding only a grant on a + connection (`canManageConfig: false`) pins it and gets 200, while the same user pinning + a connection they hold no grant on gets **403, not 500** — the `ResponseStatusException` + rethrow before the catch-all is doing its job. +- **Unpin is scoped to the connection named.** A stale click in a background tab must not + clear a pin the user has since moved elsewhere. +- **`GET /connections` carries `pinned` per caller**, so no surface needs a second + request, and two users listing the same shared connection see different values — + confirmed live. `deleteConnection` clears every pin on the connection alongside its + grants. +- **`ConnectionScopedAuthorizationSafetyTest` flags `GET /connections` now**, because the + handler resolves the caller's *pinned* connection id and the scanner matches + `(?i)connection_?id` anywhere in a handler body. That endpoint takes no arguments at all + — it returns whatever `getConnectionsForUser(username, isAdmin)` gives — so it is in + `AUTHORIZED_ELSEWHERE`, and `connectionListingTakesNoCallerSuppliedId` re-derives that + claim so the exemption cannot rot into cover for a real gap. Do not resolve this by + adding a meaningless assert, and do not weaken the scanner. +- **The pin must beat an already-selected connection, not just an empty one.** + `useDashboardStore` persists `connectionId`, so after a reload something is always + selected — the original auto-select ran only when nothing was. `useConnectionManager` + therefore applies the pin once per page load (`pinAppliedThisLoad`, module scope, reset + by `resetConnectionPinApplied()` in the auth reset). Module scope and not a ref: the + hook is called from a dozen sections, and a per-instance guard would let a + later-mounted section yank the user back to the pin after they deliberately switched. + Switching mid-session still sticks; the pin re-applies on the next load. +- Pinned connections sort first in `useConnectionManager`, so every consumer — the sidebar + switcher included — shows the default at the top. + ### 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. diff --git a/backend/src/main/java/com/dbaagent/controller/ConnectionController.java b/backend/src/main/java/com/dbaagent/controller/ConnectionController.java index 1a88e1b..69c699d 100644 --- a/backend/src/main/java/com/dbaagent/controller/ConnectionController.java +++ b/backend/src/main/java/com/dbaagent/controller/ConnectionController.java @@ -9,6 +9,7 @@ import com.dbaagent.repository.ConnectionInitHistoryRepository; import com.dbaagent.repository.ConnectionInitStatusRepository; import com.dbaagent.repository.SchemaDocumentationRepository; +import com.dbaagent.service.ConnectionPinService; import com.dbaagent.service.ConnectionService; import com.dbaagent.service.scheduler.BrainInitSchedulerService; import com.dbaagent.service.scheduler.BrainJobsService; @@ -45,6 +46,7 @@ public class ConnectionController { private final AccessControlService accessControlService; private final ConnectionAccessService connectionAccessService; private final com.dbaagent.repository.ConnectionAccessGrantRepository connectionAccessGrantRepository; + private final ConnectionPinService connectionPinService; @PostMapping("/test") public ResponseEntity> testConnection(@RequestBody ConnectionRequest request) { @@ -429,8 +431,10 @@ public ResponseEntity> getAllConnections() { String username = accessControlService.getCurrentUsername(); boolean isAdmin = accessControlService.isCurrentUserAdmin(); List connections = credentialService.getConnectionsForUser(username, isAdmin); + // One lookup for the whole list rather than one per row. + String pinnedId = connectionPinService.pinnedConnectionId(username).orElse(null); List decryptedConnections = connections.stream() - .map(conn -> toSummary(conn, username, isAdmin)) + .map(conn -> toSummary(conn, username, isAdmin, pinnedId)) .toList(); return ResponseEntity.ok(decryptedConnections); } catch (org.springframework.web.server.ResponseStatusException e) { @@ -440,6 +444,54 @@ public ResponseEntity> getAllConnections() { } } + /** + * Pin this connection as the caller's default, replacing any connection they had + * pinned before. + * + *

Gated on {@code assertCanUseConnection} rather than + * {@code assertCanManageConnectionConfig}: choosing which database you land on is a + * personal preference, not a change to the connection, and a shared connection is + * config-read-only for its recipients. Requiring manage rights would mean the people + * who most want a default — the ones who were granted exactly one connection — + * could not set one. + */ + @PutMapping("/{id}/pin") + public ResponseEntity> pinConnection(@PathVariable String id) { + Map response = new HashMap<>(); + try { + accessControlService.assertCanUseConnection(id); + connectionPinService.pin(accessControlService.requireCurrentUsername(), id); + response.put("success", true); + response.put("pinned", true); + return ResponseEntity.ok(response); + } catch (org.springframework.web.server.ResponseStatusException e) { + throw e; + } catch (Exception e) { + response.put("success", false); + response.put("message", "Failed to pin connection: " + e.getMessage()); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response); + } + } + + /** Clear the caller's default, if this connection is the one currently pinned. */ + @DeleteMapping("/{id}/pin") + public ResponseEntity> unpinConnection(@PathVariable String id) { + Map response = new HashMap<>(); + try { + accessControlService.assertCanUseConnection(id); + connectionPinService.unpin(accessControlService.requireCurrentUsername(), id); + response.put("success", true); + response.put("pinned", false); + return ResponseEntity.ok(response); + } catch (org.springframework.web.server.ResponseStatusException e) { + throw e; + } catch (Exception e) { + response.put("success", false); + response.put("message", "Failed to unpin connection: " + e.getMessage()); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response); + } + } + @DeleteMapping("/{id}") public ResponseEntity> deleteConnection(@PathVariable String id) { Map response = new HashMap<>(); @@ -447,6 +499,7 @@ public ResponseEntity> deleteConnection(@PathVariable String accessControlService.assertCanManageConnectionConfig(id); connectionService.closeConnectionPool(id); connectionAccessService.deleteAllGrantsForConnection(id); + connectionPinService.clearPinsForConnection(id); credentialService.deleteConnection(id); response.put("success", true); response.put("message", "Connection deleted successfully"); @@ -781,7 +834,7 @@ public ResponseEntity runBrainJob(@PathVariable String id, @PathVariable Stri } } - private ConnectionSummaryResponse toSummary(DatabaseConnection conn, String username, boolean isAdmin) { + private ConnectionSummaryResponse toSummary(DatabaseConnection conn, String username, boolean isAdmin, String pinnedConnectionId) { ConnectionSummaryResponse summary = new ConnectionSummaryResponse(); try { ConnectionRequest decrypted = credentialService.getDecryptedConnection(conn.getId()); @@ -821,6 +874,7 @@ private ConnectionSummaryResponse toSummary(DatabaseConnection conn, String user summary.setAccessLevel(resolved.getEffectiveAccess().name()); summary.setCanManageConfig(resolved.canManageConfig()); summary.setCanManageContent(resolved.canManageContent()); + summary.setPinned(conn.getId() != null && conn.getId().equals(pinnedConnectionId)); return summary; } diff --git a/backend/src/main/java/com/dbaagent/dto/ConnectionSummaryResponse.java b/backend/src/main/java/com/dbaagent/dto/ConnectionSummaryResponse.java index e6d0183..a971841 100644 --- a/backend/src/main/java/com/dbaagent/dto/ConnectionSummaryResponse.java +++ b/backend/src/main/java/com/dbaagent/dto/ConnectionSummaryResponse.java @@ -31,4 +31,13 @@ public class ConnectionSummaryResponse { private String accessLevel; private Boolean canManageConfig; private Boolean canManageContent; + + /** + * Whether the calling user has pinned this connection as their default. + * + *

Per caller, not per connection — two users listing the same shared connection + * see different values here. It rides the list response so the UI needs no second + * request to know which row carries the pin. + */ + private Boolean pinned; } diff --git a/backend/src/main/java/com/dbaagent/model/ConnectionPin.java b/backend/src/main/java/com/dbaagent/model/ConnectionPin.java new file mode 100644 index 0000000..10f6575 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/model/ConnectionPin.java @@ -0,0 +1,70 @@ +package com.dbaagent.model; + +import jakarta.persistence.*; +import lombok.AllArgsConstructor; +import lombok.Data; +import lombok.NoArgsConstructor; + +import java.time.LocalDateTime; + +/** + * One user's default database connection. + * + *

The pin is deliberately per user rather than a flag on + * {@link DatabaseConnection}. A connection can be shared with several people through + * {@code connection_access_grant}, and a default is a personal preference — one user's + * choice must not decide what everyone else opens on. A column on the connection row + * would also put the setting out of reach of exactly the people who need it: a shared + * connection has {@code canManageConfig == false} for its recipients, so they could + * never pin the connection they use every day. + * + *

At most one row per user — the unique constraint on {@code username} is what makes + * "always the default" true rather than merely intended. Pinning a second connection + * moves the pin instead of creating a second one; see + * {@code ConnectionPinService.pin}. + * + *

{@code connection_id} carries no foreign key, matching + * {@link ConnectionAccessGrant}. {@code ConnectionController.deleteConnection} clears + * pins alongside grants; a pin that outlives its connection is inert anyway, since the + * flag is only ever computed for connections the caller can already see. + */ +@Entity +@Table( + name = "connection_pin", + uniqueConstraints = @UniqueConstraint( + name = "ux_connection_pin_username", + columnNames = {"username"} + ) +) +@Data +@NoArgsConstructor +@AllArgsConstructor +public class ConnectionPin { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + private Long id; + + @Column(nullable = false) + private String username; + + @Column(name = "connection_id", nullable = false, length = 36) + private String connectionId; + + @Column(name = "created_at", nullable = false) + private LocalDateTime createdAt; + + @Column(name = "updated_at", nullable = false) + private LocalDateTime updatedAt; + + @PrePersist + void onCreate() { + LocalDateTime now = LocalDateTime.now(); + createdAt = now; + updatedAt = now; + } + + @PreUpdate + void onUpdate() { + updatedAt = LocalDateTime.now(); + } +} diff --git a/backend/src/main/java/com/dbaagent/repository/ConnectionPinRepository.java b/backend/src/main/java/com/dbaagent/repository/ConnectionPinRepository.java new file mode 100644 index 0000000..fe2c9be --- /dev/null +++ b/backend/src/main/java/com/dbaagent/repository/ConnectionPinRepository.java @@ -0,0 +1,27 @@ +package com.dbaagent.repository; + +import com.dbaagent.model.ConnectionPin; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.Query; +import org.springframework.transaction.annotation.Transactional; + +import java.util.Optional; + +public interface ConnectionPinRepository extends JpaRepository { + + /** + * Case-insensitive, matching how {@code ConnectionAccessGrantRepository} resolves + * usernames — a login that differs only in casing must not end up with a second pin + * the unique constraint cannot see. + */ + @Query("select p from ConnectionPin p where lower(p.username) = lower(?1)") + Optional findByUsernameIgnoreCase(String username); + + /** + * Derived deletes need their own transaction. Annotating a self-invoked caller does + * nothing — Spring proxies are bypassed by {@code this::} — which is the same trap + * {@code McpTokenRepository.deleteByUserId} documents. + */ + @Transactional + void deleteByConnectionId(String connectionId); +} diff --git a/backend/src/main/java/com/dbaagent/service/ConnectionPinService.java b/backend/src/main/java/com/dbaagent/service/ConnectionPinService.java new file mode 100644 index 0000000..712ab9a --- /dev/null +++ b/backend/src/main/java/com/dbaagent/service/ConnectionPinService.java @@ -0,0 +1,96 @@ +package com.dbaagent.service; + +import com.dbaagent.model.ConnectionPin; +import com.dbaagent.repository.ConnectionPinRepository; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; + +import java.util.Optional; + +/** + * The per-user default connection. + * + *

Callers are responsible for authorizing the connection first — this service takes an + * already-checked id. {@code ConnectionController} calls + * {@code assertCanReadConnectionContent} before every pin write, so a pin cannot be used + * to assert an interest in a connection the caller cannot see. + */ +@Service +@RequiredArgsConstructor +@Slf4j +public class ConnectionPinService { + + private final ConnectionPinRepository pinRepository; + + /** The connection this user opens by default, if they have chosen one. */ + public Optional pinnedConnectionId(String username) { + if (username == null || username.isBlank()) { + return Optional.empty(); + } + return pinRepository.findByUsernameIgnoreCase(username).map(ConnectionPin::getConnectionId); + } + + /** + * Make {@code connectionId} this user's default, replacing any previous pin. + * + *

Moving the existing row rather than inserting a second one is what keeps "the + * default" singular; the unique constraint on {@code username} is the backstop. Two + * pins racing in from different tabs can still collide on that constraint, so the + * loser re-reads and updates instead of surfacing a 500 for what is really a + * last-write-wins preference. + */ + @Transactional + public void pin(String username, String connectionId) { + if (username == null || username.isBlank() || connectionId == null || connectionId.isBlank()) { + return; + } + Optional existing = pinRepository.findByUsernameIgnoreCase(username); + if (existing.isPresent()) { + ConnectionPin pin = existing.get(); + if (connectionId.equals(pin.getConnectionId())) { + return; + } + pin.setConnectionId(connectionId); + pinRepository.save(pin); + return; + } + + ConnectionPin pin = new ConnectionPin(); + pin.setUsername(username); + pin.setConnectionId(connectionId); + try { + pinRepository.save(pin); + } catch (DataIntegrityViolationException e) { + pinRepository.findByUsernameIgnoreCase(username).ifPresent(concurrent -> { + concurrent.setConnectionId(connectionId); + pinRepository.save(concurrent); + }); + } + } + + /** + * Clear this user's default, but only when it is still the connection they asked to + * unpin. A stale click in another tab must not silently drop a pin the user has since + * moved somewhere else. + */ + @Transactional + public void unpin(String username, String connectionId) { + if (username == null || username.isBlank()) { + return; + } + pinRepository.findByUsernameIgnoreCase(username) + .filter(pin -> connectionId == null || connectionId.equals(pin.getConnectionId())) + .ifPresent(pinRepository::delete); + } + + /** Drop every user's pin on a connection that is being deleted. */ + public void clearPinsForConnection(String connectionId) { + if (connectionId == null || connectionId.isBlank()) { + return; + } + pinRepository.deleteByConnectionId(connectionId); + } +} diff --git a/backend/src/main/resources/db/migration/V120__create_connection_pin.sql b/backend/src/main/resources/db/migration/V120__create_connection_pin.sql new file mode 100644 index 0000000..ebfe5ea --- /dev/null +++ b/backend/src/main/resources/db/migration/V120__create_connection_pin.sql @@ -0,0 +1,24 @@ +-- Per-user default connection ("pin"). +-- +-- One row per user: the unique constraint is what makes a pinned connection *the* +-- default rather than one of several. Pinning a second connection moves this row +-- (ConnectionPinService.pin) instead of inserting another. +-- +-- Deliberately not a column on database_connection: a connection can be shared with +-- several users via connection_access_grant, and one user's default must not decide +-- what anyone else opens on. +-- +-- NOTE: this repository has no Flyway runtime — schema is applied by +-- spring.jpa.hibernate.ddl-auto=update from the ConnectionPin entity. Apply by hand +-- with psql only if you manage schema manually. +CREATE TABLE connection_pin ( + id BIGSERIAL PRIMARY KEY, + username VARCHAR(255) NOT NULL, + connection_id VARCHAR(36) NOT NULL, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP +); + +CREATE UNIQUE INDEX ux_connection_pin_username ON connection_pin (username); + +CREATE INDEX idx_connection_pin_connection ON connection_pin (connection_id); diff --git a/backend/src/test/java/com/dbaagent/controller/ConnectionControllerTest.java b/backend/src/test/java/com/dbaagent/controller/ConnectionControllerTest.java index 5b1d85e..9109a6a 100644 --- a/backend/src/test/java/com/dbaagent/controller/ConnectionControllerTest.java +++ b/backend/src/test/java/com/dbaagent/controller/ConnectionControllerTest.java @@ -6,6 +6,7 @@ import com.dbaagent.repository.ConnectionInitStatusRepository; import com.dbaagent.repository.ConnectionAccessGrantRepository; import com.dbaagent.repository.SchemaDocumentationRepository; +import com.dbaagent.service.ConnectionPinService; import com.dbaagent.service.ConnectionService; import com.dbaagent.service.CredentialService; import com.dbaagent.service.SchemaScannerService; @@ -26,6 +27,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -43,6 +46,7 @@ class ConnectionControllerTest { @Mock private AccessControlService accessControlService; @Mock private ConnectionAccessService connectionAccessService; @Mock private ConnectionAccessGrantRepository connectionAccessGrantRepository; + @Mock private ConnectionPinService connectionPinService; private ConnectionController controller; @@ -60,7 +64,8 @@ void setUp() { brainJobsService, accessControlService, connectionAccessService, - connectionAccessGrantRepository + connectionAccessGrantRepository, + connectionPinService ); } @@ -108,4 +113,70 @@ void savedConnectionTestHydratesPersistedSecretsAndSshConfigFromIdOnlyRequest() assertThat(effective.getSshPrivateKey()).isEqualTo("pem-secret"); assertThat(response.getBody()).containsEntry("connectionSuccessful", true); } + + @Test + void pinningAConnectionAuthorizesUseAccessAndRecordsItForTheCallingUser() { + when(accessControlService.requireCurrentUsername()).thenReturn("analyst"); + + ResponseEntity> response = controller.pinConnection("conn-1"); + + verify(accessControlService).assertCanUseConnection("conn-1"); + verify(connectionPinService).pin("analyst", "conn-1"); + assertThat(response.getBody()).containsEntry("pinned", true); + } + + /** + * A shared connection is config-read-only for its recipients, so requiring manage + * rights here would lock exactly those users out of setting a default. Pinning must + * check use access and nothing stronger. + */ + @Test + void pinningDoesNotRequireConfigManagementRights() { + when(accessControlService.requireCurrentUsername()).thenReturn("analyst"); + + controller.pinConnection("conn-1"); + + verify(accessControlService, never()).assertCanManageConnectionConfig("conn-1"); + } + + @Test + void unpinningIsScopedToTheConnectionTheCallerNamed() { + when(accessControlService.requireCurrentUsername()).thenReturn("analyst"); + + ResponseEntity> response = controller.unpinConnection("conn-1"); + + verify(accessControlService).assertCanUseConnection("conn-1"); + verify(connectionPinService).unpin("analyst", "conn-1"); + assertThat(response.getBody()).containsEntry("pinned", false); + } + + /** + * A denial must surface as the 403 it is. Every handler here ends in a catch-all that + * would otherwise report "Failed to pin connection" with a 500 — the caller could not + * then tell "not yours" from "broken". + */ + @Test + void aDeniedPinPropagatesTheStatusRatherThanBecomingA500() { + org.mockito.Mockito.doThrow( + new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.FORBIDDEN, "Access denied for this connection")) + .when(accessControlService).assertCanUseConnection("conn-2"); + + assertThatThrownBy(() -> controller.pinConnection("conn-2")) + .isInstanceOf(org.springframework.web.server.ResponseStatusException.class); + + verify(connectionPinService, never()).pin(org.mockito.ArgumentMatchers.anyString(), + org.mockito.ArgumentMatchers.anyString()); + } + + /** + * A deleted connection must not leave a pin behind that silently becomes someone + * else's default if the id is ever reused. + */ + @Test + void deletingAConnectionClearsEveryPinOnIt() { + controller.deleteConnection("conn-1"); + + verify(connectionPinService).clearPinsForConnection("conn-1"); + } } diff --git a/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java index 4ad60b2..624caa7 100644 --- a/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java +++ b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java @@ -61,6 +61,11 @@ class ConnectionScopedAuthorizationSafetyTest { * so the {@code connectionId} there is a label on the caller's own conversation * rather than a reference to someone else's data. There is no connection * authorization to perform. + *

  • {@code ConnectionController.getAllConnections} takes no arguments at all. It + * lists whatever {@code getConnectionsForUser(username, isAdmin)} returns, so + * the caller cannot name a connection to be shown one — the scan sees it only + * because the handler resolves the caller's pinned connection id. + * {@link #connectionListingTakesNoCallerSuppliedId()} re-derives that claim. * * * {@link #everyDelegatedCheckStillExists()} re-derives the first group, so removing the @@ -71,7 +76,8 @@ class ConnectionScopedAuthorizationSafetyTest { "DashboardWorkspaceController.java:60", "AgentChatController.java:25", "AgentConversationController.java:29", - "AgentConversationController.java:45" + "AgentConversationController.java:45", + "ConnectionController.java:428" ); /** Service methods that own a delegated connection check. */ @@ -382,6 +388,30 @@ void controllerAdvicesDoNotSwallowAuthorizationDenials() throws IOException { .isEmpty(); } + /** + * {@code GET /connections} is exempt because it accepts nothing from the caller — the + * list it returns is derived entirely from the authenticated username. That is a claim + * about the handler's signature, so check it: if it ever grows a parameter, the + * exemption would start covering an endpoint that can be pointed at someone + * else's connection. + */ + @Test + void connectionListingTakesNoCallerSuppliedId() throws IOException { + String source = Files.readString(CONTROLLER_DIR.resolve("ConnectionController.java")); + + assertThat(source) + .as("GET /connections has gained a parameter, so it may no longer be scoped " + + "purely by the authenticated user. Remove its AUTHORIZED_ELSEWHERE entry " + + "and authorize the caller-supplied value.") + .contains("public ResponseEntity> getAllConnections() {"); + + assertThat(source) + .as("GET /connections no longer filters by the calling user. Its exemption from " + + "the connection-scope scan assumed getConnectionsForUser(username, isAdmin) " + + "was doing the scoping.") + .contains("credentialService.getConnectionsForUser(username, isAdmin)"); + } + /** * A handler exempted because its check lives in the service layer stays exempt only * while that check is actually there. Without this, deleting the service-layer assert diff --git a/backend/src/test/java/com/dbaagent/service/ConnectionPinServiceTest.java b/backend/src/test/java/com/dbaagent/service/ConnectionPinServiceTest.java new file mode 100644 index 0000000..cf0b033 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/ConnectionPinServiceTest.java @@ -0,0 +1,131 @@ +package com.dbaagent.service; + +import com.dbaagent.model.ConnectionPin; +import com.dbaagent.repository.ConnectionPinRepository; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.dao.DataIntegrityViolationException; + +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class ConnectionPinServiceTest { + + @Mock private ConnectionPinRepository pinRepository; + @InjectMocks private ConnectionPinService service; + + /** + * The whole point of the feature: a pinned connection is the default. A + * second pin has to move the existing row, not add another one — otherwise + * "always the default" is decided by whichever row a query happens to return first. + */ + @Test + void pinningASecondConnectionMovesTheExistingPin() { + ConnectionPin existing = new ConnectionPin(); + existing.setId(7L); + existing.setUsername("analyst"); + existing.setConnectionId("conn-a"); + when(pinRepository.findByUsernameIgnoreCase("analyst")).thenReturn(Optional.of(existing)); + + service.pin("analyst", "conn-b"); + + ArgumentCaptor saved = ArgumentCaptor.forClass(ConnectionPin.class); + verify(pinRepository).save(saved.capture()); + assertThat(saved.getValue().getId()).isEqualTo(7L); + assertThat(saved.getValue().getConnectionId()).isEqualTo("conn-b"); + } + + @Test + void pinningTheAlreadyPinnedConnectionWritesNothing() { + ConnectionPin existing = new ConnectionPin(); + existing.setUsername("analyst"); + existing.setConnectionId("conn-a"); + when(pinRepository.findByUsernameIgnoreCase("analyst")).thenReturn(Optional.of(existing)); + + service.pin("analyst", "conn-a"); + + verify(pinRepository, never()).save(any()); + } + + @Test + void aFirstPinInsertsARowForThatUser() { + when(pinRepository.findByUsernameIgnoreCase("analyst")).thenReturn(Optional.empty()); + + service.pin("analyst", "conn-a"); + + ArgumentCaptor saved = ArgumentCaptor.forClass(ConnectionPin.class); + verify(pinRepository).save(saved.capture()); + assertThat(saved.getValue().getUsername()).isEqualTo("analyst"); + assertThat(saved.getValue().getConnectionId()).isEqualTo("conn-a"); + } + + /** + * Two tabs pinning at once collide on the unique constraint. That is a preference, + * not a conflict worth a 500 — the loser re-reads and updates. + */ + @Test + void aConcurrentInsertLosesTheRaceAndUpdatesInstead() { + ConnectionPin winner = new ConnectionPin(); + winner.setId(3L); + winner.setUsername("analyst"); + winner.setConnectionId("conn-a"); + when(pinRepository.findByUsernameIgnoreCase("analyst")) + .thenReturn(Optional.empty()) + .thenReturn(Optional.of(winner)); + when(pinRepository.save(any(ConnectionPin.class))) + .thenThrow(new DataIntegrityViolationException("duplicate key")) + .thenReturn(winner); + + service.pin("analyst", "conn-b"); + + assertThat(winner.getConnectionId()).isEqualTo("conn-b"); + } + + /** + * A stale click in a background tab must not clear a pin the user has since moved. + */ + @Test + void unpinningADifferentConnectionLeavesTheCurrentPinAlone() { + ConnectionPin existing = new ConnectionPin(); + existing.setUsername("analyst"); + existing.setConnectionId("conn-a"); + when(pinRepository.findByUsernameIgnoreCase("analyst")).thenReturn(Optional.of(existing)); + + service.unpin("analyst", "conn-b"); + + verify(pinRepository, never()).delete(any()); + } + + @Test + void unpinningThePinnedConnectionRemovesIt() { + ConnectionPin existing = new ConnectionPin(); + existing.setUsername("analyst"); + existing.setConnectionId("conn-a"); + when(pinRepository.findByUsernameIgnoreCase("analyst")).thenReturn(Optional.of(existing)); + + service.unpin("analyst", "conn-a"); + + verify(pinRepository).delete(existing); + } + + @Test + void anAnonymousCallerHasNoPinAndWritesNone() { + assertThat(service.pinnedConnectionId(null)).isEmpty(); + assertThat(service.pinnedConnectionId(" ")).isEmpty(); + + service.pin(null, "conn-a"); + service.pin("analyst", null); + + verify(pinRepository, never()).save(any()); + } +} diff --git a/src/components/ManageConnectionsModal.js b/src/components/ManageConnectionsModal.js index 54ec35d..e3acbc5 100644 --- a/src/components/ManageConnectionsModal.js +++ b/src/components/ManageConnectionsModal.js @@ -10,12 +10,16 @@ import { Plus, Shield, RefreshCw, + Pin, + PinOff, } from "lucide-react"; import styles from "./ManageConnectionsModal.module.css"; import ConnectionWizard from "./ConnectionWizard"; import ConnectionSlowQueryConfig from "./ConnectionSlowQueryConfig"; import SlowQuerySourceModal from "./SlowQuerySourceModal"; import { connectionAPI, brainAPI } from "@/lib/api/client"; +import { queryClient } from "@/lib/queryClient"; +import { queryKeys } from "@/lib/queryKeys"; import { useAuth } from "@/hooks/useAuth"; import { PERMISSIONS } from "@/lib/permissions"; import { getConnectionAccessBadge, getConnectionAccessLabel } from "@/lib/features"; @@ -34,6 +38,7 @@ export default function ManageConnectionsModal({ const [showAddModal, setShowAddModal] = useState(false); const [editingConnection, setEditingConnection] = useState(null); const [slowQuerySourceConn, setSlowQuerySourceConn] = useState(null); + const [pinningId, setPinningId] = useState(null); useEffect(() => { if (isOpen) { @@ -109,6 +114,38 @@ export default function ManageConnectionsModal({ } }; + /** + * Pin (or unpin) a connection as this user's default. + * + * The pin is per user, so this changes nothing for anyone else the connection is + * shared with — and it is gated on *use* access rather than config rights, which is + * why the button stays enabled on a connection this user cannot edit. + * + * The list here is fetched directly rather than through TanStack Query, so the cache + * has to be invalidated explicitly or the sidebar switcher keeps the old pin until + * something else refetches it. + */ + const handleTogglePin = async (conn) => { + try { + setPinningId(conn.id); + if (conn.pinned) { + await connectionAPI.unpinConnection(conn.id); + } else { + await connectionAPI.pinConnection(conn.id); + } + await fetchConnections(); + queryClient.invalidateQueries({ queryKey: queryKeys.connections.all }); + } catch (err) { + console.error("Failed to update default connection:", err); + alert( + "Failed to update default connection: " + + (err.response?.data?.message || err.message), + ); + } finally { + setPinningId(null); + } + }; + const handleNewConnectionSaved = async (newConnectionId) => { setShowAddModal(false); await fetchConnections(); @@ -165,6 +202,9 @@ export default function ManageConnectionsModal({ + @@ -180,9 +220,33 @@ export default function ManageConnectionsModal({ {connections.map((conn) => ( +
    + + Name Access Type
    + +
    {conn.connectionName} + {conn.pinned && ( + Your default + )} {getConnectionAccessLabel(conn) && ( {getConnectionAccessLabel(conn)} )} diff --git a/src/components/ManageConnectionsModal.module.css b/src/components/ManageConnectionsModal.module.css index b02d088..7c295f8 100644 --- a/src/components/ManageConnectionsModal.module.css +++ b/src/components/ManageConnectionsModal.module.css @@ -326,3 +326,56 @@ color: var(--color-grey); font-size: var(--font-size-sm); } + +/* Default-connection pin */ +.pinHeader { + width: 40px; + text-align: center; +} + +.pinButton { + display: flex; + align-items: center; + justify-content: center; + padding: 6px; + border: 1px solid transparent; + border-radius: var(--radius-sm); + background: transparent; + color: var(--color-light-5); + cursor: pointer; + transition: all 0.2s; +} + +.pinButton:hover:not(:disabled) { + background: var(--color-light-1); + border-color: var(--color-light-3); + color: var(--color-black); +} + +.pinButton:disabled { + opacity: 0.5; + cursor: not-allowed; +} + +.pinButtonActive { + color: var(--color-black); + border-color: var(--color-light-3); + background: var(--color-light-1); +} + +.pinButton .spinner { + animation: spin 1s linear infinite; +} + +.pinnedLabel { + align-self: flex-start; + padding: 2px 6px; + border: 1px solid var(--color-light-3); + border-radius: var(--radius-sm); + background: var(--color-light-1); + font-size: var(--font-size-xs); + font-weight: 600; + color: var(--color-light-7); + text-transform: uppercase; + letter-spacing: 0.03em; +} diff --git a/src/components/layout/AppSidebar.jsx b/src/components/layout/AppSidebar.jsx index efe9d05..809b935 100644 --- a/src/components/layout/AppSidebar.jsx +++ b/src/components/layout/AppSidebar.jsx @@ -1,7 +1,8 @@ import { useState, useEffect, useRef } from 'react' -import { Brain, Code2, Database, Settings, PanelLeftClose, PanelLeftOpen, LogOut, User, ChevronDown, Check, Newspaper, Gauge, MessageSquare, LayoutDashboard } from 'lucide-react' +import { Brain, Code2, Database, Settings, PanelLeftClose, PanelLeftOpen, LogOut, User, ChevronDown, Check, Pin, Newspaper, Gauge, MessageSquare, LayoutDashboard } from 'lucide-react' import { useActiveSection, useSetActiveSection } from '@/lib/stores/useNavStore' import { useConnectionManager } from '@/lib/hooks/useConnectionManager' +import { useSetConnectionPin } from '@/lib/hooks/queries' import { AGENTS_ENABLED, canAccessHomeSection, getConnectionAccessBadge, getConnectionAccessLabel } from '@/lib/features' import { PERMISSIONS } from '@/lib/permissions' import ManageConnectionsModal from '@/components/ManageConnectionsModal' @@ -40,6 +41,10 @@ export default function AppSidebar() { hasPermission(PERMISSIONS.MANAGE_USERS) || hasPermission(PERMISSIONS.MANAGE_PERMISSIONS) const { connections, connectionId, selectedConnection, changeConnection, isLoading, refetch } = useConnectionManager() + // Pinning lives here as well as in Manage Connections because that modal needs + // MANAGE_CONNECTIONS to open at all — a Developer or Data Engineer would otherwise + // have no way to choose the connection they land on, which is exactly who benefits. + const setConnectionPin = useSetConnectionPin() const visibleNavItems = NAV_ITEMS.filter(({ id }) => canAccessHomeSection(id, role, selectedConnection, permissions)) const initials = username.slice(0, 2).toUpperCase() @@ -153,22 +158,41 @@ export default function AppSidebar() {
    Connections
    {connections.map((conn) => ( - + + +
    ))}
    )} diff --git a/src/components/layout/AppSidebar.module.css b/src/components/layout/AppSidebar.module.css index e2a451a..7f459a3 100644 --- a/src/components/layout/AppSidebar.module.css +++ b/src/components/layout/AppSidebar.module.css @@ -260,12 +260,27 @@ padding: 8px 10px 4px; } -.dropdownItem { +/* Row wrapper: the connection itself is one button, the pin toggle another. They + cannot nest — a button inside a button is invalid HTML and the inner click never + behaves. */ +.dropdownRow { display: flex; align-items: center; - gap: 8px; width: calc(100% - 8px); margin: 0 4px 4px; + border-radius: 8px; +} + +.dropdownRow:hover { + background: #f3f4f6; +} + +.dropdownItem { + display: flex; + align-items: center; + gap: 8px; + flex: 1; + min-width: 0; padding: 8px 10px; border: none; border-radius: 8px; @@ -277,9 +292,6 @@ transition: background 0.1s; } -.dropdownItem:hover { - background: #f3f4f6; -} .dropdownItemActive { background: #f3f4f6; @@ -297,6 +309,35 @@ color: #111827; } +.dropdownPin { + display: flex; + align-items: center; + justify-content: center; + flex-shrink: 0; + padding: 7px; + margin-right: 4px; + border: none; + border-radius: 6px; + background: transparent; + color: #d1d5db; + cursor: pointer; + transition: color 0.15s, background 0.15s; +} + +.dropdownPin:hover:not(:disabled) { + background: #e5e7eb; + color: #111827; +} + +.dropdownPin:disabled { + opacity: 0.5; + cursor: not-allowed; +} + +.dropdownPinActive { + color: #111827; +} + /* ── User profile dropdown ── */ .userMenuWrap { position: relative; diff --git a/src/hooks/useAuth.jsx b/src/hooks/useAuth.jsx index 799a875..af7c783 100644 --- a/src/hooks/useAuth.jsx +++ b/src/hooks/useAuth.jsx @@ -3,6 +3,7 @@ import { useNavigate, useLocation } from 'react-router-dom' import { getActionPermission, getActionConfig } from '@/lib/actions' import { authAPI, setupAPI, adminAPI, AUTH_CHANGE_EVENT } from '@/lib/api/client' import { clearAgentRemoteUser } from '@/lib/api/agentClient' +import { resetConnectionPinApplied } from '@/lib/hooks/useConnectionManager' import { queryClient } from '@/lib/queryClient' import { useChatStore } from '@/lib/stores/useChatStore' import { useConnectionStore } from '@/lib/stores/useConnectionStore' @@ -22,6 +23,10 @@ const isPublicAuthPath = (pathname) => AUTH_PUBLIC_PATHS.some((prefix) => pathna const resetClientSessionState = () => { localStorage.removeItem('selectedConnectionId') + // The pinned connection is applied once per page load; sign-out has to re-arm it or + // the next user to sign in on this tab would land on whatever was selected last. + resetConnectionPinApplied() + useChatStore.getState().resetStore() useConnectionStore.persist.clearStorage() diff --git a/src/lib/api/client.js b/src/lib/api/client.js index 214d410..ac341d8 100644 --- a/src/lib/api/client.js +++ b/src/lib/api/client.js @@ -754,6 +754,21 @@ export const connectionAPI = { return response.data; }, + // Pin a connection as this user's default. Per user, not per connection — the + // backend keys the pin on the caller, so pinning a shared connection does not + // change what anyone else opens on. + pinConnection: async (connectionId) => { + const response = await apiClient.put(`/api/connections/${connectionId}/pin`); + return response.data; + }, + + unpinConnection: async (connectionId) => { + const response = await apiClient.delete( + `/api/connections/${connectionId}/pin`, + ); + return response.data; + }, + getInitStatus: async (connectionId) => { const response = await apiClient.get( `/api/connections/${connectionId}/init-status`, diff --git a/src/lib/hooks/queries/index.js b/src/lib/hooks/queries/index.js index b978958..d043d66 100644 --- a/src/lib/hooks/queries/index.js +++ b/src/lib/hooks/queries/index.js @@ -11,6 +11,7 @@ export { useSaveConnection, useUpdateConnection, useDeleteConnection, + useSetConnectionPin, } from "./useConnections"; export { diff --git a/src/lib/hooks/queries/useConnections.js b/src/lib/hooks/queries/useConnections.js index 1d052bd..53e0517 100644 --- a/src/lib/hooks/queries/useConnections.js +++ b/src/lib/hooks/queries/useConnections.js @@ -67,3 +67,24 @@ export function useDeleteConnection() { }, }) } + +/** + * Pin or unpin a connection as this user's default. + * + * The `pinned` flag rides the connection list response, so invalidating that one key + * updates every surface that shows connections — the sidebar switcher included — + * without a second request. + */ +export function useSetConnectionPin() { + const queryClient = useQueryClient() + + return useMutation({ + mutationFn: ({ connectionId, pinned }) => + pinned + ? connectionAPI.pinConnection(connectionId) + : connectionAPI.unpinConnection(connectionId), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: queryKeys.connections.all }) + }, + }) +} diff --git a/src/lib/hooks/useConnectionManager.js b/src/lib/hooks/useConnectionManager.js index 82ffc58..0f497a0 100644 --- a/src/lib/hooks/useConnectionManager.js +++ b/src/lib/hooks/useConnectionManager.js @@ -1,10 +1,25 @@ -import { useEffect, useCallback } from 'react' +import { useEffect, useCallback, useMemo } from 'react' import { useConnections } from './queries' import { useConnectionId, useDashboardActions } from '@/lib/stores' import { connectionAPI } from '@/lib/api/client' import { queryClient } from '@/lib/queryClient' import { queryKeys } from '@/lib/queryKeys' +/** + * Whether this page load has already honoured the user's pinned connection. + * + * Module scope, not a ref: this hook is called from a dozen sections, and a + * per-instance guard would let a section mounted later yank the user back to their + * pinned connection after they had deliberately switched away. Cleared on sign-out by + * `resetConnectionPinApplied` so the next user's pin is applied on their first load. + */ +let pinAppliedThisLoad = false + +/** Called from the auth reset so a pin is re-applied for whoever signs in next. */ +export function resetConnectionPinApplied() { + pinAppliedThisLoad = false +} + /** * Shared connection management hook — handles auto-selection on mount, * localStorage persistence, and schema cache warming. @@ -13,23 +28,53 @@ export function useConnectionManager() { const { setConnectionId } = useDashboardActions() const connectionId = useConnectionId() const { data: connectionsData, isLoading, refetch } = useConnections() - const connections = Array.isArray(connectionsData) ? connectionsData : [] - // Auto-select saved/first connection on mount + // The user's pinned connection sorts first so every list that renders these — the + // sidebar switcher included — shows the default at the top. `pinned` is per caller, + // resolved server-side against the effective user, so "View as" sees the target + // user's default rather than the admin's. Array.sort is stable, so everything else + // keeps the order the API returned. + const connections = useMemo(() => { + const list = Array.isArray(connectionsData) ? connectionsData : [] + return [...list].sort((a, b) => Number(b.pinned === true) - Number(a.pinned === true)) + }, [connectionsData]) + + const selectConnection = useCallback( + (id) => { + localStorage.setItem('selectedConnectionId', id) + setConnectionId(id) + connectionAPI.warmupConnection(id) + }, + [setConnectionId] + ) + + // Auto-select on load: pinned wins, then the last connection used, then the first + // available. + // + // The pin has to beat an *already selected* connection, not just an empty one: + // `useDashboardStore` persists `connectionId`, so after a reload something is always + // selected and a pin that only ran on a blank slate would never actually be the + // default — which is the whole feature. It applies once per page load + // (`pinAppliedThisLoad`), so switching connections mid-session still sticks. useEffect(() => { if (connections.length === 0) return - const savedId = localStorage.getItem('selectedConnectionId') - const savedExists = savedId && connections.some((c) => c.id === savedId) - if (savedExists && !connectionId) { - setConnectionId(savedId) - connectionAPI.warmupConnection(savedId) - } else if (!savedExists && !connectionId) { - const first = connections[0].id - localStorage.setItem('selectedConnectionId', first) - setConnectionId(first) - connectionAPI.warmupConnection(first) + + const pinned = connections.find((c) => c.pinned) + + if (!pinAppliedThisLoad) { + pinAppliedThisLoad = true + if (pinned && pinned.id !== connectionId) { + selectConnection(pinned.id) + return + } } - }, [connections, connectionId, setConnectionId]) + + if (connectionId) return + + const savedId = localStorage.getItem('selectedConnectionId') + const saved = savedId ? connections.find((c) => c.id === savedId) : null + selectConnection((pinned || saved || connections[0]).id) + }, [connections, connectionId, selectConnection]) const changeConnection = useCallback( (connId) => { From c48e350d01898cdf8161081a730e228fce83fdd2 Mon Sep 17 00:00:00 2001 From: Venkat SF Date: Wed, 9 Sep 2026 06:23:05 +0000 Subject: [PATCH 2/2] fix(editor): report unparseable SQL as a syntax error, not a permissions denial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user pasted a SELECT that still carried the double quotes it had in source code ("select h.id, ...) and was told "Only admins can execute DDL or DML from the SQL Editor". That reads as a permissions problem and sent people looking for a role fix. The statement is neither DDL nor DML — with an unclosed double quote the whole thing is one quoted identifier, so it is not valid SQL at all. Two keyword heuristics disagreed and the disagreement was resolved as "mutation". QueryNormalizer.detectQueryType sanitizes a prefix away and answers SELECT; the provider's isReadOnlyQuery strips only comments, still sees the leading quote, and answers false. mutating was computed as (!readOnly && type != UNKNOWN), so a statement classifyStatement had itself labelled SELECT became a mutation. classifyStatement now records that the parser rejected the statement and, when the detected verb is read-only and no hidden write was found, returns notParseable. enforce throws STATEMENT_NOT_PARSEABLE ahead of both the read-only branch and the mutation-confirmation branch, with a message naming the likely cause. The statement is still blocked, admins included — only the diagnosis changed. An admin is deliberately not offered a confirmation prompt for a statement nothing managed to classify, since confirming past the guard is the one way this could become a bypass. The reclassification is gated on isReadOnlyVerb(queryType) and hiddenWrite == null, which is what keeps a write the parser happens to reject from being excused as a typo: an unparseable DELETE, and a malformed data-modifying CTE, both keep their mutation handling, and both are covered by tests. QueryExecutionPolicyServiceTest: 49 tests pass, including every pre-existing guard case. Full backend suite diffed against pristine HEAD — failure sets identical, no regressions. The MCP guard already reported this case honestly ("Only read-only SQL is allowed ...") and was left alone. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 19 +++ .../QueryExecutionPolicyException.java | 28 +++++ .../service/QueryExecutionPolicyService.java | 60 ++++++++- .../QueryExecutionPolicyServiceTest.java | 118 ++++++++++++++++++ 4 files changed, 224 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2b4eb0b..c271b03 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -724,6 +724,25 @@ it against a real database — not a theoretical hardening pass. it killed **every** active query on the connection, including other users' work. The cancel endpoint is scoped to the connection *and* the user who started the run, so an execution id is not a kill primitive for someone else's query. +- **A statement DeepSQL cannot parse is a syntax error, not DDL/DML — say so.** A user + pasted a SELECT still carrying the double quotes it had in source code + (`"select h.id, ...`) and got **"Only admins can execute DDL or DML from the SQL + Editor"**, which reads as a permissions problem and sends people hunting for a role fix. + Two keyword heuristics disagreed and the code resolved the disagreement as "mutation": + `QueryNormalizer.detectQueryType` sanitizes a prefix away and answered `SELECT`, while + the provider's `isReadOnlyQuery` strips only *comments*, still saw the leading `"`, and + answered false — so `mutating = !readOnly && type != UNKNOWN` labelled a SELECT a + mutation. `classifyStatement` now records that the parser rejected the statement and, + when the detected verb is read-only and no hidden write was found, returns + `notParseable`; `enforce` throws `STATEMENT_NOT_PARSEABLE` ahead of both the read-only + and confirmation branches. **The statement is still blocked, for admins too** — only the + diagnosis changed, and an admin is deliberately *not* offered a confirmation prompt for + something nothing managed to classify. The reclassification is gated on + `isReadOnlyVerb(queryType)` and `hiddenWrite == null`, which is what keeps it from + becoming a bypass: an unparseable `DELETE`, and a malformed data-modifying CTE, both keep + their mutation handling (covered by `anUnparseableWriteIsStillTreatedAsAMutation` and + `aMalformedDataModifyingCteIsStillBlockedAsAWrite`). The MCP guard already reported this + case honestly ("Only read-only SQL is allowed …") and was left alone. - **Keep the client timeout under the proxy's.** `docker/nginx/default.conf` gives up at `proxy_read_timeout 300s`; the Editor used to ask for 600s, so a 6-minute query returned an opaque 504 while still running. `QUERY_TIMEOUT_SECONDS = 240` diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java index b21cea4..7af6835 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java @@ -12,6 +12,7 @@ public class QueryExecutionPolicyException extends RuntimeException { public static final String UNSAFE_MUTATION_BLOCKED = "UNSAFE_MUTATION_BLOCKED"; public static final String DB_WRITE_PRIVILEGE_DENIED = "DB_WRITE_PRIVILEGE_DENIED"; public static final String MULTI_STATEMENT_MISSING_SEMICOLONS = "MULTI_STATEMENT_MISSING_SEMICOLONS"; + public static final String STATEMENT_NOT_PARSEABLE = "STATEMENT_NOT_PARSEABLE"; private final String errorCode; private final HttpStatus httpStatus; @@ -57,6 +58,33 @@ public static QueryExecutionPolicyException editorMutationForbidden(String query ); } + /** + * The statement reads as a SELECT by keyword but is not valid SQL, so DeepSQL cannot + * verify it is read-only. + * + *

    It stays blocked — an unclassifiable statement is exactly what the guard exists + * to stop — but it is not DDL or DML, and saying so sent a user hunting for a + * permissions problem that did not exist. The trigger was a query pasted with the + * surrounding double quotes it had in source code: {@code QueryNormalizer.detectQueryType} + * sanitizes the prefix away and answers SELECT, while {@code isReadOnlyQuery} strips only + * comments, still sees a leading {@code "}, and answers "not read-only". Those two + * answers together mean "malformed", not "mutation". + */ + public static QueryExecutionPolicyException statementNotParseable(String queryType) { + return new QueryExecutionPolicyException( + STATEMENT_NOT_PARSEABLE, + HttpStatus.BAD_REQUEST, + "DeepSQL could not parse this statement, so it was blocked before running. " + + "It starts like a SELECT but is not valid SQL — check for a stray quote, " + + "bracket or backtick. SQL copied out of code or JSON often keeps the " + + "surrounding \" characters, which makes the whole statement one quoted " + + "identifier.", + false, + queryType, + List.of() + ); + } + public static QueryExecutionPolicyException confirmationRequired(String queryType, List warnings) { return new QueryExecutionPolicyException( EDITOR_MUTATION_CONFIRMATION_REQUIRED, diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index 4bd250c..a783969 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -121,6 +121,19 @@ public PolicyDecision enforce( throw QueryExecutionPolicyException.multiStatementMissingSemicolons(); } + // Same idea, one step earlier: a statement DeepSQL could not parse is blocked for + // everyone, but it is a syntax error, not DDL/DML. Reported ahead of both the + // read-only branch and the mutation-confirmation branch, so an admin gets the same + // accurate diagnosis instead of being offered a confirmation prompt for a + // statement nothing has actually classified. + StatementClassification unparseable = classifications.stream() + .filter(StatementClassification::notParseable) + .findFirst() + .orElse(null); + if (unparseable != null) { + throw QueryExecutionPolicyException.statementNotParseable(unparseable.queryType()); + } + if (effectiveContext.mutationMode() == QueryExecutionContext.MutationMode.READ_ONLY_ONLY) { if (!allReadOnlyOrPreamble || anyMutation) { if (origin == QueryExecutionOrigin.CHAT) { @@ -210,6 +223,7 @@ private StatementClassification classifyStatement(String statement, QueryExecuti // classified as a read. String hiddenWrite = detectHiddenWrite(trimmed); + boolean parseFailed = false; try { Statement parsed = CCJSqlParserUtil.parse(trimmed); if (parsed instanceof Select select) { @@ -272,6 +286,7 @@ private StatementClassification classifyStatement(String statement, QueryExecuti return new StatementClassification("TRUNCATE", false, true, false, true, false); } } catch (Exception parseError) { + parseFailed = true; log.debug("Falling back to keyword SQL classification: {}", parseError.getMessage()); } @@ -294,9 +309,41 @@ private StatementClassification classifyStatement(String statement, QueryExecuti boolean mutating = !readOnly && !"UNKNOWN".equalsIgnoreCase(queryType); boolean requiresWhere = "UPDATE".equalsIgnoreCase(queryType) || "DELETE".equalsIgnoreCase(queryType); boolean hasWhere = !requiresWhere || containsWhereClause(trimmed); + + // The two keyword heuristics can contradict each other, and their disagreement + // means "malformed", not "mutation". `QueryNormalizer.detectQueryType` sanitizes a + // prefix away before matching, so it answers SELECT for `"select ...`; the + // provider's `isReadOnlyQuery` strips only comments, still sees the leading quote, + // and answers false. That combination used to fall through as mutating=true, and a + // user pasting a SELECT with the double quotes it carried in source code was told + // "Only admins can execute DDL or DML" — a permissions error for a syntax problem. + // + // It stays blocked: the parser rejected it, so nothing here can vouch for it being + // read-only, and this is deliberately reported the same way to admins rather than + // routed into the mutation-confirmation flow. Only the diagnosis changes. + if (parseFailed && mutating && hiddenWrite == null && isReadOnlyVerb(queryType)) { + return new StatementClassification(queryType, false, false, false, false, false, true); + } + return new StatementClassification(queryType, readOnly, mutating, requiresWhere, hasWhere, false); } + /** + * True for the statement verbs that never write. Narrow on purpose: it gates the + * "malformed, not a mutation" reclassification above, and a write verb the parser + * happens to reject (a Postgres DDL form JSqlParser does not model, say) must keep its + * existing mutation handling rather than be re-labelled a syntax error. + */ + private boolean isReadOnlyVerb(String queryType) { + if (queryType == null) { + return false; + } + return switch (queryType.toUpperCase(Locale.ROOT)) { + case "SELECT", "SHOW", "DESCRIBE", "DESC", "EXPLAIN" -> true; + default -> false; + }; + } + /** * Detects a write hidden inside a statement that parses as a {@link Select}: * a data-modifying CTE ({@code WITH x AS (DELETE ...) SELECT ...}, which @@ -548,7 +595,18 @@ public record StatementClassification( boolean mutating, boolean requiresWhereClause, boolean hasWhereClause, - boolean sessionPreamble + boolean sessionPreamble, + boolean notParseable ) { + public StatementClassification( + String queryType, + boolean readOnly, + boolean mutating, + boolean requiresWhereClause, + boolean hasWhereClause, + boolean sessionPreamble + ) { + this(queryType, readOnly, mutating, requiresWhereClause, hasWhereClause, sessionPreamble, false); + } } } diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java index 309c4f2..6634989 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java @@ -615,4 +615,122 @@ void mcpAdminExplainDrop_isBlockedEvenWhenConfirmed() { .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); assertThat(exception.getMessage()).contains("DROP and TRUNCATE"); } + + // ── A malformed statement is a syntax error, not a permissions problem ────────── + // + // Reported from the field: a user pasted a SELECT that still carried the double + // quotes it had in source code and was told "Only admins can execute DDL or DML from + // the SQL Editor", which reads as a permissions problem and sent them looking for a + // role fix. The statement is neither DDL nor DML — it is not valid SQL at all. + // + // The cause is two keyword heuristics disagreeing: QueryNormalizer.detectQueryType + // sanitizes the prefix away and answers SELECT, while the provider's isReadOnlyQuery + // strips only comments, still sees the leading quote, and answers false. mutating was + // computed as (!readOnly && type != UNKNOWN), so "SELECT" became a mutation. + + private static final String QUOTED_SELECT = + "\"select h.id, h.name, h.city, case when h.country = 'India' then 'IN' " + + "when h.country = 'United States' then 'US' else 'XX' end country_code from hotel h"; + + @Test + void selectPastedWithItsSurroundingQuotes_isReportedAsASyntaxErrorNotAPermissionError() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest(QUOTED_SELECT, 10, 30), + QueryExecutionContext.editor("analyst", false, false), + "mysql" + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.STATEMENT_NOT_PARSEABLE); + assertThat(exception.getMessage()).doesNotContain("Only admins"); + assertThat(exception.getMessage()).contains("could not parse"); + } + + /** + * An admin gets the same diagnosis rather than a confirmation prompt. Offering to + * "confirm this DDL/DML" for a statement nothing managed to classify would invite + * confirming past the guard, and the statement cannot run anyway. + */ + @Test + void aMalformedSelectIsNotOfferedToAdminsAsAConfirmableMutation() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest(QUOTED_SELECT, 10, 30), + QueryExecutionContext.editor("admin", true, false), + "mysql" + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.STATEMENT_NOT_PARSEABLE); + assertThat(exception.isRequiresConfirmation()).isFalse(); + } + + /** Confirmation cannot get a malformed statement through either. */ + @Test + void aConfirmedAdminStillCannotRunAMalformedStatement() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest(QUOTED_SELECT, 10, 30), + QueryExecutionContext.editor("admin", true, true), + "mysql" + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.STATEMENT_NOT_PARSEABLE); + } + + /** + * The reclassification is gated on the detected verb being read-only, so a write the + * parser rejects keeps its mutation handling instead of being excused as a typo. This + * is the half that stops the fix from becoming a bypass. + */ + @Test + void anUnparseableWriteIsStillTreatedAsAMutation() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("DELETE FROM hotel WHERE (((", 10, 30), + QueryExecutionContext.editor("analyst", false, false), + "mysql" + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_FORBIDDEN); + } + + /** + * The data-modifying CTE this whole guard exists for must not slip through the new + * branch: `detectHiddenWrite` vetoes it before the parse result is consulted, so a + * malformed variant is still a blocked write rather than a reported typo. + */ + @Test + void aMalformedDataModifyingCteIsStillBlockedAsAWrite() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("WITH x AS (DELETE FROM hotel RETURNING *) SELECT * FROM x WHERE (((", 10, 30), + QueryExecutionContext.editor("analyst", false, false), + "mysql" + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_FORBIDDEN); + } + + /** The same query without the stray quote is an ordinary read. */ + @Test + void theSameSelectWithoutTheStrayQuoteIsAllowed() { + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest(QUOTED_SELECT.substring(1), 10, 30), + QueryExecutionContext.editor("analyst", false, false), + "mysql" + ); + + assertThat(decision.mutating()).isFalse(); + assertThat(decision.primaryQueryType()).isEqualTo("SELECT"); + } }