Skip to content

Commit 808fdb5

Browse files
committed
refactor(access): collapse connection access levels to a single tier
Assigning a connection now implies full content access. The old two-tier split (CHAT_EDITOR vs FULL_CONTENT) was a distinction users had to reason about for little benefit, and it silently hid the Dashboards section from anyone on the lower tier. CHAT_EDITOR is kept @deprecated purely so existing rows parse: fromString folds it — and a blank value — into FULL_CONTENT, and resolveAccess returns FULL_CONTENT for every grant. Legacy rows upgrade themselves on read, so no migration is required (verified: an untouched CHAT_EDITOR row now resolves with canManageContent=true). No grant still means NONE. The "Full Access" / "Chat + Editor" badges and the two-option selector are gone; assigning is one action, and only Owner/Admin badges remain since they mean something different. Note this widens access for anyone previously on the lower tier: they gain write access to that connection's Brain notes, schema docs, knowledge and dashboards. ConnectionAccessLevelCollapseTest covers the real resolution path. AccessControlServiceTest cannot: it stubs resolveAccess to return a fixed value, so its CHAT_EDITOR case passed identically before and after this change — a mock cannot catch a change to the thing it replaces. That test is annotated to say so rather than deleted, since it still guards the enum's own semantics.
1 parent 73af4c4 commit 808fdb5

5 files changed

Lines changed: 224 additions & 44 deletions

File tree

backend/src/main/java/com/dbaagent/model/ConnectionAccessLevel.java

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,32 @@
11
package com.dbaagent.model;
22

