diff --git a/CLAUDE.md b/CLAUDE.md index 2362039..2760026 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -499,6 +499,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. @@ -680,6 +725,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/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/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/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/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"); + } } 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 ebcd533..19c008a 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) => {