Skip to content

Commit c4ebb5b

Browse files
venkateshsakamuri-labvenkateshsakamuri-lab
authored
Merge branch 'main' into fix/sql-guard-dangerous-functions
2 parents 38ef2ad + 89432da commit c4ebb5b

15 files changed

Lines changed: 221 additions & 43 deletions

‎backend/src/main/java/com/dbaagent/controller/ExplainController.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ public ResponseEntity<?> analyzeQuery(
9696
httpRequest.getHeader(HttpHeaders.AUTHORIZATION)
9797
),
9898
accessControlService.getCurrentUsername(),
99-
accessControlService.isCurrentUserAdmin(),
99+
accessControlService.currentUserMayMutateSql(),
100100
Boolean.TRUE.equals(request.getMutationConfirmed())
101101
),
102102
dbType

‎backend/src/main/java/com/dbaagent/controller/McpController.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public ResponseEntity<?> executeReadOnlyQuery(@RequestBody McpReadOnlyQueryReque
7878
queryRequest,
7979
QueryExecutionContext.mcp(
8080
accessControlService.getCurrentUsername(),
81-
accessControlService.isCurrentUserAdmin()
81+
accessControlService.currentUserMayMutateSql()
8282
)
8383
);
8484
return ResponseEntity.ok(Map.of(

‎backend/src/main/java/com/dbaagent/controller/SchemaController.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ private QueryExecutionContext queryExecutionContext(QueryRequest queryRequest, H
446446
return QueryExecutionContext.forSqlSurface(
447447
McpTokenService.isMcpAuthorizationHeader(httpRequest.getHeader(HttpHeaders.AUTHORIZATION)),
448448
accessControlService.getCurrentUsername(),
449-
accessControlService.isCurrentUserAdmin(),
449+
accessControlService.currentUserMayMutateSql(),
450450
Boolean.TRUE.equals(queryRequest.getMutationConfirmed())
451451
);
452452
}

‎backend/src/main/java/com/dbaagent/service/QueryExecutionContext.java‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,20 @@
44
import org.springframework.security.core.Authentication;
55
import org.springframework.security.core.context.SecurityContextHolder;
66

7+
/**
8+
* Execution origin + mutation privileges for a SQL run.
9+
*
10+
* <p>{@code actorMayMutate} is true for built-in ADMIN and DBA (wired from
11+
* {@code AccessControlService.currentUserMayMutateSql()}). It gates Editor/MCP
12+
* DDL/DML; chat stays {@link MutationMode#READ_ONLY_ONLY}. The same flag is
13+
* consulted by data-access policy during query execution so mutators are not
14+
* blocked by schema redaction meant for read-only roles.
15+
*/
716
public record QueryExecutionContext(
817
QueryExecutionOrigin origin,
918
MutationMode mutationMode,
1019
String actorUsername,
11-
boolean actorIsAdmin,
20+
boolean actorMayMutate,
1221
boolean mutationConfirmed
1322
) {
1423

@@ -27,12 +36,12 @@ public static QueryExecutionContext chat() {
2736
);
2837
}
2938

30-
public static QueryExecutionContext editor(String actorUsername, boolean actorIsAdmin, boolean mutationConfirmed) {
39+
public static QueryExecutionContext editor(String actorUsername, boolean actorMayMutate, boolean mutationConfirmed) {
3140
return new QueryExecutionContext(
3241
QueryExecutionOrigin.EDITOR,
33-
actorIsAdmin ? MutationMode.MAY_MUTATE : MutationMode.READ_ONLY_ONLY,
42+
actorMayMutate ? MutationMode.MAY_MUTATE : MutationMode.READ_ONLY_ONLY,
3443
actorUsername,
35-
actorIsAdmin,
44+
actorMayMutate,
3645
mutationConfirmed
3746
);
3847
}
@@ -51,39 +60,39 @@ public static QueryExecutionContext mcp(String actorUsername) {
5160
return mcp(actorUsername, false);
5261
}
5362

54-
public static QueryExecutionContext mcp(String actorUsername, boolean actorIsAdmin) {
55-
return mcp(actorUsername, actorIsAdmin, false);
63+
public static QueryExecutionContext mcp(String actorUsername, boolean actorMayMutate) {
64+
return mcp(actorUsername, actorMayMutate, false);
5665
}
5766

5867
/**
59-
* MCP / coding-agent SQL. Developers stay read-only. Admins may run
68+
* MCP / coding-agent SQL. Developers stay read-only. Admins and DBAs may run
6069
* non-destructive DDL/DML after the same confirmation gate as the Editor.
6170
* DROP and TRUNCATE stay blocked in {@link QueryExecutionPolicyService}.
6271
*/
6372
public static QueryExecutionContext mcp(
6473
String actorUsername,
65-
boolean actorIsAdmin,
74+
boolean actorMayMutate,
6675
boolean mutationConfirmed
6776
) {
6877
return new QueryExecutionContext(
6978
QueryExecutionOrigin.MCP,
70-
actorIsAdmin ? MutationMode.MAY_MUTATE : MutationMode.READ_ONLY_ONLY,
79+
actorMayMutate ? MutationMode.MAY_MUTATE : MutationMode.READ_ONLY_ONLY,
7180
actorUsername,
72-
actorIsAdmin,
81+
actorMayMutate,
7382
mutationConfirmed
7483
);
7584
}
7685

7786
public static QueryExecutionContext forSqlSurface(
7887
boolean mcpBearer,
7988
String actorUsername,
80-
boolean actorIsAdmin,
89+
boolean actorMayMutate,
8190
boolean mutationConfirmed
8291
) {
8392
if (mcpBearer) {
84-
return mcp(actorUsername, actorIsAdmin, mutationConfirmed);
93+
return mcp(actorUsername, actorMayMutate, mutationConfirmed);
8594
}
86-
return editor(actorUsername, actorIsAdmin, mutationConfirmed);
95+
return editor(actorUsername, actorMayMutate, mutationConfirmed);
8796
}
8897

8998
public static QueryExecutionContext scheduled() {
@@ -100,12 +109,16 @@ public static QueryExecutionContext api(String actorUsername) {
100109
return api(actorUsername, false);
101110
}
102111

103-
public static QueryExecutionContext api(String actorUsername, boolean actorIsAdmin) {
112+
/**
113+
* API / dashboard SQL is always read-only. The {@code actorMayMutate} flag here
114+
* only influences data-access policy bypass (admins), not mutation mode.
115+
*/
116+
public static QueryExecutionContext api(String actorUsername, boolean actorMayMutate) {
104117
return new QueryExecutionContext(
105118
QueryExecutionOrigin.API,
106119
MutationMode.READ_ONLY_ONLY,
107120
actorUsername,
108-
actorIsAdmin,
121+
actorMayMutate,
109122
false
110123
);
111124
}

‎backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public static QueryExecutionPolicyException editorMutationForbidden(String query
5151
return new QueryExecutionPolicyException(
5252
EDITOR_MUTATION_FORBIDDEN,
5353
HttpStatus.FORBIDDEN,
54-
"Only admins can execute DDL or DML from the SQL Editor. This Editor run was blocked before any database changes were attempted.",
54+
"Only admins or DBAs can execute DDL or DML from the SQL Editor. This Editor run was blocked before any database changes were attempted.",
5555
false,
5656
queryType,
5757
List.of()

‎backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -156,15 +156,15 @@ public PolicyDecision enforce(
156156
}
157157

158158
StatementClassification mutation = classifications.getFirst();
159-
if (!effectiveContext.actorIsAdmin()) {
159+
if (!effectiveContext.actorMayMutate()) {
160160
throw QueryExecutionPolicyException.editorMutationForbidden(mutation.queryType());
161161
}
162162

163163
if (origin == QueryExecutionOrigin.MCP
164164
&& isDropOrTruncateStatement(mutation.queryType(), statements.getFirst())) {
165165
throw QueryExecutionPolicyException.unsafeMutation(
166166
"DROP and TRUNCATE are blocked on MCP and coding-agent loops. "
167-
+ "CREATE, ALTER, and DML still require admin privileges plus confirmation.",
167+
+ "CREATE, ALTER, and DML still require admin or DBA privileges plus confirmation.",
168168
mutation.queryType()
169169
);
170170
}
@@ -316,7 +316,7 @@ private StatementClassification classifyStatement(String statement, QueryExecuti
316316
// provider's `isReadOnlyQuery` strips only comments, still sees the leading quote,
317317
// and answers false. That combination used to fall through as mutating=true, and a
318318
// user pasting a SELECT with the double quotes it carried in source code was told
319-
// "Only admins can execute DDL or DML" — a permissions error for a syntax problem.
319+
// "Only admins or DBAs can execute DDL or DML" — a permissions error for a syntax problem.
320320
//
321321
// It stays blocked: the parser rejected it, so nothing here can vouch for it being
322322
// read-only, and this is deliberately reported the same way to admins rather than

‎backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ public QueryGuardDecision enforcePreExecution(
129129
ConnectionChatAccessPolicyService.EffectivePolicy policy = policyService.resolveEffectivePolicy(
130130
connectionId,
131131
executionContext.actorUsername(),
132-
executionContext.actorIsAdmin()
132+
executionContext.actorMayMutate()
133133
);
134134
if (!policy.protectsAnything()) {
135135
return QueryGuardDecision.allow(policy);
@@ -339,7 +339,7 @@ public QueryResult redactResult(
339339
ConnectionChatAccessPolicyService.EffectivePolicy policy = policyService.resolveEffectivePolicy(
340340
connectionId,
341341
executionContext.actorUsername(),
342-
executionContext.actorIsAdmin()
342+
executionContext.actorMayMutate()
343343
);
344344
if (!policy.protectsAnything() || !policy.redactMode()) {
345345
return result;

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import com.dbaagent.model.ChatFeedback;
66
import com.dbaagent.model.EffectiveConnectionAccess;
77
import com.dbaagent.model.Permission;
8+
import com.dbaagent.model.Role;
89
import com.dbaagent.repository.AnalysisHistoryRepository;
910
import com.dbaagent.repository.ChatFeedbackRepository;
1011
import com.dbaagent.repository.ChatRepository;
@@ -297,6 +298,38 @@ public boolean isCurrentUserAdmin() {
297298
.anyMatch(authority -> "ROLE_ADMIN".equals(authority.getAuthority()));
298299
}
299300

301+
/**
302+
* Whether the current principal may run confirmed DDL/DML on SQL surfaces
303+
* (Editor / MCP). Built-in ADMIN and DBA only — not custom roles, and not
304+
* DEVELOPER / DATA_ENGINEER. Distinct from {@link #isCurrentUserAdmin()}:
305+
* DBA must not receive MANAGE_USERS or other admin-only product controls.
306+
*/
307+
public boolean currentUserMayMutateSql() {
308+
if (ImpersonationContext.isActive()) {
309+
return ImpersonationContext.current()
310+
.map(state -> {
311+
if (state.target() == null) {
312+
return false;
313+
}
314+
Role role = state.target().getRoleEnum();
315+
return role == Role.ADMIN || role == Role.DBA;
316+
})
317+
.orElse(false);
318+
}
319+
if (!authEnabled) {
320+
return true;
321+
}
322+
Authentication authentication = currentAuthentication();
323+
if (authentication == null || !authentication.isAuthenticated()) {
324+
return false;
325+
}
326+
return authentication.getAuthorities().stream()
327+
.anyMatch(authority -> {
328+
String value = authority.getAuthority();
329+
return "ROLE_ADMIN".equals(value) || "ROLE_DBA".equals(value);
330+
});
331+
}
332+
300333
private Authentication currentAuthentication() {
301334
return SecurityContextHolder.getContext().getAuthentication();
302335
}

‎backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ void useAnalyzeTrue_mcpBearer_usesMcpExecutionContext() {
140140
when(httpRequest.getHeader(HttpHeaders.AUTHORIZATION))
141141
.thenReturn("Bearer dsql_mcp_public.secret");
142142
when(accessControlService.getCurrentUsername()).thenReturn("admin");
143-
when(accessControlService.isCurrentUserAdmin()).thenReturn(true);
143+
when(accessControlService.currentUserMayMutateSql()).thenReturn(true);
144144
when(explainPlanService.analyzeQuery(eq("conn-1"), anyString(), eq(true)))
145145
.thenReturn(new ExplainPlanAnalysis());
146146

@@ -154,7 +154,7 @@ void useAnalyzeTrue_mcpBearer_usesMcpExecutionContext() {
154154
assertThat(captor.getValue().origin()).isEqualTo(QueryExecutionOrigin.MCP);
155155
assertThat(captor.getValue().mutationMode())
156156
.isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
157-
assertThat(captor.getValue().actorIsAdmin()).isTrue();
157+
assertThat(captor.getValue().actorMayMutate()).isTrue();
158158
}
159159

160160
@Test

‎backend/src/test/java/com/dbaagent/service/QueryExecutionContextTest.java‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,20 +13,29 @@ void mcpFactoryProducesReadOnlyContextWithMcpOrigin() {
1313
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
1414
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.READ_ONLY_ONLY);
1515
assertThat(ctx.actorUsername()).isEqualTo("user-1");
16-
assertThat(ctx.actorIsAdmin()).isFalse();
16+
assertThat(ctx.actorMayMutate()).isFalse();
1717
assertThat(ctx.mutationConfirmed()).isFalse();
1818
}
1919

2020
@Test
21-
void mcpFactoryHonoursAdminFlagFromSecurityContext() {
21+
void mcpFactoryHonoursMayMutateFlagFromSecurityContext() {
2222
QueryExecutionContext ctx = QueryExecutionContext.mcp("admin", true);
2323
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
2424
assertThat(ctx.actorUsername()).isEqualTo("admin");
25-
assertThat(ctx.actorIsAdmin()).isTrue();
25+
assertThat(ctx.actorMayMutate()).isTrue();
2626
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
2727
assertThat(ctx.mutationConfirmed()).isFalse();
2828
}
2929

30+
@Test
31+
void mcpDbaMayMutateWithConfirmation() {
32+
QueryExecutionContext ctx = QueryExecutionContext.mcp("dba", true, true);
33+
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
34+
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
35+
assertThat(ctx.actorMayMutate()).isTrue();
36+
assertThat(ctx.mutationConfirmed()).isTrue();
37+
}
38+
3039
@Test
3140
void mcpAdminConfirmedFactoryPassesConfirmationFlag() {
3241
QueryExecutionContext ctx = QueryExecutionContext.mcp("admin", true, true);
@@ -36,11 +45,19 @@ void mcpAdminConfirmedFactoryPassesConfirmationFlag() {
3645
}
3746

3847
@Test
39-
void mcpNonAdminRemainsReadOnlyEvenWhenConfirmed() {
48+
void mcpNonMutatorRemainsReadOnlyEvenWhenConfirmed() {
4049
QueryExecutionContext ctx = QueryExecutionContext.mcp("dev", false, true);
4150
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
4251
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.READ_ONLY_ONLY);
43-
assertThat(ctx.actorIsAdmin()).isFalse();
52+
assertThat(ctx.actorMayMutate()).isFalse();
53+
}
54+
55+
@Test
56+
void editorDbaGetsMayMutateMode() {
57+
QueryExecutionContext ctx = QueryExecutionContext.editor("dba", true, false);
58+
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.EDITOR);
59+
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
60+
assertThat(ctx.actorMayMutate()).isTrue();
4461
}
4562

4663
@Test
@@ -60,7 +77,7 @@ void scheduledFactoryProducesMayMutateInternalActor() {
6077
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.SCHEDULED);
6178
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
6279
assertThat(ctx.actorUsername()).isNull();
63-
assertThat(ctx.actorIsAdmin()).isTrue();
80+
assertThat(ctx.actorMayMutate()).isTrue();
6481
assertThat(ctx.mutationConfirmed()).isTrue();
6582
}
6683

0 commit comments

Comments
 (0)