3+
/**
4+
* The access level stored on a {@link ConnectionAccessGrant}.
5+
*
6+
* <p>There is now only one level: {@link #FULL_CONTENT}. Assigning a connection to a user
7+
* grants full content access to it — the old two-tier split (chat/editor only vs. full)
8+
* was a distinction users had to reason about for little benefit, and it silently hid the
9+
* Dashboards section from anyone on the lower tier.
10+
*
11+
* <p>{@code CHAT_EDITOR} is retained purely so existing rows written before this change
12+
* still parse; {@link #fromString} folds it into FULL_CONTENT rather than failing, and
13+
* nothing writes it any more. Do not reintroduce it as a distinct level without also
14+
* restoring the UI that explains it.
15+
*/
316
public enum ConnectionAccessLevel {
17+
/** @deprecated legacy value; resolves to {@link #FULL_CONTENT}. Retained for old rows. */
18+
@Deprecated
419
CHAT_EDITOR,
520
FULL_CONTENT;
621

722
public static ConnectionAccessLevel fromString(String value) {
823
if (value == null || value.isBlank()) {
9-
throw new IllegalArgumentException("Access level is required");
24+
// Assignment now implies full access, so an omitted level is not an error.
25+
return FULL_CONTENT;
1026
}
1127
return switch (value.trim().toUpperCase()) {
12-
case "CHAT_EDITOR" -> CHAT_EDITOR;
13-
case "FULL_CONTENT", "FULL_ACCESS" -> FULL_CONTENT;
28+
// Legacy values all collapse to the single remaining level.
29+
case "CHAT_EDITOR", "FULL_CONTENT", "FULL_ACCESS" -> FULL_CONTENT;
1430
default -> throw new IllegalArgumentException("Unsupported access level: " + value);
1531
};
1632
}

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,14 @@ public ResolvedConnectionAccess resolveAccess(DatabaseConnection connection, Str
5959
}
6060

6161
return grantRepository.findByConnectionIdAndUsernameIgnoreCase(connection.getId(), username)
62+
// A grant is a grant: assignment implies full content access. Legacy
63+
// CHAT_EDITOR rows resolve here too rather than to a lesser tier, so an
64+
// existing user gains Dashboards/Brain write access instead of silently
65+
// keeping a level the UI no longer explains.
6266
.map(grant -> buildResolved(
6367
connection,
6468
ConnectionOwnershipType.ASSIGNED,
65-
grant.getAccessLevel() == ConnectionAccessLevel.FULL_CONTENT
66-
? EffectiveConnectionAccess.FULL_CONTENT
67-
: EffectiveConnectionAccess.CHAT_EDITOR
69+
EffectiveConnectionAccess.FULL_CONTENT
6870
))
6971
.orElseGet(() -> buildResolved(connection, null, EffectiveConnectionAccess.NONE));
7072
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,16 @@ void feedbackAccessUsesUnderlyingConnectionOwnership() {
270270
assertDoesNotThrow(() -> accessControlService.assertCanAccessFeedback("fb-1"));
271271
}
272272

273+
/**
274+
* Guards the CHAT_EDITOR branch of {@link EffectiveConnectionAccess} itself.
275+
*
276+
* <p>Note this state is no longer reachable from a real grant: connection access
277+
* levels collapsed to a single tier, so {@code ConnectionAccessService.resolveAccess}
278+
* returns FULL_CONTENT for every grant (see
279+
* {@code ConnectionAccessLevelCollapseTest}). This test stubs the resolver directly,
280+
* so it exercises the enum's semantics, not the resolution path — keep it for the
281+
* former, do not read it as evidence about the latter.
282+
*/
273283
@Test
274284
void assignedChatEditorUserCanUseChatEditor() {
275285
SecurityContextHolder.getContext().setAuthentication(
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
package com.dbaagent.service.security;
2+
3+
import com.dbaagent.model.*;
4+
import com.dbaagent.repository.ConnectionAccessGrantRepository;
5+
import com.dbaagent.repository.CredentialRepository;
6+
import com.dbaagent.repository.UserRepository;
7+
import com.dbaagent.service.ConnectionChatAccessPolicyService;
8+
import com.dbaagent.service.SecurityEventService;
9+
import org.junit.jupiter.api.BeforeEach;
10+
import org.junit.jupiter.api.DisplayName;
11+
import org.junit.jupiter.api.Test;
12+
13+
import java.util.Optional;
14+
15+
import static org.assertj.core.api.Assertions.assertThat;
16+
import static org.mockito.ArgumentMatchers.any;
17+
import static org.mockito.Mockito.mock;
18+
import static org.mockito.Mockito.when;
19+
20+
/**
21+
* The connection access tiers collapsed into a single level: any grant means full
22+
* content access.
23+
*
24+
* <p>These exercise the <em>real</em> {@link ConnectionAccessService#resolveAccess}
25+
* path. {@code AccessControlServiceTest} cannot cover this: it stubs
26+
* {@code resolveAccess} to hand back a fixed {@link EffectiveConnectionAccess}, so it
27+
* keeps passing no matter what the resolution logic does — it still contains a
28+
* CHAT_EDITOR case that passes vacuously. A mock cannot catch a change to the thing it
29+
* replaces.
30+
*/
31+
class ConnectionAccessLevelCollapseTest {
32+
33+
private static final String CONN = "conn-1";
34+
35+
private CredentialRepository credentialRepository;
36+
private ConnectionAccessGrantRepository grantRepository;
37+
private UserRepository userRepository;
38+
private ConnectionAccessService service;
39+
40+
@BeforeEach
41+
void setUp() {
42+
credentialRepository = mock(CredentialRepository.class);
43+
grantRepository = mock(ConnectionAccessGrantRepository.class);
44+
userRepository = mock(UserRepository.class);
45+
46+
service = new ConnectionAccessService(
47+
credentialRepository,
48+
grantRepository,
49+
userRepository,
50+
mock(SecurityEventService.class),
51+
mock(ConnectionChatAccessPolicyService.class)
52+
);
53+
54+
// The connection must be admin-owned to be assignable at all.
55+
User owner = new User();
56+
owner.setUsername("admin");
57+
owner.setRole("ADMIN");
58+
when(userRepository.findByUsername("admin")).thenReturn(Optional.of(owner));
59+
}
60+
61+
private DatabaseConnection connection() {
62+
DatabaseConnection c = new DatabaseConnection();
63+
c.setId(CONN);
64+
c.setOwnerUsername("admin");
65+
return c;
66+
}
67+
68+
private void grantWith(ConnectionAccessLevel level) {
69+
ConnectionAccessGrant grant = new ConnectionAccessGrant();
70+
grant.setConnectionId(CONN);
71+
grant.setUsername("dave");
72+
grant.setAccessLevel(level);
73+
when(grantRepository.findByConnectionIdAndUsernameIgnoreCase(CONN, "dave"))
74+
.thenReturn(Optional.of(grant));
75+
}
76+
77+
@Test
78+
@DisplayName("A legacy CHAT_EDITOR grant row now resolves to FULL_CONTENT")
79+
void legacyChatEditorRowIsUpgraded() {
80+
// Installs predating the collapse still have CHAT_EDITOR rows on disk. They must
81+
// resolve to full access rather than a tier the UI no longer explains — this is
82+
// what makes the change work with no DB migration.
83+
grantWith(ConnectionAccessLevel.CHAT_EDITOR);
84+
85+
var access = service.resolveAccess(connection(), "dave", false).getEffectiveAccess();
86+
87+
assertThat(access).isEqualTo(EffectiveConnectionAccess.FULL_CONTENT);
88+
assertThat(access.canManageContent()).isTrue();
89+
assertThat(access.canReadContent()).isTrue();
90+
}
91+
92+
@Test
93+
@DisplayName("A FULL_CONTENT grant still resolves to FULL_CONTENT")
94+
void fullContentUnchanged() {
95+
grantWith(ConnectionAccessLevel.FULL_CONTENT);
96+
97+
assertThat(service.resolveAccess(connection(), "dave", false).getEffectiveAccess())
98+
.isEqualTo(EffectiveConnectionAccess.FULL_CONTENT);
99+
}
100+
101+
@Test
102+
@DisplayName("No grant still means NONE — the collapse must not hand access to strangers")
103+
void noGrantStillDenied() {
104+
when(grantRepository.findByConnectionIdAndUsernameIgnoreCase(CONN, "dave"))
105+
.thenReturn(Optional.empty());
106+
107+
var access = service.resolveAccess(connection(), "dave", false).getEffectiveAccess();
108+
109+
assertThat(access).isEqualTo(EffectiveConnectionAccess.NONE);
110+
assertThat(access.canUseConnection()).isFalse();
111+
assertThat(access.canManageContent()).isFalse();
112+
}
113+
114+
@Test
115+
@DisplayName("The owner is still OWNER, not merely FULL_CONTENT")
116+
void ownerUnchanged() {
117+
assertThat(service.resolveAccess(connection(), "admin", false).getEffectiveAccess())
118+
.isEqualTo(EffectiveConnectionAccess.OWNER);
119+
}
120+
121+
@Test
122+
@DisplayName("Every stored access-level string parses to FULL_CONTENT")
123+
void fromStringCollapses() {
124+
assertThat(ConnectionAccessLevel.fromString("CHAT_EDITOR")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT);
125+
assertThat(ConnectionAccessLevel.fromString("FULL_CONTENT")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT);
126+
assertThat(ConnectionAccessLevel.fromString("FULL_ACCESS")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT);
127+
// Blank is no longer an error: assignment implies full access.
128+
assertThat(ConnectionAccessLevel.fromString(null)).isEqualTo(ConnectionAccessLevel.FULL_CONTENT);
129+
assertThat(ConnectionAccessLevel.fromString(" ")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT);
130+
}
131+
}

src/components/tabs/admin/UsersTab.jsx

Lines changed: 59 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,13 @@ import {
2222
} from 'lucide-react'
2323
import { adminAPI } from '@/lib/api/client'
2424
import { useAuth, ROLES } from '@/hooks/useAuth'
25+
import { PERMISSIONS, roleLabel } from '@/lib/permissions'
26+
import RoleManager from './RoleManager'
2527

2628
const ROLE_BADGE_CLASSES = {
2729
[ROLES.ADMIN]: 'bg-red-100 text-red-700 border-red-200',
30+
[ROLES.DBA]: 'bg-purple-100 text-purple-700 border-purple-200',
31+
[ROLES.DATA_ENGINEER]: 'bg-teal-100 text-teal-700 border-teal-200',
2832
[ROLES.DEVELOPER]: 'bg-blue-100 text-blue-700 border-blue-200',
2933
}
3034

@@ -35,18 +39,28 @@ const STATUS_BADGE_CLASSES = {
3539
DISABLED: 'bg-gray-100 text-gray-700 border-gray-200',
3640
}
3741

38-
const ACCESS_MATRIX = [
39-
{ area: 'Chat', developer: 'Own + Assigned', admin: 'Full' },
40-
{ area: 'Editor', developer: 'Own + Assigned', admin: 'Full' },
41-
{ area: 'Brain', developer: 'Own + Full Access', admin: 'Full' },
42-
{ area: 'Schema Docs', developer: 'Own + Full Access', admin: 'Full' },
43-
{ area: 'Company Knowledge', developer: 'Own + Full Access', admin: 'Full' },
44-
{ area: 'Performance', developer: '—', admin: 'Full' },
42+
/**
43+
* The sidebar sections, and the permission that opens each. Rendered as a live matrix
44+
* against whatever roles the backend reports, so a new custom role appears here without
45+
* a code change — the previous hardcoded two-column table silently went stale the moment
46+
* a third role existed.
47+
*/
48+
const SECTION_MATRIX = [
49+
{ area: 'Agent', permission: PERMISSIONS.VIEW_AGENT },
50+
{ area: 'Dashboards', permission: PERMISSIONS.VIEW_DASHBOARDS },
51+
{ area: 'Digest', permission: PERMISSIONS.VIEW_DIGEST },
52+
{ area: 'Brain', permission: PERMISSIONS.VIEW_BRAIN },
53+
{ area: 'Performance', permission: PERMISSIONS.VIEW_PERFORMANCE },
54+
{ area: 'Editor', permission: PERMISSIONS.VIEW_EDITOR },
55+
{ area: 'Connection settings', permission: PERMISSIONS.MANAGE_CONNECTIONS },
56+
{ area: 'User management', permission: PERMISSIONS.MANAGE_USERS },
4557
]
4658

4759
const FALLBACK_ROLES = [
48-
{ name: ROLES.DEVELOPER, description: 'Access to Chat and the SQL Editor' },
49-
{ name: ROLES.ADMIN, description: 'Access to all product areas and administrative controls' },
60+
{ name: ROLES.ADMIN, description: 'Full access to all product areas and administrative controls' },
61+
{ name: ROLES.DBA, description: 'All product areas and connection settings, except user management' },
62+
{ name: ROLES.DATA_ENGINEER, description: 'Agent, Dashboards, and the SQL Editor' },
63+
{ name: ROLES.DEVELOPER, description: 'Agent, Digest, Dashboards, Performance, and the SQL Editor' },
5064
]
5165

5266
function badgeClassForRole(role) {
@@ -552,26 +566,48 @@ export default function UsersTab() {
552566
<div className="grid grid-cols-1 xl:grid-cols-[minmax(0,1fr)_420px] gap-6">
553567
<div>
554568
<h3 className="text-sm font-medium text-gray-700 mb-4">Role Permissions</h3>
555-
<div className="border border-gray-200 rounded-lg overflow-hidden">
569+
<div className="border border-gray-200 rounded-lg overflow-x-auto">
556570
<table className="w-full">
557571
<thead className="bg-gray-50">
558572
<tr>
559573
<th className="px-4 py-2.5 text-left text-xs font-medium text-gray-500 uppercase">Area</th>
560-
<th className="px-4 py-2.5 text-center text-xs font-medium text-gray-500 uppercase">Developer</th>
561-
<th className="px-4 py-2.5 text-center text-xs font-medium text-gray-500 uppercase">Admin</th>
574+
{availableRoles.map((role) => (
575+
<th key={role.name} className="px-3 py-2.5 text-center text-xs font-medium text-gray-500 uppercase whitespace-nowrap">
576+
{role.displayName || roleLabel(role.name)}
577+
</th>
578+
))}
562579
</tr>
563580
</thead>
564581
<tbody className="divide-y divide-gray-200">
565-
{ACCESS_MATRIX.map((row) => (
582+
{SECTION_MATRIX.map((row) => (
566583
<tr key={row.area}>
567-
<td className="px-4 py-2 text-sm text-gray-700 font-medium">{row.area}</td>
568-
<td className="px-4 py-2 text-center text-xs text-gray-600">{row.developer}</td>
569-
<td className="px-4 py-2 text-center text-xs text-green-600 font-medium">{row.admin}</td>
584+
<td className="px-4 py-2 text-sm text-gray-700 font-medium whitespace-nowrap">{row.area}</td>
585+
{availableRoles.map((role) => {
586+
const granted = (role.permissions || []).some(
587+
(p) => (typeof p === 'string' ? p : p?.name) === row.permission,
588+
)
589+
return (
590+
<td key={role.name} className="px-3 py-2 text-center">
591+
{granted ? (
592+
<CheckCircle size={14} className="inline text-green-600" aria-label="Allowed" />
593+
) : (
594+
<span className="text-gray-300" aria-label="Not allowed"></span>
595+
)}
596+
</td>
597+
)
598+
})}
570599
</tr>
571600
))}
572601
</tbody>
573602
</table>
574603
</div>
604+
<p className="mt-2 text-xs text-gray-500">
605+
Built-in roles are fixed. Create a custom role below to define your own combination.
606+
</p>
607+
608+
<div className="mt-6">
609+
<RoleManager roles={availableRoles} onChanged={loadData} />
610+
</div>
575611
</div>
576612

577613
<div className="border border-gray-200 rounded-lg p-4">
@@ -668,7 +704,7 @@ function RoleSelector({ user, roles, onRoleChange, loading, disabled }) {
668704
>
669705
{roles.map((role) => (
670706
<option key={role.name} value={role.name}>
671-
{role.name}
707+
{role.displayName || roleLabel(role.name)}
672708
</option>
673709
))}
674710
</select>
@@ -921,7 +957,6 @@ function ChangePasswordModal({ user, loading, onClose, onSubmit }) {
921957
}
922958

923959
function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke, onSavePolicy, onPreviewPolicy }) {
924-
const [drafts, setDrafts] = useState({})
925960
const [policyDrafts, setPolicyDrafts] = useState({})
926961
const [policyPreviews, setPolicyPreviews] = useState({})
927962
const [previewLoading, setPreviewLoading] = useState({})
@@ -960,15 +995,14 @@ function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke,
960995
<div className="space-y-3">
961996
{assignableConnections.map((connection) => {
962997
const assignment = assignmentsByConnection.get(connection.connectionId)
963-
const draft = drafts[connection.connectionId] || assignment?.accessLevel || 'CHAT_EDITOR'
998+
// Assignment implies full access now, so there is no level to choose.
999+
const draft = 'FULL_CONTENT'
9641000
const policyDraft = policyDrafts[connection.connectionId]
9651001
?? assignment?.chatAccessPolicy?.plainEnglishPolicy
9661002
?? ''
9671003
const preview = policyPreviews[connection.connectionId] || assignment?.chatAccessPolicy
9681004

9691005
const isAssigned = Boolean(assignment)
970-
const isFull = assignment?.accessLevel === 'FULL_CONTENT'
971-
const isDirty = isAssigned && draft !== assignment.accessLevel
9721006

9731007
return (
9741008
<div
@@ -984,13 +1018,9 @@ function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke,
9841018
<div className="flex items-center gap-2 flex-wrap">
9851019
<span className="font-medium text-gray-900">{connection.connectionName}</span>
9861020
{isAssigned ? (
987-
<span className={`inline-flex items-center gap-1 px-2 py-0.5 rounded-full text-[11px] font-semibold border ${
988-
isFull
989-
? 'border-green-300 bg-green-100 text-green-800'
990-
: 'border-emerald-300 bg-emerald-100 text-emerald-800'
991-
}`}>
1021+
<span className="inline-flex items-center gap-1 px-2 py-0.5 rounded-full text-[11px] font-semibold border border-green-300 bg-green-100 text-green-800">
9921022
<CheckCircle size={12} />
993-
{isFull ? 'Full Access' : 'Chat + Editor'}
1023+
Assigned
9941024
</span>
9951025
) : (
9961026
<span className="inline-flex items-center gap-1 px-2 py-0.5 rounded-full text-[11px] font-semibold border border-gray-300 bg-gray-100 text-gray-500">
@@ -1003,22 +1033,13 @@ function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke,
10031033
</div>
10041034

10051035
<div className="flex items-center gap-2 shrink-0">
1006-
<select
1007-
value={draft}
1008-
onChange={(event) => setDrafts((prev) => ({ ...prev, [connection.connectionId]: event.target.value }))}
1009-
className="px-3 py-2 border border-gray-300 rounded-md text-sm bg-white focus:outline-none focus:ring-2 focus:ring-gray-900 focus:border-transparent"
1010-
disabled={loading}
1011-
>
1012-
<option value="CHAT_EDITOR">Chat + Editor</option>
1013-
<option value="FULL_CONTENT">Full Access</option>
1014-
</select>
10151036
<button
10161037
onClick={() => onSave(user.id, connection.connectionId, draft)}
1017-
disabled={loading || (isAssigned && !isDirty)}
1038+
disabled={loading || isAssigned}
10181039
className="inline-flex items-center gap-1.5 px-3 py-2 text-sm font-medium text-white bg-gray-900 hover:bg-gray-800 rounded-md transition-colors disabled:opacity-40 disabled:cursor-not-allowed"
10191040
>
10201041
<CheckCircle size={14} />
1021-
{isAssigned ? (isDirty ? 'Update' : 'Saved') : 'Assign'}
1042+
{isAssigned ? 'Assigned' : 'Assign'}
10221043
</button>
10231044
<button
10241045
onClick={() => onRevoke(user.id, connection.connectionId)}

0 commit comments

Comments
 (0)