From d3986803ce8c091624c18f95bfb5e2fcdea3ac1b Mon Sep 17 00:00:00 2001 From: sumit Date: Sun, 23 Aug 2026 15:54:14 +0530 Subject: [PATCH 1/5] feat(roles): permission-driven roles with admin-defined custom roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the two-role hierarchy (DEVELOPER < ADMIN, compared by ordinal) with permission sets. The shipped roles deliberately overlap without nesting — Data Engineer has Dashboards but not Digest, Developer has Digest but no connection settings — so "is role A at least role B" has no answer, and Role.isAtLeast is gone. ADMIN everything; a fixed point that ignores overrides, so no configuration change can lock out the last administrator DBA all menus + connection settings, but NOT user creation DATA_ENGINEER Agent, Dashboards, Editor DEVELOPER Agent, Digest, Dashboards, Performance, Editor custom an admin-defined permission set (custom_roles) One VIEW_* permission per sidebar section drives the nav, so the UI gates on capabilities rather than a rank. A "role code" is either a built-in Role name or a CustomRole.code — they share the users.role namespace, so creation refuses a colliding code. Role.fromString returns null for anything unrecognised instead of collapsing to DEVELOPER: mapping a custom role onto a built-in one would hand its holders the wrong permissions, and an unknown code must grant nothing rather than silent access. Every token-minting path resolves by role code via PermissionService (JwtUtil gained a String overload); a Role-typed path cannot represent a custom role. AuthController gates its success branch on roleCode, not the nullable role — the latter sent every custom-role login down the challenge branch and NPE'd in Map.of, a 500 on every such sign-in. RolePermissionConstraintInitializer drops the stale CHECK constraints Hibernate generated under the old enums; ddl-auto=update never drops a constraint, so on any existing database an override for DBA or a new section permission was rejected outright. Verified live against a running install, not inferred. --- .../RolePermissionConstraintInitializer.java | 95 ++++++ .../dbaagent/controller/AuthController.java | 32 +- .../controller/AuthInternalController.java | 6 +- .../controller/ImpersonationController.java | 7 +- .../controller/PermissionController.java | 316 +++++++++++------- .../java/com/dbaagent/model/CustomRole.java | 126 +++++++ .../java/com/dbaagent/model/Permission.java | 114 ++++--- .../main/java/com/dbaagent/model/Role.java | 114 +++---- .../model/RolePermissionOverride.java | 19 +- .../main/java/com/dbaagent/model/User.java | 39 ++- .../repository/CustomRoleRepository.java | 18 + .../RolePermissionOverrideRepository.java | 13 +- .../dbaagent/repository/UserRepository.java | 3 + .../security/CustomUserDetailsService.java | 20 +- .../java/com/dbaagent/security/JwtUtil.java | 24 +- .../dbaagent/service/AuthSessionService.java | 27 +- .../dbaagent/service/CustomRoleService.java | 162 +++++++++ .../service/PasswordlessAuthService.java | 31 +- .../dbaagent/service/PermissionService.java | 279 ++++++++++------ .../SlackConversationStateService.java | 4 +- .../service/UserManagementService.java | 83 +++-- .../service/AuthFlowResultCustomRoleTest.java | 69 ++++ .../service/PermissionResolutionTest.java | 165 +++++++++ src/components/tabs/admin/RoleManager.jsx | 305 +++++++++++++++++ src/hooks/useAuth.jsx | 36 +- src/lib/permissions.js | 125 +++++-- 26 files changed, 1774 insertions(+), 458 deletions(-) create mode 100644 backend/src/main/java/com/dbaagent/config/RolePermissionConstraintInitializer.java create mode 100644 backend/src/main/java/com/dbaagent/model/CustomRole.java create mode 100644 backend/src/main/java/com/dbaagent/repository/CustomRoleRepository.java create mode 100644 backend/src/main/java/com/dbaagent/service/CustomRoleService.java create mode 100644 backend/src/test/java/com/dbaagent/service/AuthFlowResultCustomRoleTest.java create mode 100644 backend/src/test/java/com/dbaagent/service/PermissionResolutionTest.java create mode 100644 src/components/tabs/admin/RoleManager.jsx diff --git a/backend/src/main/java/com/dbaagent/config/RolePermissionConstraintInitializer.java b/backend/src/main/java/com/dbaagent/config/RolePermissionConstraintInitializer.java new file mode 100644 index 0000000..ce93c62 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/config/RolePermissionConstraintInitializer.java @@ -0,0 +1,95 @@ +package com.dbaagent.config; + +import lombok.extern.slf4j.Slf4j; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.DependsOn; +import org.springframework.jdbc.core.JdbcTemplate; + +import javax.sql.DataSource; + +/** + * Drops the stale CHECK constraints Hibernate generated for + * {@code role_permission_overrides} under the original two-role, twenty-permission + * enums. + * + *

{@code ddl-auto=update} adds columns and tables but never drops a constraint it + * previously created, so on any database created before the role model changed both + * checks survive and reject every new value. Inserting an override for the DBA role, or + * for any of the new section permissions, fails with: + * + * 

+ *   ERROR: new row for relation "role_permission_overrides" violates check constraint
+ *          "role_permission_overrides_permission_code_check"
+ *
+ * + *

That was observed against a live install, not inferred — the tables looked correct + * and only an actual INSERT revealed it. There is no Flyway runtime in this repo + * (see CLAUDE.md), so this initializer is what actually applies the matching statements + * in {@code V117__create_dashboard_workspaces_and_custom_roles.sql}. + * + *

Dropping rather than rewriting the constraints is deliberate: the {@code Role} / + * {@code Permission} enums plus {@code PermissionService} are the authority for these + * values, and a database-level copy of an enum has to be re-migrated on every future + * addition — which is precisely how this broke. + */ +@Configuration +@Slf4j +public class RolePermissionConstraintInitializer { + + private static final String TABLE = "role_permission_overrides"; + + @Bean("rolePermissionConstraintBootstrap") + @DependsOn("entityManagerFactory") + public Object rolePermissionConstraintBootstrap(DataSource dataSource) { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + + if (!tableExists(jdbc, TABLE)) { + return new Object(); + } + + int dropped = 0; + dropped += dropCheckIfPresent(jdbc, TABLE + "_role_check"); + dropped += dropCheckIfPresent(jdbc, TABLE + "_permission_code_check"); + + // Widen the role column so a custom role code fits; harmless if already wide. + try { + jdbc.execute("ALTER TABLE " + TABLE + " ALTER COLUMN role TYPE VARCHAR(64)"); + } catch (RuntimeException e) { + log.warn("Could not widen {}.role: {}", TABLE, e.getMessage()); + } + + if (dropped > 0) { + log.info("Dropped {} stale CHECK constraint(s) on {} left over from the previous role model", + dropped, TABLE); + } + return new Object(); + } + + private int dropCheckIfPresent(JdbcTemplate jdbc, String constraintName) { + Integer count = jdbc.queryForObject(""" + SELECT COUNT(*) + FROM information_schema.table_constraints + WHERE table_schema = 'public' AND table_name = ? AND constraint_name = ? + """, Integer.class, TABLE, constraintName); + if (count == null || count == 0) { + return 0; + } + try { + jdbc.execute("ALTER TABLE " + TABLE + " DROP CONSTRAINT IF EXISTS " + constraintName); + return 1; + } catch (RuntimeException e) { + log.warn("Could not drop stale constraint {}: {}", constraintName, e.getMessage()); + return 0; + } + } + + private boolean tableExists(JdbcTemplate jdbc, String tableName) { + Integer count = jdbc.queryForObject(""" + SELECT COUNT(*) + FROM information_schema.tables + WHERE table_schema = 'public' AND table_name = ? + """, Integer.class, tableName); + return count != null && count > 0; + } +} diff --git a/backend/src/main/java/com/dbaagent/controller/AuthController.java b/backend/src/main/java/com/dbaagent/controller/AuthController.java index 105b272..55104b1 100644 --- a/backend/src/main/java/com/dbaagent/controller/AuthController.java +++ b/backend/src/main/java/com/dbaagent/controller/AuthController.java @@ -167,7 +167,8 @@ public ResponseEntity completeGoogleLogin( return redirectToFrontend("/login?error=" + urlEncode(result.message())); } - if (result.sessionAuthentication() != null && result.user() != null && result.role() != null) { + // Gate on roleCode, not role: role is null for a custom-role user. + if (result.sessionAuthentication() != null && result.user() != null && result.roleCode() != null) { authSessionService.writeSessionCookies(httpResponse, result.sessionAuthentication()); return redirectToFrontend("/dashboard"); } @@ -224,8 +225,8 @@ public ResponseEntity refreshSession(HttpServletRequest httpRequest, HttpServ } Map payload = toAuthPayload( effectiveUser, - effectiveUser.getRoleEnum(), - permissionService.getEffectivePermissionCodes(effectiveUser.getRoleEnum()) + effectiveUser.getRoleCode(), + permissionService.getEffectivePermissionCodes(effectiveUser.getRoleCode()) ); impersonationService.decorateAuthPayload(httpRequest, user, payload); return ResponseEntity.ok(payload); @@ -332,9 +333,9 @@ public ResponseEntity getCurrentUser(HttpServletRequest httpRequest) { return ResponseEntity.status(401).body(Map.of("message", "Not authenticated")); } User user = currentUserEntity(); - Role role = user.getRoleEnum(); - Set permissions = permissionService.getEffectivePermissionCodes(role); - Map response = toAuthPayload(user, role, permissions); + String roleCode = user.getRoleCode(); + Set permissions = permissionService.getEffectivePermissionCodes(roleCode); + Map response = toAuthPayload(user, roleCode, permissions); impersonationService.decorateAuthPayload(httpRequest, user, response); return ResponseEntity.ok(response); } @@ -378,13 +379,17 @@ private ResponseEntity authResponse(PasswordlessAuthService.AuthFlowResult re if (!result.success()) { return ResponseEntity.status(400).body(Map.of("message", result.message())); } - if (result.sessionAuthentication() != null && result.user() != null && result.role() != null) { + // Gate on roleCode, not role: result.role() is null for a user holding a custom + // role, which sent an otherwise-successful login down the "challenge required" + // branch below and then NPE'd in Map.of on a null challengeId — a 500 on every + // custom-role login. Observed live, not inferred. + if (result.sessionAuthentication() != null && result.user() != null && result.roleCode() != null) { authSessionService.writeSessionCookies(httpResponse, result.sessionAuthentication()); authSessionService.clearImpersonationCookie(httpResponse); Set permissionNames = result.permissions() == null ? Set.of() : result.permissions().stream() .map(Enum::name) .collect(Collectors.toSet()); - return ResponseEntity.ok(toAuthPayload(result.user(), result.role(), permissionNames)); + return ResponseEntity.ok(toAuthPayload(result.user(), result.roleCode(), permissionNames)); } return ResponseEntity.ok(Map.of( "challengeId", result.nextChallengeId(), @@ -401,10 +406,19 @@ private ResponseEntity authError(ResponseStatusException e) { } private Map toAuthPayload(User user, Role role, Set permissions) { + return toAuthPayload(user, role != null ? role.name() : user.getRoleCode(), permissions); + } + + /** + * Auth payload keyed by role code, so a user holding a custom role reports + * that role rather than the nearest built-in one. + */ + private Map toAuthPayload(User user, String roleCode, Set permissions) { Map response = new LinkedHashMap<>(); response.put("username", user.getUsername()); response.put("email", user.getEmail()); - response.put("role", role.name()); + response.put("role", roleCode); + response.put("roleName", permissionService.describeRole(roleCode)); response.put("permissions", permissions); response.put("emailVerified", user.isEmailVerified()); response.put("accountStatus", user.getAccountStatus()); diff --git a/backend/src/main/java/com/dbaagent/controller/AuthInternalController.java b/backend/src/main/java/com/dbaagent/controller/AuthInternalController.java index 3223957..5f26700 100644 --- a/backend/src/main/java/com/dbaagent/controller/AuthInternalController.java +++ b/backend/src/main/java/com/dbaagent/controller/AuthInternalController.java @@ -60,11 +60,11 @@ public ResponseEntity issueToken( .body(Map.of("message", "Admin user not found")); } - Role role = admin.getRoleEnum(); - Set permissions = permissionService.getEffectivePermissions(role); + String roleCode = admin.getRoleCode(); + Set permissions = permissionService.getEffectivePermissions(roleCode); AuthSessionService.SessionAuthentication session = authSessionService.createSession( - admin, role, permissions, + admin, roleCode, permissions, request.getRemoteAddr(), "DeepSQL-TestSuite/1.0", true diff --git a/backend/src/main/java/com/dbaagent/controller/ImpersonationController.java b/backend/src/main/java/com/dbaagent/controller/ImpersonationController.java index 8810972..aec2339 100644 --- a/backend/src/main/java/com/dbaagent/controller/ImpersonationController.java +++ b/backend/src/main/java/com/dbaagent/controller/ImpersonationController.java @@ -81,12 +81,13 @@ public ResponseEntity> stop( } private Map toAuthPayload(User user, User impersonator) { - Role role = user.getRoleEnum(); - Set permissions = permissionService.getEffectivePermissionCodes(role); + String roleCode = user.getRoleCode(); + Set permissions = permissionService.getEffectivePermissionCodes(roleCode); Map payload = new LinkedHashMap<>(); payload.put("username", user.getUsername()); payload.put("email", user.getEmail()); - payload.put("role", role.name()); + payload.put("role", roleCode); + payload.put("roleName", permissionService.describeRole(roleCode)); payload.put("permissions", permissions); payload.put("emailVerified", user.isEmailVerified()); payload.put("accountStatus", user.getAccountStatus()); diff --git a/backend/src/main/java/com/dbaagent/controller/PermissionController.java b/backend/src/main/java/com/dbaagent/controller/PermissionController.java index 89b3bcb..174fce6 100644 --- a/backend/src/main/java/com/dbaagent/controller/PermissionController.java +++ b/backend/src/main/java/com/dbaagent/controller/PermissionController.java @@ -1,8 +1,10 @@ package com.dbaagent.controller; +import com.dbaagent.model.CustomRole; import com.dbaagent.model.Permission; import com.dbaagent.model.Role; import com.dbaagent.model.RolePermissionOverride; +import com.dbaagent.service.CustomRoleService; import com.dbaagent.service.PermissionService; import org.springframework.http.ResponseEntity; import org.springframework.security.access.prepost.PreAuthorize; @@ -13,31 +15,35 @@ import java.util.*; /** - * Controller for permission-related endpoints. + * Permission and role endpoints. * - * Public endpoints: - * - GET /api/permissions/me - Get current user's effective permissions - * - * Admin endpoints: - * - GET /api/permissions/registry - Get full permission registry - * - GET /api/permissions/roles - Get all roles with permissions - * - GET /api/permissions/overrides - Get all overrides - * - POST /api/permissions/overrides - Create/update an override - * - DELETE /api/permissions/overrides - Remove an override + * 

+ *   GET    /permissions/me            current user's effective permissions
+ *   GET    /permissions/registry      every permission and who holds it        (admin)
+ *   GET    /permissions/roles         built-in + custom roles                  (admin)
+ *   POST   /permissions/roles         create a custom role                     (admin)
+ *   PUT    /permissions/roles/{code}  update a custom role                     (admin)
+ *   DELETE /permissions/roles/{code}  delete an unused custom role             (admin)
+ *   GET    /permissions/overrides     all role-permission overrides            (admin)
+ *   POST   /permissions/overrides     create or update an override             (admin)
+ *   DELETE /permissions/overrides     remove an override                       (admin)
+ *
*/ @RestController @RequestMapping("/permissions") public class PermissionController { private final PermissionService permissionService; + private final CustomRoleService customRoleService; - public PermissionController(PermissionService permissionService) { + public PermissionController(PermissionService permissionService, CustomRoleService customRoleService) { this.permissionService = permissionService; + this.customRoleService = customRoleService; } /** - * Get the current user's effective permissions. - * This is called by frontend after login to know what the user can do. + * The current user's effective permissions — what the frontend uses to decide which + * menus and actions to show. */ @GetMapping("/me") public ResponseEntity getMyPermissions() { @@ -46,55 +52,113 @@ public ResponseEntity getMyPermissions() { return ResponseEntity.status(401).body(Map.of("error", "Not authenticated")); } - // Extract role from authorities - Role role = extractRoleFromAuthentication(auth); - Set permissions = permissionService.getEffectivePermissionCodes(role); + String roleCode = extractRoleCode(auth); + Set permissions = permissionService.getEffectivePermissionCodes(roleCode); Map response = new HashMap<>(); - response.put("role", role.name()); + response.put("role", roleCode); + response.put("roleName", permissionService.describeRole(roleCode)); response.put("permissions", permissions); response.put("permissionCount", permissions.size()); - return ResponseEntity.ok(response); } - /** - * Get the full permission registry. - * Shows all permissions with their default roles and any overrides. - */ @GetMapping("/registry") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity getPermissionRegistry() { List registry = permissionService.getPermissionRegistry(); + List> catalog = Arrays.stream(Permission.values()) + .map(p -> Map.of("code", p.name(), "description", p.getDescription())) + .toList(); return ResponseEntity.ok(Map.of( "permissions", registry, + "catalog", catalog, "totalPermissions", Permission.values().length, - "totalRoles", Role.values().length + "totalRoles", permissionService.getAllRoleCodes().size() )); } - /** - * Get all roles with their effective permissions. - */ @GetMapping("/roles") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity getRoles() { List roles = permissionService.getRoleRegistry(); - return ResponseEntity.ok(Map.of("roles", roles)); + for (PermissionService.RoleInfo role : roles) { + role.userCount = customRoleService.countUsersWithRole(role.code); + } + return ResponseEntity.ok(Map.of( + "roles", roles, + "catalog", Arrays.stream(Permission.values()) + .map(p -> Map.of("code", p.name(), "description", p.getDescription())) + .toList() + )); + } + + /** Create a custom role from a name plus an explicit permission list. */ + @PostMapping("/roles") + @PreAuthorize("hasRole('ADMIN')") + public ResponseEntity createCustomRole(@RequestBody Map request) { + try { + CustomRole role = customRoleService.createRole( + asString(request.get("name")), + asString(request.get("description")), + asStringList(request.get("permissions")), + currentActor() + ); + return ResponseEntity.ok(Map.of("success", true, "role", describe(role))); + } catch (IllegalArgumentException e) { + return ResponseEntity.badRequest().body(Map.of("success", false, "message", e.getMessage())); + } + } + + @PutMapping("/roles/{code}") + @PreAuthorize("hasRole('ADMIN')") + public ResponseEntity updateCustomRole(@PathVariable String code, @RequestBody Map request) { + try { + if (Role.isBuiltIn(code)) { + // Built-in role permission sets are code, not data. Changing one is what + // overrides are for, so the admin's change survives an upgrade. + return ResponseEntity.badRequest().body(Map.of( + "success", false, + "message", "Built-in roles cannot be edited directly. Use a permission override, " + + "or create a custom role." + )); + } + CustomRole role = customRoleService.updateRole( + code, + asString(request.get("name")), + request.containsKey("description") ? asString(request.get("description")) : null, + request.containsKey("permissions") ? asStringList(request.get("permissions")) : null, + currentActor() + ); + return ResponseEntity.ok(Map.of("success", true, "role", describe(role))); + } catch (IllegalArgumentException e) { + return ResponseEntity.badRequest().body(Map.of("success", false, "message", e.getMessage())); + } + } + + @DeleteMapping("/roles/{code}") + @PreAuthorize("hasRole('ADMIN')") + public ResponseEntity deleteCustomRole(@PathVariable String code) { + try { + if (Role.isBuiltIn(code)) { + return ResponseEntity.badRequest().body(Map.of( + "success", false, "message", "Built-in roles cannot be deleted")); + } + customRoleService.deleteRole(code, currentActor()); + return ResponseEntity.ok(Map.of("success", true, "message", "Role deleted")); + } catch (IllegalArgumentException e) { + return ResponseEntity.badRequest().body(Map.of("success", false, "message", e.getMessage())); + } } - /** - * Get all permission overrides. - */ @GetMapping("/overrides") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity getOverrides() { - List overrides = permissionService.getAllOverrides(); - List> overrideList = overrides.stream() + List> overrideList = permissionService.getAllOverrides().stream() .map(o -> { Map map = new HashMap<>(); map.put("id", o.getId()); - map.put("role", o.getRole().name()); + map.put("role", o.getRole()); map.put("permission", o.getPermissionCode().name()); map.put("granted", o.isGranted()); map.put("reason", o.getReason()); @@ -103,137 +167,137 @@ public ResponseEntity getOverrides() { return map; }) .toList(); - - return ResponseEntity.ok(Map.of( - "overrides", overrideList, - "totalOverrides", overrideList.size() - )); + return ResponseEntity.ok(Map.of("overrides", overrideList, "totalOverrides", overrideList.size())); } - /** - * Create or update a permission override. - */ @PostMapping("/overrides") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity setOverride(@RequestBody Map request) { try { - String roleStr = (String) request.get("role"); - String permissionStr = (String) request.get("permission"); + String roleCode = asString(request.get("role")); + String permissionStr = asString(request.get("permission")); Boolean granted = (Boolean) request.get("granted"); - String reason = (String) request.get("reason"); + String reason = asString(request.get("reason")); - if (roleStr == null || permissionStr == null || granted == null) { + if (roleCode == null || permissionStr == null || granted == null) { return ResponseEntity.badRequest().body(Map.of( - "error", "Missing required fields: role, permission, granted" - )); + "error", "Missing required fields: role, permission, granted")); } - Role role = Role.valueOf(roleStr.toUpperCase()); - Permission permission = Permission.valueOf(permissionStr.toUpperCase()); - - // Get current user for audit - Authentication auth = SecurityContextHolder.getContext().getAuthentication(); - String updatedBy = auth != null ? auth.getName() : "system"; + Permission permission = Permission.fromCode(permissionStr); + if (permission == null) { + return ResponseEntity.badRequest().body(Map.of("error", "Unknown permission: " + permissionStr)); + } RolePermissionOverride override = permissionService.setOverride( - role, permission, granted, reason, updatedBy - ); + roleCode, permission, granted, reason, currentActor()); + + Map overrideBody = new HashMap<>(); + overrideBody.put("id", override.getId()); + overrideBody.put("role", override.getRole()); + overrideBody.put("permission", override.getPermissionCode().name()); + overrideBody.put("granted", override.isGranted()); + overrideBody.put("reason", override.getReason()); return ResponseEntity.ok(Map.of( "success", true, - "message", granted ? - "Permission " + permission + " granted to " + role : - "Permission " + permission + " revoked from " + role, - "override", Map.of( - "id", override.getId(), - "role", override.getRole().name(), - "permission", override.getPermissionCode().name(), - "granted", override.isGranted(), - "reason", override.getReason() - ) + "message", (granted ? "Permission granted to " : "Permission revoked from ") + + permissionService.describeRole(roleCode), + "override", overrideBody )); } catch (IllegalArgumentException e) { - return ResponseEntity.badRequest().body(Map.of( - "error", "Invalid role or permission: " + e.getMessage() - )); + return ResponseEntity.badRequest().body(Map.of("error", e.getMessage())); } } - /** - * Remove a permission override (revert to default). - */ @DeleteMapping("/overrides") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity removeOverride(@RequestBody Map request) { - try { - String roleStr = request.get("role"); - String permissionStr = request.get("permission"); - - if (roleStr == null || permissionStr == null) { - return ResponseEntity.badRequest().body(Map.of( - "error", "Missing required fields: role, permission" - )); - } - - Role role = Role.valueOf(roleStr.toUpperCase()); - Permission permission = Permission.valueOf(permissionStr.toUpperCase()); - - permissionService.removeOverride(role, permission); - - return ResponseEntity.ok(Map.of( - "success", true, - "message", "Override removed. Permission " + permission + - " for " + role + " reverted to default behavior." - )); - } catch (IllegalArgumentException e) { + String roleCode = request.get("role"); + String permissionStr = request.get("permission"); + if (roleCode == null || permissionStr == null) { return ResponseEntity.badRequest().body(Map.of( - "error", "Invalid role or permission: " + e.getMessage() - )); + "error", "Missing required fields: role, permission")); } + Permission permission = Permission.fromCode(permissionStr); + if (permission == null) { + return ResponseEntity.badRequest().body(Map.of("error", "Unknown permission: " + permissionStr)); + } + permissionService.removeOverride(roleCode, permission); + return ResponseEntity.ok(Map.of( + "success", true, + "message", "Override removed; " + permission + " reverted to the role's default." + )); } - /** - * Check if a specific role has a specific permission. - */ + /** Whether a role holds a permission, and whether that differs from its default. */ @GetMapping("/check") - public ResponseEntity checkPermission( - @RequestParam String role, - @RequestParam String permission) { - try { - Role r = Role.valueOf(role.toUpperCase()); - Permission p = Permission.valueOf(permission.toUpperCase()); + public ResponseEntity checkPermission(@RequestParam String role, @RequestParam String permission) { + Permission p = Permission.fromCode(permission); + if (p == null) { + return ResponseEntity.badRequest().body(Map.of("error", "Unknown permission: " + permission)); + } + if (!permissionService.roleExists(role)) { + return ResponseEntity.badRequest().body(Map.of("error", "Unknown role: " + role)); + } + boolean granted = permissionService.hasPermission(role, p); + Role builtIn = Role.fromString(role); + boolean isDefault = builtIn != null && p.isGrantedByDefaultTo(builtIn); + return ResponseEntity.ok(Map.of( + "role", role.toUpperCase(), + "permission", p.name(), + "granted", granted, + "isDefault", isDefault, + "hasOverride", granted != isDefault + )); + } - boolean hasPermission = permissionService.hasPermission(r, p); - boolean isDefault = p.isGrantedByDefaultTo(r); + private static Map describe(CustomRole role) { + Map body = new HashMap<>(); + body.put("code", role.getCode()); + body.put("name", role.getName()); + body.put("description", role.getDescription()); + body.put("permissions", role.getPermissionCodeSet()); + body.put("builtIn", false); + return body; + } - return ResponseEntity.ok(Map.of( - "role", r.name(), - "permission", p.name(), - "granted", hasPermission, - "isDefault", isDefault, - "hasOverride", hasPermission != isDefault - )); - } catch (IllegalArgumentException e) { - return ResponseEntity.badRequest().body(Map.of( - "error", "Invalid role or permission: " + e.getMessage() - )); + private static String currentActor() { + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + return auth != null ? auth.getName() : "system"; + } + + private static String asString(Object value) { + if (value == null) return null; + String s = String.valueOf(value).trim(); + return s.isEmpty() ? null : s; + } + + @SuppressWarnings("unchecked") + private static List asStringList(Object value) { + if (value == null) return List.of(); + if (value instanceof Collection collection) { + return collection.stream().filter(Objects::nonNull).map(String::valueOf).toList(); } + return List.of(String.valueOf(value)); } - private Role extractRoleFromAuthentication(Authentication auth) { + /** + * The caller's role code from their granted authorities. + * + *

Custom roles carry a {@code ROLE_} authority just like built-in ones (see + * {@code CustomUserDetailsService}), so this returns the code as stored without + * needing to know whether it is built-in. + */ + private static String extractRoleCode(Authentication auth) { if (auth.getAuthorities() != null) { for (var authority : auth.getAuthorities()) { - String authorityStr = authority.getAuthority(); - if (authorityStr.startsWith("ROLE_")) { - String roleName = authorityStr.substring(5); - try { - return Role.valueOf(roleName.toUpperCase()); - } catch (IllegalArgumentException ignored) { - // Continue to next authority - } + String value = authority.getAuthority(); + if (value != null && value.startsWith("ROLE_")) { + return value.substring(5); } } } - return Role.DEVELOPER; // Default fallback + return Role.DEVELOPER.name(); } } diff --git a/backend/src/main/java/com/dbaagent/model/CustomRole.java b/backend/src/main/java/com/dbaagent/model/CustomRole.java new file mode 100644 index 0000000..f624806 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/model/CustomRole.java @@ -0,0 +1,126 @@ +package com.dbaagent.model; + +import jakarta.persistence.*; +import java.time.LocalDateTime; +import java.util.Collections; +import java.util.EnumSet; +import java.util.LinkedHashSet; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * An admin-defined role: a name plus an explicit set of permissions. + * + *

Unlike the built-in {@link Role} values, a custom role has no defaults to inherit — + * its permission set is exactly what an admin ticked, stored as a comma-separated list of + * {@link Permission} codes. Unknown codes (a permission removed in a later release) are + * dropped on read rather than failing, so an old row cannot break login. + * + *

The {@code code} is the value written to {@code users.role}, so it shares a namespace + * with the built-in role names; {@code DashboardWorkspaceService} and the role admin API + * both refuse to create a custom role whose code collides with a built-in one. + */ +@Entity +@Table(name = "custom_roles", uniqueConstraints = { + @UniqueConstraint(name = "ux_custom_roles_code", columnNames = {"code"}) +}) +public class CustomRole { + + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + private Long id; + + /** Stable uppercase identifier written to {@code users.role}. */ + @Column(nullable = false, length = 64) + private String code; + + @Column(nullable = false, length = 128) + private String name; + + @Column(length = 500) + private String description; + + /** Comma-separated {@link Permission} codes. */ + @Column(name = "permission_codes", columnDefinition = "TEXT") + private String permissionCodes; + + @Column(name = "created_at", nullable = false) + private LocalDateTime createdAt = LocalDateTime.now(); + + @Column(name = "updated_at", nullable = false) + private LocalDateTime updatedAt = LocalDateTime.now(); + + @Column(name = "created_by", length = 255) + private String createdBy; + + @Column(name = "updated_by", length = 255) + private String updatedBy; + + @PreUpdate + public void preUpdate() { + this.updatedAt = LocalDateTime.now(); + } + + /** The permissions this role grants, with unknown codes dropped. */ + @Transient + public Set getPermissions() { + if (permissionCodes == null || permissionCodes.isBlank()) { + return EnumSet.noneOf(Permission.class); + } + Set resolved = EnumSet.noneOf(Permission.class); + for (String raw : permissionCodes.split(",")) { + Permission permission = Permission.fromCode(raw); + if (permission != null) { + resolved.add(permission); + } + } + return resolved; + } + + public void setPermissions(Set permissions) { + if (permissions == null || permissions.isEmpty()) { + this.permissionCodes = ""; + return; + } + // LinkedHashSet over the enum's natural order keeps the stored string stable, so + // an unchanged permission set does not produce a spurious row update. + this.permissionCodes = new LinkedHashSet<>(EnumSet.copyOf(permissions)).stream() + .map(Enum::name) + .collect(Collectors.joining(",")); + } + + @Transient + public Set getPermissionCodeSet() { + Set codes = getPermissions().stream() + .map(Enum::name) + .collect(Collectors.toCollection(LinkedHashSet::new)); + return Collections.unmodifiableSet(codes); + } + + public Long getId() { return id; } + public void setId(Long id) { this.id = id; } + + public String getCode() { return code; } + public void setCode(String code) { this.code = code; } + + public String getName() { return name; } + public void setName(String name) { this.name = name; } + + public String getDescription() { return description; } + public void setDescription(String description) { this.description = description; } + + public String getPermissionCodes() { return permissionCodes; } + public void setPermissionCodes(String permissionCodes) { this.permissionCodes = permissionCodes; } + + public LocalDateTime getCreatedAt() { return createdAt; } + public void setCreatedAt(LocalDateTime createdAt) { this.createdAt = createdAt; } + + public LocalDateTime getUpdatedAt() { return updatedAt; } + public void setUpdatedAt(LocalDateTime updatedAt) { this.updatedAt = updatedAt; } + + public String getCreatedBy() { return createdBy; } + public void setCreatedBy(String createdBy) { this.createdBy = createdBy; } + + public String getUpdatedBy() { return updatedBy; } + public void setUpdatedBy(String updatedBy) { this.updatedBy = updatedBy; } +} diff --git a/backend/src/main/java/com/dbaagent/model/Permission.java b/backend/src/main/java/com/dbaagent/model/Permission.java index ac7e814..dce662f 100644 --- a/backend/src/main/java/com/dbaagent/model/Permission.java +++ b/backend/src/main/java/com/dbaagent/model/Permission.java @@ -2,69 +2,103 @@ /** * Enumeration of all permissions in the system. - * Each permission has a description and a default minimum role. * - * The defaultMinRole determines which roles get this permission by default - * through role hierarchy inheritance: - * - DEVELOPER permissions: Available to DEVELOPER, ADMIN - * - ADMIN permissions: Available to ADMIN only + *

Permissions are the unit of authorization. A {@link Role} is nothing more than a + * named bundle of these, and a {@link CustomRole} is a bundle an admin defined at + * runtime. There is no longer a role hierarchy: the built-in roles overlap without + * nesting (Data Engineer sees Dashboards but not Digest; Developer sees Digest but + * cannot touch connection settings), so "is role A at least role B" is not a question + * with an answer. Ask whether a role holds a permission instead. * - * This can be overridden via RolePermissionOverride for exceptions. + *

{@link #defaultRoles} lists which built-in roles hold the permission out of the box. + * {@link RolePermissionOverride} can still add or remove one per role. */ public enum Permission { - // ==================== ADMIN PRODUCT PERMISSIONS ==================== - VIEW_DASHBOARD("View dashboards and overview information", Role.ADMIN), - VIEW_SCHEMA("Browse database schema, tables, and columns", Role.ADMIN), - VIEW_SLOW_QUERIES("View slow query analysis and history", Role.ADMIN), - VIEW_BRAIN("View Brain overview and database insights", Role.ADMIN), - VIEW_PERFORMANCE("View performance metrics and insights", Role.ADMIN), - VIEW_GROWTH("View growth monitoring data", Role.ADMIN), - VIEW_PLAYBOOKS("View playbook definitions and history", Role.ADMIN), + // ==================== SECTION / MENU PERMISSIONS ==================== + // One per top-level sidebar destination. These drive both the nav in the UI and + // the server-side section checks, so a hidden menu is not merely cosmetic. + VIEW_AGENT("Open the Agent chat section", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), + VIEW_DASHBOARDS("Open the Dashboards section", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), + VIEW_DIGEST("Open the Digest section", Role.ADMIN, Role.DBA, Role.DEVELOPER), + VIEW_BRAIN("Open the Brain section and database insights", Role.ADMIN, Role.DBA), + VIEW_PERFORMANCE("Open the Performance section (slow queries and workload)", Role.ADMIN, Role.DBA, Role.DEVELOPER), + VIEW_EDITOR("Open the SQL Editor section", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), - // ==================== DEVELOPER PERMISSIONS ==================== - EXECUTE_QUERIES("Execute SQL queries in SQL Runner", Role.DEVELOPER), - USE_CHAT("Use the AI chat assistant", Role.DEVELOPER), - EXPORT_DATA("Export query results and reports", Role.DEVELOPER), + // ==================== READ PERMISSIONS ==================== + VIEW_DASHBOARD("View dashboards and overview information", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), + VIEW_SCHEMA("Browse database schema, tables, and columns", Role.ADMIN, Role.DBA), + VIEW_SLOW_QUERIES("View slow query analysis and history", Role.ADMIN, Role.DBA, Role.DEVELOPER), + VIEW_GROWTH("View growth monitoring data", Role.ADMIN, Role.DBA), + VIEW_PLAYBOOKS("View playbook definitions and history", Role.ADMIN, Role.DBA), - // ==================== ADMIN ACTION PERMISSIONS ==================== - RUN_ANALYSIS("Run analysis tasks (Key Columns, Schema, Anti-patterns)", Role.ADMIN), - RUN_INGESTION("Run slow query log ingestion", Role.ADMIN), - EXECUTE_PLAYBOOKS("Execute playbooks and automation", Role.ADMIN), - USE_INDEX_ADVISOR("Apply Index Advisor recommendations", Role.ADMIN), - MANAGE_ALERTS("Acknowledge and manage alerts", Role.ADMIN), + // ==================== CORE PRODUCT PERMISSIONS ==================== + EXECUTE_QUERIES("Execute SQL queries in SQL Runner", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), + USE_CHAT("Use the AI chat assistant", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), + EXPORT_DATA("Export query results and reports", Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), - // ==================== ADMIN PERMISSIONS (ADMIN ONLY) ==================== - MANAGE_CONNECTIONS("Create, edit, and delete database connections", Role.ADMIN), + // ==================== ACTION PERMISSIONS ==================== + RUN_ANALYSIS("Run analysis tasks (Key Columns, Schema, Anti-patterns)", Role.ADMIN, Role.DBA), + RUN_INGESTION("Run slow query log ingestion", Role.ADMIN, Role.DBA), + EXECUTE_PLAYBOOKS("Execute playbooks and automation", Role.ADMIN, Role.DBA), + USE_INDEX_ADVISOR("Apply Index Advisor recommendations", Role.ADMIN, Role.DBA), + MANAGE_ALERTS("Acknowledge and manage alerts", Role.ADMIN, Role.DBA), + + // ==================== WORKSPACE PERMISSIONS ==================== + // Creating a workspace is a normal authoring action for anyone who can build + // dashboards; granting other people access to one is an administrative act and + // is checked per-workspace on top of this (see DashboardWorkspaceService). + MANAGE_DASHBOARD_WORKSPACES("Create dashboard workspaces and manage their members", + Role.ADMIN, Role.DBA, Role.DATA_ENGINEER, Role.DEVELOPER), + + // ==================== ADMINISTRATIVE PERMISSIONS ==================== + // DBA gets connection settings but explicitly NOT user creation. + MANAGE_CONNECTIONS("Create, edit, and delete database connections", Role.ADMIN, Role.DBA), + MANAGE_SETTINGS("Modify system settings and configurations", Role.ADMIN, Role.DBA), MANAGE_USERS("View, edit roles, and delete users", Role.ADMIN), MANAGE_INVITE_CODES("Generate and manage invite codes", Role.ADMIN), - MANAGE_SETTINGS("Modify system settings and configurations", Role.ADMIN), - MANAGE_PERMISSIONS("Manage role-permission overrides", Role.ADMIN); + MANAGE_PERMISSIONS("Manage roles and role-permission overrides", Role.ADMIN); private final String description; - private final Role defaultMinRole; + private final java.util.Set defaultRoles; - Permission(String description, Role defaultMinRole) { + Permission(String description, Role... defaultRoles) { this.description = description; - this.defaultMinRole = defaultMinRole; + this.defaultRoles = defaultRoles.length == 0 + ? java.util.EnumSet.noneOf(Role.class) + : java.util.EnumSet.copyOf(java.util.Arrays.asList(defaultRoles)); } public String getDescription() { return description; } - /** - * The minimum role that gets this permission by default. - * Higher roles in the hierarchy automatically inherit this permission. - */ - public Role getDefaultMinRole() { - return defaultMinRole; + /** The built-in roles that hold this permission by default. */ + public java.util.Set getDefaultRoles() { + return java.util.Collections.unmodifiableSet(defaultRoles); } /** - * Check if a role has this permission by default (via hierarchy). - * A role has the permission if it's at or above the defaultMinRole. + * Check if a role has this permission by default, before overrides. + * + *

ADMIN always holds every permission — an admin who could be locked out of + * user management by an override would be an unrecoverable install. */ public boolean isGrantedByDefaultTo(Role role) { - return role.isAtLeast(defaultMinRole); + if (role == null) { + return false; + } + return role == Role.ADMIN || defaultRoles.contains(role); + } + + /** Parse a permission code leniently; returns null when unknown. */ + public static Permission fromCode(String code) { + if (code == null || code.isBlank()) { + return null; + } + try { + return Permission.valueOf(code.trim().toUpperCase()); + } catch (IllegalArgumentException e) { + return null; + } } } diff --git a/backend/src/main/java/com/dbaagent/model/Role.java b/backend/src/main/java/com/dbaagent/model/Role.java index ce3c452..4f4d9c0 100644 --- a/backend/src/main/java/com/dbaagent/model/Role.java +++ b/backend/src/main/java/com/dbaagent/model/Role.java @@ -6,110 +6,104 @@ import java.util.stream.Collectors; /** - * Enumeration of user roles in the system. - * Roles are hierarchical: each role inherits permissions from lower roles. + * Built-in user roles. * - * Hierarchy (lowest to highest): - * DEVELOPER (0) → ADMIN (1) + *

These are not a hierarchy. The old model ranked DEVELOPER below ADMIN and + * compared roles with {@code ordinal()}; the current roles deliberately overlap without + * nesting — DATA_ENGINEER can open Dashboards but not Digest, DEVELOPER can open Digest + * but cannot edit connection settings — so there is no ordering to compare. Every + * authorization question is "does this role hold permission X", answered by + * {@link Permission#isGrantedByDefaultTo} and, with overrides applied, by + * {@code PermissionService}. * - * Permission assignment works via Permission.defaultMinRole: - * - Permission with defaultMinRole=DEVELOPER → granted to DEVELOPER, ADMIN - * - Permission with defaultMinRole=ADMIN → granted to ADMIN only - * - * This can be overridden via RolePermissionOverride for exceptions. + *

Roles beyond these are defined at runtime as {@link CustomRole} rows. A user's + * {@code role} column holds either one of these names or a custom role's code, which is + * why {@link #fromString} returns null for anything unrecognised rather than silently + * downgrading to DEVELOPER — a custom role name must not be mistaken for a built-in one. */ public enum Role { - /** - * DEVELOPER: Default product user. - * Can use Chat and the SQL Editor. - */ - DEVELOPER("Access to Chat and the SQL Editor"), + /** Full access to every product area and all administrative controls. */ + ADMIN("Admin", "Full access to all product areas and administrative controls"), - /** - * ADMIN: Full access. - * Inherits all DEVELOPER permissions plus access to all product areas and admin controls. - */ - ADMIN("Access to all product areas and administrative controls"); + /** All menu items and connection settings, but not user creation. */ + DBA("DBA", "All product areas and connection settings, except user management"), + + /** Agent, Dashboards, and the SQL Editor. */ + DATA_ENGINEER("Data Engineer", "Agent, Dashboards, and the SQL Editor"), + + /** Agent, Digest, Dashboards, Performance, and the SQL Editor. */ + DEVELOPER("Developer", "Agent, Digest, Dashboards, Performance, and the SQL Editor"); + private final String displayName; private final String description; - Role(String description) { + Role(String displayName, String description) { + this.displayName = displayName; this.description = description; } + public String getDisplayName() { + return displayName; + } + public String getDescription() { return description; } - /** - * Get the default permissions for this role based on hierarchy. - * A role gets all permissions where permission.defaultMinRole <= this role. - */ + /** The permissions this role holds by default, before any override is applied. */ public Set getDefaultPermissions() { return Arrays.stream(Permission.values()) .filter(p -> p.isGrantedByDefaultTo(this)) .collect(Collectors.toCollection(() -> EnumSet.noneOf(Permission.class))); } - /** - * Get all permissions for this role. - * Alias for getDefaultPermissions() for simpler API usage. - */ + /** Alias for {@link #getDefaultPermissions()}. */ public Set getPermissions() { return getDefaultPermissions(); } - /** - * Check if this role has a permission by default (without considering overrides). - */ + /** Whether this role holds the permission by default (ignores overrides). */ public boolean hasPermissionByDefault(Permission permission) { - return permission.isGrantedByDefaultTo(this); + return permission != null && permission.isGrantedByDefaultTo(this); } - /** - * Check if this role has a specific permission. - * Alias for hasPermissionByDefault() for simpler API usage. - */ + /** Alias for {@link #hasPermissionByDefault}. */ public boolean hasPermission(Permission permission) { return hasPermissionByDefault(permission); } - /** - * Check if this role is at or above another role in the hierarchy. - * Used for permission inheritance. - */ - public boolean isAtLeast(Role other) { - return this.ordinal() >= other.ordinal(); + public boolean isAdmin() { + return this == ADMIN; } /** - * Check if this role is strictly above another role in the hierarchy. - */ - public boolean isAbove(Role other) { - return this.ordinal() > other.ordinal(); - } - - /** - * Get a role by name, case-insensitive. - * Legacy roles collapse into DEVELOPER for backward compatibility. + * Resolve a built-in role by name, case-insensitive. + * + *

Returns {@code null} when the name is not a built-in role — the caller is then + * expected to look for a {@link CustomRole} with that code. Legacy role names that + * predate this enum collapse into DEVELOPER, which is what installs upgrading from + * the two-role model carry in their {@code users.role} column. */ public static Role fromString(String roleName) { if (roleName == null || roleName.isBlank()) { - return DEVELOPER; + return null; } return switch (roleName.trim().toUpperCase()) { case "ADMIN" -> ADMIN; + case "DBA" -> DBA; + case "DATA_ENGINEER", "DATA-ENGINEER", "DATAENGINEER" -> DATA_ENGINEER; case "DEVELOPER", "EDITOR", "VIEWER", "USER" -> DEVELOPER; - default -> DEVELOPER; + default -> null; }; } - /** - * Get all roles at or above the given minimum role. - */ - public static Set getRolesAtOrAbove(Role minRole) { - return Arrays.stream(values()) - .filter(r -> r.isAtLeast(minRole)) - .collect(Collectors.toSet()); + /** As {@link #fromString}, but falls back to DEVELOPER instead of returning null. */ + public static Role fromStringOrDefault(String roleName) { + Role role = fromString(roleName); + return role != null ? role : DEVELOPER; + } + + public static boolean isBuiltIn(String roleName) { + return fromString(roleName) != null; } } diff --git a/backend/src/main/java/com/dbaagent/model/RolePermissionOverride.java b/backend/src/main/java/com/dbaagent/model/RolePermissionOverride.java index d2f1427..35fc826 100644 --- a/backend/src/main/java/com/dbaagent/model/RolePermissionOverride.java +++ b/backend/src/main/java/com/dbaagent/model/RolePermissionOverride.java @@ -23,9 +23,14 @@ public class RolePermissionOverride { @GeneratedValue(strategy = GenerationType.IDENTITY) private Long id; - @Column(nullable = false) - @Enumerated(EnumType.STRING) - private Role role; + /** + * The role this override applies to, as a role code: either a built-in {@link Role} + * name or a {@link CustomRole#getCode()}. It was an {@code @Enumerated} {@link Role} + * before custom roles existed; the column is unchanged (both forms are the same + * uppercase string), so existing rows keep working. + */ + @Column(name = "role", nullable = false, length = 64) + private String role; @Column(name = "permission_code", nullable = false) @Enumerated(EnumType.STRING) @@ -58,7 +63,7 @@ public RolePermissionOverride() { this.updatedAt = LocalDateTime.now(); } - public RolePermissionOverride(Role role, Permission permissionCode, boolean granted, String updatedBy) { + public RolePermissionOverride(String role, Permission permissionCode, boolean granted, String updatedBy) { this(); this.role = role; this.permissionCode = permissionCode; @@ -66,7 +71,7 @@ public RolePermissionOverride(Role role, Permission permissionCode, boolean gran this.updatedBy = updatedBy; } - public RolePermissionOverride(Role role, Permission permissionCode, boolean granted, String reason, String updatedBy) { + public RolePermissionOverride(String role, Permission permissionCode, boolean granted, String reason, String updatedBy) { this(role, permissionCode, granted, updatedBy); this.reason = reason; } @@ -85,11 +90,11 @@ public void setId(Long id) { this.id = id; } - public Role getRole() { + public String getRole() { return role; } - public void setRole(Role role) { + public void setRole(String role) { this.role = role; } diff --git a/backend/src/main/java/com/dbaagent/model/User.java b/backend/src/main/java/com/dbaagent/model/User.java index a05f7b9..28262ef 100644 --- a/backend/src/main/java/com/dbaagent/model/User.java +++ b/backend/src/main/java/com/dbaagent/model/User.java @@ -60,14 +60,26 @@ public class User { private LocalDateTime invitedAt; /** - * Get the Role enum for this user. - * Returns DEVELOPER as default if role is null or invalid. + * The built-in {@link Role} for this user, or null when {@code role} names a + * custom role. + * + *

It used to fall back to DEVELOPER for anything unrecognised. That is wrong now + * that custom roles exist: a user holding the custom role "ANALYST" would report + * itself as a DEVELOPER and inherit Developer's permissions instead of the ones the + * admin ticked. Callers that need a permission answer must go through + * {@code PermissionService}, which resolves both kinds by role code. */ @Transient public Role getRoleEnum() { return Role.fromString(this.role); } + /** The user's role code, built-in or custom, as stored. */ + @Transient + public String getRoleCode() { + return role == null || role.isBlank() ? Role.DEVELOPER.name() : role.trim().toUpperCase(); + } + /** * Set the role from a Role enum. */ @@ -76,27 +88,38 @@ public void setRoleEnum(Role role) { } /** - * Get all permissions for this user based on their role. + * Default permissions for this user's built-in role. + * + *

Empty for a custom role — the authoritative answer for any role lives in + * {@code PermissionService.getEffectivePermissions(roleCode)}, which also applies + * overrides. This remains only for callers that already had a built-in role in hand. */ @Transient public Set getPermissions() { - return getRoleEnum().getPermissions(); + Role builtIn = getRoleEnum(); + return builtIn == null ? java.util.EnumSet.noneOf(Permission.class) : builtIn.getPermissions(); } /** - * Check if this user has a specific permission. + * Whether this user's built-in role holds a permission by default. + * Custom roles and overrides are not consulted here; use {@code PermissionService}. */ @Transient public boolean hasPermission(Permission permission) { - return getRoleEnum().hasPermission(permission); + Role builtIn = getRoleEnum(); + return builtIn != null && builtIn.hasPermission(permission); } /** - * Check if this user has at least the specified role level. + * Whether this user holds exactly the given built-in role. + * + *

Formerly {@code isAtLeast}, a rank comparison. The roles no longer form a + * hierarchy — DATA_ENGINEER and DEVELOPER each have menus the other lacks — so an + * ordering comparison has no meaning and would silently answer nonsense. */ @Transient public boolean hasRole(Role requiredRole) { - return getRoleEnum().isAtLeast(requiredRole); + return requiredRole != null && getRoleEnum() == requiredRole; } /** diff --git a/backend/src/main/java/com/dbaagent/repository/CustomRoleRepository.java b/backend/src/main/java/com/dbaagent/repository/CustomRoleRepository.java new file mode 100644 index 0000000..06c3ecb --- /dev/null +++ b/backend/src/main/java/com/dbaagent/repository/CustomRoleRepository.java @@ -0,0 +1,18 @@ +package com.dbaagent.repository; + +import com.dbaagent.model.CustomRole; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; + +import java.util.List; +import java.util.Optional; + +@Repository +public interface CustomRoleRepository extends JpaRepository { + + Optional findByCodeIgnoreCase(String code); + + boolean existsByCodeIgnoreCase(String code); + + List findAllByOrderByNameAsc(); +} diff --git a/backend/src/main/java/com/dbaagent/repository/RolePermissionOverrideRepository.java b/backend/src/main/java/com/dbaagent/repository/RolePermissionOverrideRepository.java index 896e21c..68aece7 100644 --- a/backend/src/main/java/com/dbaagent/repository/RolePermissionOverrideRepository.java +++ b/backend/src/main/java/com/dbaagent/repository/RolePermissionOverrideRepository.java @@ -1,7 +1,6 @@ package com.dbaagent.repository; import com.dbaagent.model.Permission; -import com.dbaagent.model.Role; import com.dbaagent.model.RolePermissionOverride; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.Query; @@ -16,32 +15,32 @@ public interface RolePermissionOverrideRepository extends JpaRepository findByRole(Role role); + List findByRoleIgnoreCase(String role); /** * Find a specific override by role and permission. */ - Optional findByRoleAndPermissionCode(Role role, Permission permissionCode); + Optional findByRoleIgnoreCaseAndPermissionCode(String role, Permission permissionCode); /** * Find all overrides that grant permissions (used for adding permissions to roles). */ - List findByRoleAndGrantedTrue(Role role); + List findByRoleIgnoreCaseAndGrantedTrue(String role); /** * Find all overrides that revoke permissions (used for removing permissions from roles). */ - List findByRoleAndGrantedFalse(Role role); + List findByRoleIgnoreCaseAndGrantedFalse(String role); /** * Delete an override by role and permission. */ - void deleteByRoleAndPermissionCode(Role role, Permission permissionCode); + void deleteByRoleIgnoreCaseAndPermissionCode(String role, Permission permissionCode); /** * Check if an override exists for a role-permission pair. */ - boolean existsByRoleAndPermissionCode(Role role, Permission permissionCode); + boolean existsByRoleIgnoreCaseAndPermissionCode(String role, Permission permissionCode); /** * Count overrides (for admin dashboard). diff --git a/backend/src/main/java/com/dbaagent/repository/UserRepository.java b/backend/src/main/java/com/dbaagent/repository/UserRepository.java index b58cd6e..3636745 100644 --- a/backend/src/main/java/com/dbaagent/repository/UserRepository.java +++ b/backend/src/main/java/com/dbaagent/repository/UserRepository.java @@ -12,4 +12,7 @@ public interface UserRepository extends JpaRepository { Optional findByEmailIgnoreCase(String email); boolean existsByEmailIgnoreCase(String email); List findAllByAccountStatus(String accountStatus); + + /** How many users hold this role code. Guards deletion of a custom role in use. */ + long countByRoleIgnoreCase(String role); } diff --git a/backend/src/main/java/com/dbaagent/security/CustomUserDetailsService.java b/backend/src/main/java/com/dbaagent/security/CustomUserDetailsService.java index 9381c51..93cbea7 100644 --- a/backend/src/main/java/com/dbaagent/security/CustomUserDetailsService.java +++ b/backend/src/main/java/com/dbaagent/security/CustomUserDetailsService.java @@ -1,7 +1,6 @@ package com.dbaagent.security; import com.dbaagent.model.Permission; -import com.dbaagent.model.Role; import com.dbaagent.model.User; import com.dbaagent.repository.UserRepository; import com.dbaagent.service.PermissionService; @@ -50,19 +49,22 @@ public Optional loadUserEntityByUsername(String username) { } /** - * Build Spring Security authorities from user's role and permissions. - * Includes both ROLE_xxx authority and individual permission authorities. + * Build Spring Security authorities from the user's role code and its effective + * permissions. + * + *

The role code is used verbatim, so a custom role yields {@code ROLE_ANALYST} + * exactly as a built-in one yields {@code ROLE_DBA}. Resolving through + * {@link PermissionService} rather than {@code Role.getPermissions()} is what makes + * custom roles and permission overrides take effect at login — reading the enum + * directly would give a custom-role user nothing at all. */ private Collection buildAuthorities(User user) { List authorities = new ArrayList<>(); - Role role = user.getRoleEnum(); + String roleCode = user.getRoleCode(); + authorities.add(new SimpleGrantedAuthority("ROLE_" + roleCode)); - // Add role authority (e.g., ROLE_ADMIN, ROLE_DEVELOPER) - authorities.add(new SimpleGrantedAuthority("ROLE_" + role.name())); - - // Add individual permission authorities - for (Permission permission : permissionService.getEffectivePermissions(role)) { + for (Permission permission : permissionService.getEffectivePermissions(roleCode)) { authorities.add(new SimpleGrantedAuthority(permission.name())); } diff --git a/backend/src/main/java/com/dbaagent/security/JwtUtil.java b/backend/src/main/java/com/dbaagent/security/JwtUtil.java index 1cdfc99..d21f698 100644 --- a/backend/src/main/java/com/dbaagent/security/JwtUtil.java +++ b/backend/src/main/java/com/dbaagent/security/JwtUtil.java @@ -145,9 +145,31 @@ public String generateAccessToken( Set permissions, Duration ttl, Long impersonateUserId + ) { + return generateAccessToken(username, sessionId, role == null ? null : role.name(), + permissions, ttl, impersonateUserId); + } + + /** + * Mint an access token for a role code, which may name a built-in + * {@link Role} or a custom one. + * + *

The Role-typed overloads delegate here. They cannot represent a custom role, so + * every caller that resolves a user's role from the database must use this variant — + * otherwise a custom-role user's token would carry the wrong role claim. + */ + public String generateAccessToken( + String username, + String sessionId, + String roleCode, + Set permissions, + Duration ttl, + Long impersonateUserId ) { Map claims = new HashMap<>(); - claims.put("role", role.name()); + claims.put("role", roleCode == null || roleCode.isBlank() + ? Role.DEVELOPER.name() + : roleCode.trim().toUpperCase()); List permissionNames = permissions.stream() .map(Permission::name) diff --git a/backend/src/main/java/com/dbaagent/service/AuthSessionService.java b/backend/src/main/java/com/dbaagent/service/AuthSessionService.java index 3effbf0..acc4b7b 100644 --- a/backend/src/main/java/com/dbaagent/service/AuthSessionService.java +++ b/backend/src/main/java/com/dbaagent/service/AuthSessionService.java @@ -31,6 +31,7 @@ public class AuthSessionService { private final UserSessionRepository userSessionRepository; private final JwtUtil jwtUtil; private final SecurityEventService securityEventService; + private final PermissionService permissionService; @Value("${security.cookie.name:auth_token}") private String accessCookieName; @@ -56,7 +57,7 @@ public class AuthSessionService { @Transactional public SessionAuthentication createSession( User user, - Role role, + String roleCode, Set permissions, String clientIp, String userAgent, @@ -79,9 +80,10 @@ public SessionAuthentication createSession( String accessToken = jwtUtil.generateAccessToken( user.getUsername(), session.getId(), - role, + roleCode, permissions, - Duration.ofMinutes(accessMinutes) + Duration.ofMinutes(accessMinutes), + null ); securityEventService.log(SecurityEventService.EventRequest.builder() @@ -119,7 +121,7 @@ public Optional findValidSessionByRefreshToken(String refreshTokenH public Optional refreshSession( String rawRefreshToken, User user, - Role role, + String roleCode, Set permissions, String clientIp, String userAgent @@ -129,7 +131,7 @@ public Optional refreshSession( .filter(session -> !session.isRevoked()) .filter(session -> !session.isRefreshExpired()) .filter(session -> session.getUserId().equals(user.getId())) - .map(session -> rotateSession(session, user, role, permissions, clientIp, userAgent)); + .map(session -> rotateSession(session, user, roleCode, permissions, clientIp, userAgent)); } @Transactional @@ -169,12 +171,14 @@ public void reissueAccessToken( if (response == null || sessionId == null || sessionId.isBlank() || sessionOwner == null) { return; } - Role role = sessionOwner.getRoleEnum(); + // Resolve by role code, not Role enum: a custom-role user has no Role value, and + // Role.getPermissions() would also skip any admin-configured override. + String roleCode = sessionOwner.getRoleCode(); String accessToken = jwtUtil.generateAccessToken( sessionOwner.getUsername(), sessionId, - role, - role.getPermissions(), + roleCode, + permissionService.getEffectivePermissions(roleCode), Duration.ofMinutes(accessMinutes), impersonateUserId ); @@ -209,7 +213,7 @@ public void clearSessionCookies(HttpServletResponse response) { private SessionAuthentication rotateSession( UserSession session, User user, - Role role, + String roleCode, Set permissions, String clientIp, String userAgent @@ -226,9 +230,10 @@ private SessionAuthentication rotateSession( String accessToken = jwtUtil.generateAccessToken( user.getUsername(), session.getId(), - role, + roleCode, permissions, - Duration.ofMinutes(accessMinutes) + Duration.ofMinutes(accessMinutes), + null ); securityEventService.log(SecurityEventService.EventRequest.builder() diff --git a/backend/src/main/java/com/dbaagent/service/CustomRoleService.java b/backend/src/main/java/com/dbaagent/service/CustomRoleService.java new file mode 100644 index 0000000..49955bb --- /dev/null +++ b/backend/src/main/java/com/dbaagent/service/CustomRoleService.java @@ -0,0 +1,162 @@ +package com.dbaagent.service; + +import com.dbaagent.model.CustomRole; +import com.dbaagent.model.Permission; +import com.dbaagent.model.Role; +import com.dbaagent.repository.CustomRoleRepository; +import com.dbaagent.repository.RolePermissionOverrideRepository; +import com.dbaagent.repository.UserRepository; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; + +import java.util.Collection; +import java.util.EnumSet; +import java.util.List; +import java.util.Locale; +import java.util.Set; + +/** + * CRUD for admin-defined roles. + * + *

A custom role's {@code code} shares a namespace with the built-in {@link Role} names + * because both are written to {@code users.role}, so creation refuses a code that + * collides with a built-in one. Deletion refuses while any user still holds the role — + * silently reassigning people is not a decision this service should make on its own, and + * an orphaned code would resolve to no permissions at their next login. + */ +@Service +@RequiredArgsConstructor +@Slf4j +public class CustomRoleService { + + private final CustomRoleRepository customRoleRepository; + private final RolePermissionOverrideRepository overrideRepository; + private final UserRepository userRepository; + + public List listRoles() { + return customRoleRepository.findAllByOrderByNameAsc(); + } + + public CustomRole getByCode(String code) { + return customRoleRepository.findByCodeIgnoreCase(normalizeCode(code)) + .orElseThrow(() -> new IllegalArgumentException("Custom role not found: " + code)); + } + + @Transactional + public CustomRole createRole(String name, String description, Collection permissionCodes, String actor) { + String cleanName = requireName(name); + String code = deriveCode(cleanName); + + if (Role.isBuiltIn(code)) { + throw new IllegalArgumentException( + "\"" + cleanName + "\" collides with the built-in " + code + " role. Choose a different name."); + } + if (customRoleRepository.existsByCodeIgnoreCase(code)) { + throw new IllegalArgumentException("A role named \"" + cleanName + "\" already exists"); + } + + CustomRole role = new CustomRole(); + role.setCode(code); + role.setName(cleanName); + role.setDescription(trimToNull(description)); + role.setPermissions(resolvePermissions(permissionCodes)); + role.setCreatedBy(actor); + role.setUpdatedBy(actor); + + CustomRole saved = customRoleRepository.save(role); + log.info("Custom role created: code={}, permissions={}, by={}", + saved.getCode(), saved.getPermissions().size(), actor); + return saved; + } + + @Transactional + public CustomRole updateRole(String code, String name, String description, + Collection permissionCodes, String actor) { + CustomRole role = getByCode(code); + + if (name != null && !name.isBlank()) { + // The code is the identity written to users.role, so renaming changes only the + // label. Re-deriving the code would orphan every user holding the old one. + role.setName(requireName(name)); + } + if (description != null) { + role.setDescription(trimToNull(description)); + } + if (permissionCodes != null) { + role.setPermissions(resolvePermissions(permissionCodes)); + } + role.setUpdatedBy(actor); + + CustomRole saved = customRoleRepository.save(role); + log.info("Custom role updated: code={}, by={}", saved.getCode(), actor); + return saved; + } + + @Transactional + public void deleteRole(String code, String actor) { + CustomRole role = getByCode(code); + long holders = userRepository.countByRoleIgnoreCase(role.getCode()); + if (holders > 0) { + throw new IllegalArgumentException( + "Cannot delete \"" + role.getName() + "\": " + holders + " user(s) still have this role. " + + "Reassign them first."); + } + // Overrides are keyed by role code with no FK, so they would dangle silently. + overrideRepository.deleteAll(overrideRepository.findByRoleIgnoreCase(role.getCode())); + customRoleRepository.delete(role); + log.info("Custom role deleted: code={}, by={}", role.getCode(), actor); + } + + /** How many users hold this role code (built-in or custom). */ + public long countUsersWithRole(String roleCode) { + return userRepository.countByRoleIgnoreCase(normalizeCode(roleCode)); + } + + private static Set resolvePermissions(Collection codes) { + Set resolved = EnumSet.noneOf(Permission.class); + if (codes == null) { + return resolved; + } + for (String raw : codes) { + Permission permission = Permission.fromCode(raw); + if (permission == null) { + throw new IllegalArgumentException("Unknown permission: " + raw); + } + resolved.add(permission); + } + return resolved; + } + + private static String requireName(String name) { + String clean = name == null ? "" : name.trim(); + if (clean.isEmpty()) { + throw new IllegalArgumentException("Role name is required"); + } + if (clean.length() > 128) { + throw new IllegalArgumentException("Role name must be 128 characters or fewer"); + } + return clean; + } + + /** "Data Analyst" -> "DATA_ANALYST". */ + private static String deriveCode(String name) { + String code = name.trim().toUpperCase(Locale.ROOT).replaceAll("[^A-Z0-9]+", "_") + .replaceAll("^_+|_+$", ""); + if (code.isEmpty()) { + throw new IllegalArgumentException("Role name must contain at least one letter or number"); + } + return code.length() > 64 ? code.substring(0, 64) : code; + } + + private static String normalizeCode(String code) { + return code == null ? "" : code.trim().toUpperCase(Locale.ROOT); + } + + private static String trimToNull(String value) { + if (value == null) return null; + String trimmed = value.trim(); + return trimmed.isEmpty() ? null : trimmed; + } +} diff --git a/backend/src/main/java/com/dbaagent/service/PasswordlessAuthService.java b/backend/src/main/java/com/dbaagent/service/PasswordlessAuthService.java index 9a836b3..c3c1317 100644 --- a/backend/src/main/java/com/dbaagent/service/PasswordlessAuthService.java +++ b/backend/src/main/java/com/dbaagent/service/PasswordlessAuthService.java @@ -292,12 +292,12 @@ public Optional refresh(String rawRefr return authSessionService.findValidSessionByRefreshToken(tokenHash) .flatMap(session -> userRepository.findById(session.getUserId()) .map(user -> { - Role role = user.getRoleEnum(); + String roleCode = user.getRoleCode(); return authSessionService.refreshSession( rawRefreshToken, user, - role, - permissionService.getEffectivePermissions(role), + roleCode, + permissionService.getEffectivePermissions(roleCode), clientIp, userAgent ); @@ -531,11 +531,12 @@ private AuthFlowResult issueSession( String requestId, boolean mfaVerified ) { + String roleCode = user.getRoleCode(); Role role = user.getRoleEnum(); - Set permissions = permissionService.getEffectivePermissions(role); + Set permissions = permissionService.getEffectivePermissions(roleCode); AuthSessionService.SessionAuthentication session = authSessionService.createSession( user, - role, + roleCode, permissions, clientIp, userAgent, @@ -547,7 +548,7 @@ private AuthFlowResult issueSession( user.setLastLoginAt(LocalDateTime.now()); user.setLastLoginIp(clientIp); userRepository.save(user); - return AuthFlowResult.authenticated(user, role, permissions, session); + return AuthFlowResult.authenticated(user, role, roleCode, permissions, session); } private void markFailedOtp(AuthLoginChallenge challenge, User user, String clientIp, String userAgent, String requestId) { @@ -746,7 +747,13 @@ public record AuthFlowResult( boolean mfaRequired, boolean mfaSetupRequired, User user, + /** + * The built-in role, or null when the user holds a custom role. Prefer + * {@link #roleCode()} — a null here does NOT mean "not authenticated". + */ Role role, + /** The user's role code, built-in or custom. Non-null on a successful auth. */ + String roleCode, Set permissions, AuthSessionService.SessionAuthentication sessionAuthentication ) { @@ -777,11 +784,23 @@ static AuthFlowResult authenticated( Role role, Set permissions, AuthSessionService.SessionAuthentication sessionAuthentication + ) { + return authenticated(user, role, user != null ? user.getRoleCode() : null, + permissions, sessionAuthentication); + } + + static AuthFlowResult authenticated( + User user, + Role role, + String roleCode, + Set permissions, + AuthSessionService.SessionAuthentication sessionAuthentication ) { return AuthFlowResult.builder() .success(true) .user(user) .role(role) + .roleCode(roleCode) .permissions(permissions) .sessionAuthentication(sessionAuthentication) .build(); diff --git a/backend/src/main/java/com/dbaagent/service/PermissionService.java b/backend/src/main/java/com/dbaagent/service/PermissionService.java index e8b9019..38bcd19 100644 --- a/backend/src/main/java/com/dbaagent/service/PermissionService.java +++ b/backend/src/main/java/com/dbaagent/service/PermissionService.java @@ -1,8 +1,10 @@ package com.dbaagent.service; +import com.dbaagent.model.CustomRole; import com.dbaagent.model.Permission; import com.dbaagent.model.Role; import com.dbaagent.model.RolePermissionOverride; +import com.dbaagent.repository.CustomRoleRepository; import com.dbaagent.repository.RolePermissionOverrideRepository; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -13,11 +15,18 @@ import java.util.stream.Collectors; /** - * Service for computing effective permissions based on: - * 1. Default permissions from Permission.defaultMinRole (hierarchy-based) - * 2. Custom overrides from RolePermissionOverride table + * Resolves the effective permissions of a role code. * - * Effective permission = (default permissions) + (granted overrides) - (revoked overrides) + *

A role code is either a built-in {@link Role} name or a {@link CustomRole} code. + * The effective set is: + * + * 

+ *   built-in : Role default permissions + granted overrides - revoked overrides
+ *   custom   : the role's own explicit permission set + granted - revoked
+ *
+ * + *

ADMIN is a fixed point: it holds every permission and overrides are ignored for it, + * so no configuration change can lock the last administrator out of user management. */ @Service public class PermissionService { @@ -25,167 +34,233 @@ public class PermissionService { private static final Logger log = LoggerFactory.getLogger(PermissionService.class); private final RolePermissionOverrideRepository overrideRepository; + private final CustomRoleRepository customRoleRepository; - public PermissionService(RolePermissionOverrideRepository overrideRepository) { + public PermissionService(RolePermissionOverrideRepository overrideRepository, + CustomRoleRepository customRoleRepository) { this.overrideRepository = overrideRepository; + this.customRoleRepository = customRoleRepository; } - /** - * Get the effective permissions for a role. - * Combines default hierarchy-based permissions with any overrides. - */ - public Set getEffectivePermissions(Role role) { - // Start with default permissions based on hierarchy - Set effective = new HashSet<>(role.getDefaultPermissions()); + /** Effective permissions for a role code (built-in name or custom role code). */ + public Set getEffectivePermissions(String roleCode) { + String normalized = normalize(roleCode); + if (normalized == null) { + return EnumSet.noneOf(Permission.class); + } + + Role builtIn = Role.fromString(normalized); + if (builtIn == Role.ADMIN) { + return EnumSet.allOf(Permission.class); + } - // Apply overrides - List overrides = overrideRepository.findByRole(role); - for (RolePermissionOverride override : overrides) { + Set effective = EnumSet.noneOf(Permission.class); + if (builtIn != null) { + effective.addAll(builtIn.getDefaultPermissions()); + } else { + Optional custom = customRoleRepository.findByCodeIgnoreCase(normalized); + if (custom.isEmpty()) { + // An unknown role code grants nothing. Falling back to a real role here + // would turn a typo, or a custom role someone deleted, into silent access. + log.warn("Unknown role code '{}' resolved to no permissions", roleCode); + return effective; + } + effective.addAll(custom.get().getPermissions()); + } + + for (RolePermissionOverride override : overrideRepository.findByRoleIgnoreCase(normalized)) { if (override.isGranted()) { - // Add permission (even if not in defaults) effective.add(override.getPermissionCode()); } else { - // Remove permission (even if in defaults) effective.remove(override.getPermissionCode()); } } - return effective; } - /** - * Get effective permissions as a Set of permission code strings. - * This is what gets returned to the frontend. - */ - public Set getEffectivePermissionCodes(Role role) { - return getEffectivePermissions(role).stream() + /** Convenience overload for a built-in role. */ + public Set getEffectivePermissions(Role role) { + return role == null ? EnumSet.noneOf(Permission.class) : getEffectivePermissions(role.name()); + } + + public Set getEffectivePermissionCodes(String roleCode) { + return getEffectivePermissions(roleCode).stream() .map(Enum::name) - .collect(Collectors.toSet()); + .collect(Collectors.toCollection(TreeSet::new)); + } + + public Set getEffectivePermissionCodes(Role role) { + return getEffectivePermissionCodes(role == null ? null : role.name()); + } + + public boolean hasPermission(String roleCode, Permission permission) { + return permission != null && getEffectivePermissions(roleCode).contains(permission); } - /** - * Check if a role has a specific permission (considering overrides). - */ public boolean hasPermission(Role role, Permission permission) { - // Check for override first - Optional override = - overrideRepository.findByRoleAndPermissionCode(role, permission); + return hasPermission(role == null ? null : role.name(), permission); + } - if (override.isPresent()) { - return override.get().isGranted(); + /** True when the role code names a built-in role or an existing custom role. */ + public boolean roleExists(String roleCode) { + String normalized = normalize(roleCode); + if (normalized == null) { + return false; } + return Role.isBuiltIn(normalized) || customRoleRepository.existsByCodeIgnoreCase(normalized); + } - // Fall back to default - return permission.isGrantedByDefaultTo(role); + /** Display name for a role code, for UI and audit messages. */ + public String describeRole(String roleCode) { + String normalized = normalize(roleCode); + if (normalized == null) { + return "Unknown"; + } + Role builtIn = Role.fromString(normalized); + if (builtIn != null) { + return builtIn.getDisplayName(); + } + return customRoleRepository.findByCodeIgnoreCase(normalized) + .map(CustomRole::getName) + .orElse(normalized); } /** - * Get the full permission registry with role assignments. - * Returns all permissions with their default min role and any overrides. + * The full permission registry: every permission, which roles hold it, and any + * overrides recorded against it. */ public List getPermissionRegistry() { - List allOverrides = overrideRepository.findAll(); - Map> overridesByPermission = allOverrides.stream() - .collect(Collectors.groupingBy(RolePermissionOverride::getPermissionCode)); + List allRoleCodes = getAllRoleCodes(); + Map> overridesByPermission = + overrideRepository.findAll().stream() + .collect(Collectors.groupingBy(RolePermissionOverride::getPermissionCode)); + + Map> effectiveByRole = allRoleCodes.stream() + .collect(Collectors.toMap(code -> code, this::getEffectivePermissions)); return Arrays.stream(Permission.values()) .map(p -> { PermissionInfo info = new PermissionInfo(); info.code = p.name(); info.description = p.getDescription(); - info.defaultMinRole = p.getDefaultMinRole().name(); - - // Compute effective roles for this permission - Set effectiveRoles = new HashSet<>(); - for (Role role : Role.values()) { - if (hasPermission(role, p)) { - effectiveRoles.add(role.name()); - } - } - info.effectiveRoles = effectiveRoles; - - // Include overrides for this permission - info.overrides = overridesByPermission.getOrDefault(p, Collections.emptyList()) - .stream() + info.defaultRoles = p.getDefaultRoles().stream().map(Enum::name) + .collect(Collectors.toCollection(TreeSet::new)); + info.effectiveRoles = effectiveByRole.entrySet().stream() + .filter(entry -> entry.getValue().contains(p)) + .map(Map.Entry::getKey) + .collect(Collectors.toCollection(TreeSet::new)); + info.overrides = overridesByPermission.getOrDefault(p, List.of()).stream() .map(o -> { OverrideInfo oi = new OverrideInfo(); - oi.role = o.getRole().name(); + oi.role = o.getRole(); oi.granted = o.isGranted(); oi.reason = o.getReason(); return oi; }) .collect(Collectors.toList()); - return info; }) .collect(Collectors.toList()); } - /** - * Get all overrides (for admin view). - */ public List getAllOverrides() { return overrideRepository.findAll(); } - /** - * Create or update an override. - */ @Transactional - public RolePermissionOverride setOverride(Role role, Permission permission, boolean granted, String reason, String updatedBy) { - Optional existing = - overrideRepository.findByRoleAndPermissionCode(role, permission); - - RolePermissionOverride override; - if (existing.isPresent()) { - override = existing.get(); - override.setGranted(granted); - override.setReason(reason); - override.setUpdatedBy(updatedBy); - } else { - override = new RolePermissionOverride(role, permission, granted, reason, updatedBy); + public RolePermissionOverride setOverride(String roleCode, Permission permission, boolean granted, + String reason, String updatedBy) { + String normalized = normalize(roleCode); + if (normalized == null || !roleExists(normalized)) { + throw new IllegalArgumentException("Unknown role: " + roleCode); + } + if (Role.ADMIN.name().equals(normalized)) { + // Admin holds every permission unconditionally; storing an override here + // would record a rule getEffectivePermissions deliberately ignores. + throw new IllegalArgumentException("The Admin role always holds every permission and cannot be overridden"); } + RolePermissionOverride override = overrideRepository + .findByRoleIgnoreCaseAndPermissionCode(normalized, permission) + .orElseGet(() -> new RolePermissionOverride(normalized, permission, granted, reason, updatedBy)); + override.setGranted(granted); + override.setReason(reason); + override.setUpdatedBy(updatedBy); + RolePermissionOverride saved = overrideRepository.save(override); log.info("Permission override set: role={}, permission={}, granted={}, by={}", - role, permission, granted, updatedBy); + normalized, permission, granted, updatedBy); return saved; } - /** - * Remove an override (revert to default behavior). - */ @Transactional - public void removeOverride(Role role, Permission permission) { - overrideRepository.deleteByRoleAndPermissionCode(role, permission); - log.info("Permission override removed: role={}, permission={}", role, permission); + public void removeOverride(String roleCode, Permission permission) { + String normalized = normalize(roleCode); + if (normalized == null) { + return; + } + overrideRepository.deleteByRoleIgnoreCaseAndPermissionCode(normalized, permission); + log.info("Permission override removed: role={}, permission={}", normalized, permission); } - /** - * Get role info with effective permissions. - */ + /** Every role code in the system: built-ins first, then custom roles by name. */ + public List getAllRoleCodes() { + List codes = Arrays.stream(Role.values()).map(Enum::name) + .collect(Collectors.toCollection(ArrayList::new)); + customRoleRepository.findAllByOrderByNameAsc().stream() + .map(CustomRole::getCode) + .filter(code -> !Role.isBuiltIn(code)) + .forEach(codes::add); + return codes; + } + + /** Role registry for the admin UI: built-in and custom roles with their permissions. */ public List getRoleRegistry() { - return Arrays.stream(Role.values()) - .map(role -> { - RoleInfo info = new RoleInfo(); - info.code = role.name(); - info.description = role.getDescription(); - info.level = role.ordinal(); - info.defaultPermissions = role.getDefaultPermissions().stream() - .map(Enum::name) - .collect(Collectors.toSet()); - info.effectivePermissions = getEffectivePermissionCodes(role); - info.overrideCount = overrideRepository.findByRole(role).size(); - return info; - }) - .collect(Collectors.toList()); + List roles = new ArrayList<>(); + + for (Role role : Role.values()) { + RoleInfo info = new RoleInfo(); + info.code = role.name(); + info.name = role.getDisplayName(); + info.description = role.getDescription(); + info.builtIn = true; + info.editable = role != Role.ADMIN; + info.defaultPermissions = role.getDefaultPermissions().stream().map(Enum::name) + .collect(Collectors.toCollection(TreeSet::new)); + info.effectivePermissions = getEffectivePermissionCodes(role.name()); + info.overrideCount = role == Role.ADMIN ? 0 : overrideRepository.findByRoleIgnoreCase(role.name()).size(); + roles.add(info); + } + + for (CustomRole custom : customRoleRepository.findAllByOrderByNameAsc()) { + RoleInfo info = new RoleInfo(); + info.code = custom.getCode(); + info.name = custom.getName(); + info.description = custom.getDescription(); + info.builtIn = false; + info.editable = true; + info.defaultPermissions = custom.getPermissionCodeSet().stream() + .collect(Collectors.toCollection(TreeSet::new)); + info.effectivePermissions = getEffectivePermissionCodes(custom.getCode()); + info.overrideCount = overrideRepository.findByRoleIgnoreCase(custom.getCode()).size(); + roles.add(info); + } + + return roles; + } + + private static String normalize(String roleCode) { + if (roleCode == null || roleCode.isBlank()) { + return null; + } + return roleCode.trim().toUpperCase(); } // DTO classes for API responses public static class PermissionInfo { public String code; public String description; - public String defaultMinRole; + public Set defaultRoles; public Set effectiveRoles; public List overrides; } @@ -198,10 +273,14 @@ public static class OverrideInfo { public static class RoleInfo { public String code; + public String name; public String description; - public int level; + public boolean builtIn; + public boolean editable; public Set defaultPermissions; public Set effectivePermissions; public int overrideCount; + /** Filled in by the controller; how many users currently hold this role. */ + public long userCount; } } diff --git a/backend/src/main/java/com/dbaagent/service/SlackConversationStateService.java b/backend/src/main/java/com/dbaagent/service/SlackConversationStateService.java index a2152cf..9b92726 100644 --- a/backend/src/main/java/com/dbaagent/service/SlackConversationStateService.java +++ b/backend/src/main/java/com/dbaagent/service/SlackConversationStateService.java @@ -261,7 +261,9 @@ public StatusSnapshot buildStatusSnapshot() { serviceUsername, serviceUser.isPresent(), serviceUser.map(User::getUsername).orElse(null), - serviceUser.map(user -> user.getRoleEnum().name()).orElse(null), + // getRoleCode, not getRoleEnum().name(): the enum is null for a custom role + // and this status snapshot would NPE on it. + serviceUser.map(User::getRoleCode).orElse(null), serviceUser.map(User::isAdmin).orElse(false), allowedConnections.size(), Math.toIntExact(channelBindingRepository.count()), diff --git a/backend/src/main/java/com/dbaagent/service/UserManagementService.java b/backend/src/main/java/com/dbaagent/service/UserManagementService.java index a6394ca..5613486 100644 --- a/backend/src/main/java/com/dbaagent/service/UserManagementService.java +++ b/backend/src/main/java/com/dbaagent/service/UserManagementService.java @@ -47,6 +47,9 @@ public class UserManagementService { @Autowired private UserSessionRepository userSessionRepository; + @Autowired + private PermissionService permissionService; + /** * Create a new user (admin only). */ @@ -61,10 +64,7 @@ public Map createUser(String username, String password, String e if (password.length() < 8) { throw new IllegalArgumentException("Password must be at least 8 characters"); } - Role role = Role.DEVELOPER; - if (roleName != null && !roleName.isBlank()) { - role = Role.fromString(roleName); - } + String roleCode = resolveRoleCode(roleName); String normalizedEmail = email.trim().toLowerCase(); if (userRepository.existsByEmailIgnoreCase(normalizedEmail)) { throw new IllegalArgumentException("A user with this email already exists"); @@ -79,7 +79,7 @@ public Map createUser(String username, String password, String e user.setUsername(resolvedUsername); user.setEmail(normalizedEmail); user.setPassword(passwordEncoder.encode(password)); - user.setRoleEnum(role); + user.setRole(roleCode); user.setEmailVerifiedAt(LocalDateTime.now()); user.setAccountStatusEnum(UserAccountStatus.ACTIVE); user.setInvitedAt(LocalDateTime.now()); @@ -91,10 +91,10 @@ public Map createUser(String username, String password, String e .userId(user.getId()) .email(user.getEmail()) .targetResource("user:" + user.getId()) - .metadata(Map.of("role", role.name(), "action", "user_created")) + .metadata(Map.of("role", roleCode, "action", "user_created")) .build()); - log.info("Admin created user: {} with role {}", normalizedEmail, role.name()); + log.info("Admin created user: {} with role {}", normalizedEmail, roleCode); return toUserDTO(user); } @@ -136,8 +136,8 @@ public Map updateUserRole(Long userId, String roleName) { throw new IllegalArgumentException("Cannot change the admin user's role"); } - Role role = Role.fromString(roleName); - user.setRole(role.name()); + String roleCode = resolveRoleCode(roleName); + user.setRole(roleCode); userRepository.save(user); securityEventService.log(SecurityEventService.EventRequest.builder() .eventType(SecurityEventType.ROLE_CHANGED) @@ -145,10 +145,10 @@ public Map updateUserRole(Long userId, String roleName) { .userId(user.getId()) .email(user.getEmail()) .targetResource("user:" + user.getId()) - .metadata(Map.of("role", role.name())) + .metadata(Map.of("role", roleCode)) .build()); - log.info("Updated user {} role to {}", user.getUsername(), role.name()); + log.info("Updated user {} role to {}", user.getUsername(), roleCode); return toUserDTO(user); } @@ -207,7 +207,9 @@ public Map resendInvite(Long userId) { if (user.getAccountStatusEnum() == UserAccountStatus.ACTIVE) { throw new IllegalArgumentException("Direct account creation is enabled. Active users do not need invite emails."); } - Role role = user.getRoleEnum(); + // UserInviteService is typed to the built-in enum; a custom-role invite carries + // the closest built-in and the real role is already stored on the user row. + Role role = Role.fromStringOrDefault(user.getRole()); var invite = userInviteService.createInvite(user.getEmail(), user.getUsername(), role, "admin", InviteType.STANDARD, null, null); return toUserDTO(invite.user()); } @@ -231,17 +233,24 @@ public Map disableUser(Long userId) { * Get all roles with their permissions. */ public List> getRolesWithPermissions() { - return Arrays.stream(Role.values()) - .map(role -> { + // Every assignable role, built-in and custom, with the permissions actually in + // effect (overrides applied) rather than the enum's raw defaults — this list is + // what the admin UI offers when changing someone's role. + return permissionService.getRoleRegistry().stream() + .map(info -> { Map roleDTO = new HashMap<>(); - roleDTO.put("name", role.name()); - roleDTO.put("description", role.getDescription()); - - List> permissions = role.getPermissions().stream() - .map(p -> { + roleDTO.put("name", info.code); + roleDTO.put("code", info.code); + roleDTO.put("displayName", info.name); + roleDTO.put("description", info.description); + roleDTO.put("builtIn", info.builtIn); + + List> permissions = info.effectivePermissions.stream() + .map(code -> { + Permission p = Permission.fromCode(code); Map permDTO = new HashMap<>(); - permDTO.put("name", p.name()); - permDTO.put("description", p.getDescription()); + permDTO.put("name", code); + permDTO.put("description", p != null ? p.getDescription() : code); return permDTO; }) .collect(Collectors.toList()); @@ -252,6 +261,28 @@ public List> getRolesWithPermissions() { .collect(Collectors.toList()); } + /** + * Normalise a requested role name to a role code that actually exists. + * + *

Blank means DEVELOPER. Anything else must name a built-in role or an existing + * custom role: an unknown code is rejected rather than silently downgraded, because a + * user whose role does not resolve gets no permissions at all at their next login. + */ + private String resolveRoleCode(String roleName) { + if (roleName == null || roleName.isBlank()) { + return Role.DEVELOPER.name(); + } + String code = roleName.trim().toUpperCase(); + Role builtIn = Role.fromString(code); + if (builtIn != null) { + return builtIn.name(); + } + if (!permissionService.roleExists(code)) { + throw new IllegalArgumentException("Unknown role: " + roleName); + } + return code; + } + /** * Get all available permissions. */ @@ -283,11 +314,11 @@ private Map toUserDTO(User user) { dto.put("mfaEnrolledAt", user.getMfaEnrolledAt()); dto.put("mfaEnrolled", user.getMfaEnrolledAt() != null); - // Include permissions for convenience - Set permissions = user.getPermissions(); - dto.put("permissions", permissions.stream() - .map(Permission::name) - .collect(Collectors.toList())); + // Effective permissions, resolved by role code so a custom role reports what it + // actually grants; user.getPermissions() only knows the built-in enum. + dto.put("roleName", permissionService.describeRole(user.getRoleCode())); + dto.put("permissions", new java.util.ArrayList<>( + permissionService.getEffectivePermissionCodes(user.getRoleCode()))); return dto; } diff --git a/backend/src/test/java/com/dbaagent/service/AuthFlowResultCustomRoleTest.java b/backend/src/test/java/com/dbaagent/service/AuthFlowResultCustomRoleTest.java new file mode 100644 index 0000000..480887e --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/AuthFlowResultCustomRoleTest.java @@ -0,0 +1,69 @@ +package com.dbaagent.service; + +import com.dbaagent.model.Permission; +import com.dbaagent.model.Role; +import com.dbaagent.model.User; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Regression: a successful login by a user holding a custom role must be + * recognisable as successful. + * + *

{@code AuthFlowResult.role()} is null for a custom role, because {@link Role} can + * only represent built-ins. {@code AuthController.authResponse} gated its success branch + * on that field, so a custom-role login fell through to the "challenge required" branch + * and threw NullPointerException inside {@code Map.of} on a null challengeId — HTTP 500 + * on every custom-role sign-in. Verified against a running install before the fix. + * + *

The controller now gates on {@code roleCode()}, which this pins. + */ +class AuthFlowResultCustomRoleTest { + + private static User userWithRole(String role) { + User user = new User(); + user.setUsername("analyst"); + user.setEmail("analyst@localhost"); + user.setRole(role); + return user; + } + + @Test + @DisplayName("A custom-role login carries a roleCode even though role is null") + void customRoleStillYieldsARoleCode() { + User user = userWithRole("ANALYST"); + + PasswordlessAuthService.AuthFlowResult result = PasswordlessAuthService.AuthFlowResult + .authenticated(user, Role.fromString("ANALYST"), Set.of(Permission.VIEW_DASHBOARDS), null); + + assertThat(result.success()).isTrue(); + assertThat(result.role()).isNull(); // the trap + assertThat(result.roleCode()).isEqualTo("ANALYST"); + // The controller's success branch keys on this; null here was the 500. + assertThat(result.roleCode()).isNotNull(); + } + + @Test + @DisplayName("A built-in role login still carries both role and roleCode") + void builtInRoleUnchanged() { + User user = userWithRole("DBA"); + + PasswordlessAuthService.AuthFlowResult result = PasswordlessAuthService.AuthFlowResult + .authenticated(user, Role.fromString("DBA"), Set.of(Permission.VIEW_BRAIN), null); + + assertThat(result.role()).isEqualTo(Role.DBA); + assertThat(result.roleCode()).isEqualTo("DBA"); + } + + @Test + @DisplayName("User.getRoleCode never returns null, so the success gate cannot be tripped by a blank role") + void roleCodeNeverNull() { + assertThat(userWithRole(null).getRoleCode()).isEqualTo("DEVELOPER"); + assertThat(userWithRole("").getRoleCode()).isEqualTo("DEVELOPER"); + assertThat(userWithRole(" analyst ").getRoleCode()).isEqualTo("ANALYST"); + } +} diff --git a/backend/src/test/java/com/dbaagent/service/PermissionResolutionTest.java b/backend/src/test/java/com/dbaagent/service/PermissionResolutionTest.java new file mode 100644 index 0000000..a87f280 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/PermissionResolutionTest.java @@ -0,0 +1,165 @@ +package com.dbaagent.service; + +import com.dbaagent.model.CustomRole; +import com.dbaagent.model.Permission; +import com.dbaagent.model.Role; +import com.dbaagent.model.RolePermissionOverride; +import com.dbaagent.repository.CustomRoleRepository; +import com.dbaagent.repository.RolePermissionOverrideRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.Optional; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Permission resolution across built-in and custom roles. + * + *

These assert the *product* rules the roles were specified with — which menus each + * role can open — rather than restating the enum. A regression here is a user seeing a + * section they should not, so each role's section set is pinned explicitly. + */ +class PermissionResolutionTest { + + private RolePermissionOverrideRepository overrideRepository; + private CustomRoleRepository customRoleRepository; + private PermissionService permissionService; + + @BeforeEach + void setUp() { + overrideRepository = mock(RolePermissionOverrideRepository.class); + customRoleRepository = mock(CustomRoleRepository.class); + when(overrideRepository.findByRoleIgnoreCase(anyString())).thenReturn(List.of()); + when(customRoleRepository.findByCodeIgnoreCase(anyString())).thenReturn(Optional.empty()); + when(customRoleRepository.findAllByOrderByNameAsc()).thenReturn(List.of()); + permissionService = new PermissionService(overrideRepository, customRoleRepository); + } + + @Test + @DisplayName("DBA sees every section and can manage connections, but not users") + void dbaHasEverythingExceptUserManagement() { + Set permissions = permissionService.getEffectivePermissions(Role.DBA.name()); + + assertThat(permissions).contains( + Permission.VIEW_AGENT, Permission.VIEW_DASHBOARDS, Permission.VIEW_DIGEST, + Permission.VIEW_BRAIN, Permission.VIEW_PERFORMANCE, Permission.VIEW_EDITOR, + Permission.MANAGE_CONNECTIONS, Permission.MANAGE_SETTINGS); + + assertThat(permissions).doesNotContain( + Permission.MANAGE_USERS, Permission.MANAGE_INVITE_CODES, Permission.MANAGE_PERMISSIONS); + } + + @Test + @DisplayName("Data Engineer gets Agent, Dashboards and Editor only") + void dataEngineerSections() { + Set permissions = permissionService.getEffectivePermissions(Role.DATA_ENGINEER.name()); + + assertThat(permissions).contains( + Permission.VIEW_AGENT, Permission.VIEW_DASHBOARDS, Permission.VIEW_EDITOR); + assertThat(permissions).doesNotContain( + Permission.VIEW_DIGEST, Permission.VIEW_PERFORMANCE, Permission.VIEW_BRAIN, + Permission.MANAGE_CONNECTIONS, Permission.MANAGE_USERS); + } + + @Test + @DisplayName("Developer gets Agent, Digest, Dashboards, Performance and Editor, but no connection settings") + void developerSections() { + Set permissions = permissionService.getEffectivePermissions(Role.DEVELOPER.name()); + + assertThat(permissions).contains( + Permission.VIEW_AGENT, Permission.VIEW_DIGEST, Permission.VIEW_DASHBOARDS, + Permission.VIEW_PERFORMANCE, Permission.VIEW_EDITOR); + assertThat(permissions).doesNotContain( + Permission.MANAGE_CONNECTIONS, Permission.MANAGE_SETTINGS, Permission.MANAGE_USERS); + } + + @Test + @DisplayName("Roles do not nest: each of Data Engineer and Developer has something the other lacks") + void rolesAreNotAHierarchy() { + Set dataEngineer = permissionService.getEffectivePermissions(Role.DATA_ENGINEER.name()); + Set developer = permissionService.getEffectivePermissions(Role.DEVELOPER.name()); + + // Developer has Digest and Performance that Data Engineer lacks... + assertThat(developer).contains(Permission.VIEW_DIGEST, Permission.VIEW_PERFORMANCE); + assertThat(dataEngineer).doesNotContain(Permission.VIEW_DIGEST, Permission.VIEW_PERFORMANCE); + // ...so neither is a superset of the other, which is why ordinal comparison had + // to go. If this ever passes trivially, the role definitions drifted. + assertThat(developer).isNotEqualTo(dataEngineer); + } + + @Test + @DisplayName("Admin holds every permission and ignores overrides") + void adminIsAFixedPoint() { + when(overrideRepository.findByRoleIgnoreCase("ADMIN")).thenReturn(List.of( + new RolePermissionOverride("ADMIN", Permission.MANAGE_USERS, false, "tester"))); + + Set permissions = permissionService.getEffectivePermissions("ADMIN"); + + assertThat(permissions).containsExactlyInAnyOrder(Permission.values()); + assertThat(permissions).contains(Permission.MANAGE_USERS); + } + + @Test + @DisplayName("A custom role grants exactly its ticked permissions") + void customRoleGrantsItsOwnSet() { + CustomRole analyst = new CustomRole(); + analyst.setCode("ANALYST"); + analyst.setName("Analyst"); + analyst.setPermissions(Set.of(Permission.VIEW_DASHBOARDS, Permission.USE_CHAT)); + when(customRoleRepository.findByCodeIgnoreCase("ANALYST")).thenReturn(Optional.of(analyst)); + + Set permissions = permissionService.getEffectivePermissions("ANALYST"); + + assertThat(permissions).containsExactlyInAnyOrder(Permission.VIEW_DASHBOARDS, Permission.USE_CHAT); + assertThat(permissions).doesNotContain(Permission.VIEW_EDITOR, Permission.MANAGE_USERS); + } + + @Test + @DisplayName("An unknown role code grants nothing rather than falling back to Developer") + void unknownRoleGrantsNothing() { + // The old Role.fromString collapsed anything unrecognised to DEVELOPER. Keeping + // that behaviour here would turn a deleted custom role into silent Developer + // access for everyone who held it. + assertThat(permissionService.getEffectivePermissions("NO_SUCH_ROLE")).isEmpty(); + } + + @Test + @DisplayName("Overrides add and remove permissions for a non-admin role") + void overridesApply() { + when(overrideRepository.findByRoleIgnoreCase(Role.DATA_ENGINEER.name())).thenReturn(List.of( + new RolePermissionOverride(Role.DATA_ENGINEER.name(), Permission.VIEW_DIGEST, true, "tester"), + new RolePermissionOverride(Role.DATA_ENGINEER.name(), Permission.VIEW_EDITOR, false, "tester"))); + + Set permissions = permissionService.getEffectivePermissions(Role.DATA_ENGINEER.name()); + + assertThat(permissions).contains(Permission.VIEW_DIGEST); + assertThat(permissions).doesNotContain(Permission.VIEW_EDITOR); + } + + @Test + @DisplayName("An override cannot be recorded against Admin") + void adminOverrideRejected() { + when(customRoleRepository.existsByCodeIgnoreCase(anyString())).thenReturn(false); + org.junit.jupiter.api.Assertions.assertThrows(IllegalArgumentException.class, + () -> permissionService.setOverride("ADMIN", Permission.MANAGE_USERS, false, null, "tester")); + } + + @Test + @DisplayName("Legacy role names still resolve to Developer") + void legacyRoleNamesMigrate() { + // Installs upgrading from the two-role model carry EDITOR/VIEWER/USER values. + assertThat(Role.fromString("EDITOR")).isEqualTo(Role.DEVELOPER); + assertThat(Role.fromString("VIEWER")).isEqualTo(Role.DEVELOPER); + assertThat(Role.fromString("developer")).isEqualTo(Role.DEVELOPER); + assertThat(Role.fromString("DBA")).isEqualTo(Role.DBA); + assertThat(Role.fromString("ANALYST")).isNull(); + } +} diff --git a/src/components/tabs/admin/RoleManager.jsx b/src/components/tabs/admin/RoleManager.jsx new file mode 100644 index 0000000..29a42a6 --- /dev/null +++ b/src/components/tabs/admin/RoleManager.jsx @@ -0,0 +1,305 @@ +'use client' + +import { useCallback, useMemo, useState } from 'react' +import { Plus, Trash2, RefreshCw, AlertCircle, Pencil, X, ShieldCheck } from 'lucide-react' +import { permissionsAPI } from '@/lib/api/client' +import { roleLabel } from '@/lib/permissions' + +/** + * Permissions grouped for the role editor, so an admin ticks "which menus" rather than + * reading a flat list of 25 codes. Any permission not listed here still exists on the + * backend — it simply isn't offered as a checkbox. + */ +const PERMISSION_GROUPS = [ + { + label: 'Sections', + items: [ + ['VIEW_AGENT', 'Agent'], + ['VIEW_DASHBOARDS', 'Dashboards'], + ['VIEW_DIGEST', 'Digest'], + ['VIEW_BRAIN', 'Brain'], + ['VIEW_PERFORMANCE', 'Performance'], + ['VIEW_EDITOR', 'Editor'], + ], + }, + { + label: 'Working with data', + items: [ + ['EXECUTE_QUERIES', 'Run SQL queries'], + ['USE_CHAT', 'Use the AI assistant'], + ['EXPORT_DATA', 'Export results'], + ['MANAGE_DASHBOARD_WORKSPACES', 'Create dashboard workspaces'], + ['VIEW_SLOW_QUERIES', 'View slow queries'], + ['VIEW_SCHEMA', 'Browse schema'], + ], + }, + { + label: 'Administration', + items: [ + ['MANAGE_CONNECTIONS', 'Connection settings'], + ['MANAGE_SETTINGS', 'System settings'], + ['RUN_ANALYSIS', 'Run analysis tasks'], + ['MANAGE_ALERTS', 'Manage alerts'], + ['MANAGE_USERS', 'Manage users'], + ['MANAGE_PERMISSIONS', 'Manage roles'], + ], + }, +] + +const EMPTY_DRAFT = { code: null, name: '', description: '', permissions: [] } + +/** + * Create and edit custom roles. + * + *

Built-in roles are shown read-only: their permission sets are code, so changing one + * is what the override endpoint is for, and editing them here would not survive an + * upgrade. Deleting a custom role is refused by the backend while any user still holds it. + */ +export default function RoleManager({ roles = [], onChanged }) { + const [draft, setDraft] = useState(null) + const [saving, setSaving] = useState(false) + const [deletingCode, setDeletingCode] = useState(null) + const [confirmCode, setConfirmCode] = useState(null) + const [error, setError] = useState(null) + + const customRoles = useMemo( + () => roles.filter((role) => role.builtIn === false), + [roles], + ) + + const togglePermission = useCallback((code) => { + setDraft((current) => { + if (!current) return current + const has = current.permissions.includes(code) + return { + ...current, + permissions: has + ? current.permissions.filter((p) => p !== code) + : [...current.permissions, code], + } + }) + }, []) + + const startCreate = useCallback(() => { + setError(null) + setDraft({ ...EMPTY_DRAFT }) + }, []) + + const startEdit = useCallback((role) => { + setError(null) + setDraft({ + code: role.name, + name: role.displayName || roleLabel(role.name), + description: role.description || '', + permissions: (role.permissions || []).map((p) => (typeof p === 'string' ? p : p?.name)).filter(Boolean), + }) + }, []) + + const save = useCallback(async () => { + if (!draft || saving) return + const name = draft.name.trim() + if (!name) { + setError('Give the role a name.') + return + } + setSaving(true) + setError(null) + try { + if (draft.code) { + await permissionsAPI.updateRole(draft.code, { + name, + description: draft.description, + permissions: draft.permissions, + }) + } else { + await permissionsAPI.createRole({ + name, + description: draft.description, + permissions: draft.permissions, + }) + } + setDraft(null) + onChanged?.() + } catch (e) { + setError(e?.response?.data?.message || 'Could not save the role.') + } finally { + setSaving(false) + } + }, [draft, saving, onChanged]) + + const remove = useCallback(async (role) => { + setDeletingCode(role.name) + setError(null) + try { + await permissionsAPI.deleteRole(role.name) + setConfirmCode(null) + onChanged?.() + } catch (e) { + setError(e?.response?.data?.message || 'Could not delete the role.') + } finally { + setDeletingCode(null) + } + }, [onChanged]) + + return ( +

+
+
+ +

Custom roles

+
+ {!draft && ( + + )} +
+ + {error && ( +
+ + {error} +
+ )} + + {customRoles.length === 0 && !draft && ( +

+ No custom roles yet. Create one to define an exact set of sections and capabilities. +

+ )} + + {customRoles.length > 0 && ( +
    + {customRoles.map((role) => ( +
  • +
    +
    + {role.displayName || roleLabel(role.name)} +
    +
    + {(role.permissions || []).length} permission + {(role.permissions || []).length === 1 ? '' : 's'} + {role.description ? ` · ${role.description}` : ''} +
    +
    +
    + + {confirmCode === role.name ? ( + <> + + + + ) : ( + + )} +
    +
    • + ))} +
+ )} + + {draft && ( +
+
+

+ {draft.code ? `Edit ${roleLabel(draft.code)}` : 'New role'} +

+ +
+ +
+ setDraft({ ...draft, name: e.target.value })} + placeholder="Role name (e.g. Analyst)" + className="px-3 py-1.5 text-sm border border-gray-200 rounded-md focus:outline-none focus:ring-2 focus:ring-gray-900" + /> + setDraft({ ...draft, description: e.target.value })} + placeholder="Description (optional)" + className="px-3 py-1.5 text-sm border border-gray-200 rounded-md focus:outline-none focus:ring-2 focus:ring-gray-900" + /> +
+ {draft.code && ( +

+ Renaming changes the label only — people already assigned keep this role. +

+ )} + +
+ {PERMISSION_GROUPS.map((group) => ( +
+
+ {group.label} +
+
+ {group.items.map(([code, label]) => ( + + ))} +
+
+ ))} +
+ +
+ + +
+
+ )} +
+ ) +} diff --git a/src/hooks/useAuth.jsx b/src/hooks/useAuth.jsx index 2bd1663..fca85c5 100644 --- a/src/hooks/useAuth.jsx +++ b/src/hooks/useAuth.jsx @@ -9,8 +9,8 @@ import { useConnectionStore } from '@/lib/stores/useConnectionStore' import { useDashboardStore } from '@/lib/stores/useDashboardStore' import { useNavStore } from '@/lib/stores/useNavStore' -export { PERMISSIONS, ROLES, ROLE_LEVELS, normalizeRole, roleIsAtLeast } from '@/lib/permissions' -import { PERMISSIONS, ROLES, ROLE_LEVELS, normalizeRole } from '@/lib/permissions' +export { PERMISSIONS, ROLES, ROLE_LABELS, normalizeRole, roleLabel, isAdminRole, isBuiltInRole } from '@/lib/permissions' +import { PERMISSIONS, ROLES, ROLE_BASELINE_PERMISSIONS, normalizeRole, isAdminRole, roleLabel } from '@/lib/permissions' const AuthContext = createContext(null) @@ -86,6 +86,8 @@ export function AuthProvider({ children }) { email: payload?.email || '', emailVerified: payload?.emailVerified ?? false, accountStatus: payload?.accountStatus || null, + // Display label for a custom role, which has no entry in ROLE_LABELS. + roleName: payload?.roleName || null, emailTwoFactorEnabled: payload?.emailTwoFactorEnabled ?? false, mfaRequired: false, mfaEnrolled: false, @@ -263,16 +265,28 @@ export function AuthProvider({ children }) { return role === normalizeRole(requiredRole) }, [role]) + /** + * Whether the user holds every permission the named built-in role holds by default. + * + *

Formerly a rank comparison over a role hierarchy. The roles no longer nest, so + * the question "is my role at least X" is answered by permission containment instead: + * an Admin passes everything, and a custom role passes exactly when it was granted the + * same capabilities. Kept because PermissionGuard/ActionGuard still expose a minRole + * prop. + */ const hasRoleLevel = useCallback((requiredRole) => { - const normalizedRole = normalizeRole(role) + if (isAdminRole(role)) return true const normalizedRequiredRole = normalizeRole(requiredRole) - const userLevel = normalizedRole ? (ROLE_LEVELS[normalizedRole] ?? -1) : -1 - const requiredLevel = normalizedRequiredRole ? (ROLE_LEVELS[normalizedRequiredRole] ?? 999) : 999 - return userLevel >= requiredLevel - }, [role]) - - const isAdmin = useMemo(() => role === ROLES.ADMIN, [role]) - const isDeveloper = useMemo(() => role === ROLES.DEVELOPER, [role]) + if (!normalizedRequiredRole) return false + if (normalizedRequiredRole === normalizeRole(role)) return true + const required = ROLE_BASELINE_PERMISSIONS[normalizedRequiredRole] + if (!required) return false + return required.every((permission) => permissions.has(permission)) + }, [role, permissions]) + + const isAdmin = useMemo(() => isAdminRole(role), [role]) + const isDeveloper = useMemo(() => normalizeRole(role) === ROLES.DEVELOPER, [role]) + const roleDisplayName = useMemo(() => roleLabel(role, user?.roleName), [role, user?.roleName]) const impersonating = Boolean(user?.impersonating) const canSwitchProfile = isAdmin || impersonating @@ -298,6 +312,7 @@ export function AuthProvider({ children }) { impersonatorEmail: user?.impersonatorEmail || null, canSwitchProfile, role, + roleDisplayName, permissions: [...permissions], hasPermission, hasAnyPermission, @@ -323,6 +338,7 @@ export function AuthProvider({ children }) { stopImpersonation, user, role, + roleDisplayName, permissions, impersonating, canSwitchProfile, diff --git a/src/lib/permissions.js b/src/lib/permissions.js index 1ec222a..e9804ec 100644 --- a/src/lib/permissions.js +++ b/src/lib/permissions.js @@ -1,78 +1,141 @@ /** - * Permission constants - mirrors backend Permission enum. + * Permission and role constants — mirrors the backend Permission and Role enums. * - * These are the permission codes returned by the backend. - * DO NOT use these directly in components - use ACTIONS instead. + * The backend is the authority: `/auth/me` and `/permissions/me` return the user's + * effective permission codes, and the UI gates on those. These constants exist so + * components refer to a permission by name instead of a string literal. */ export const PERMISSIONS = { - // Admin-only product area permissions + // Section / menu permissions — one per top-level sidebar destination. + VIEW_AGENT: 'VIEW_AGENT', + VIEW_DASHBOARDS: 'VIEW_DASHBOARDS', + VIEW_DIGEST: 'VIEW_DIGEST', + VIEW_BRAIN: 'VIEW_BRAIN', + VIEW_PERFORMANCE: 'VIEW_PERFORMANCE', + VIEW_EDITOR: 'VIEW_EDITOR', + + // Read permissions VIEW_DASHBOARD: 'VIEW_DASHBOARD', VIEW_SCHEMA: 'VIEW_SCHEMA', VIEW_SLOW_QUERIES: 'VIEW_SLOW_QUERIES', - VIEW_BRAIN: 'VIEW_BRAIN', - VIEW_PERFORMANCE: 'VIEW_PERFORMANCE', VIEW_GROWTH: 'VIEW_GROWTH', VIEW_PLAYBOOKS: 'VIEW_PLAYBOOKS', - // Developer permissions + // Core product permissions EXECUTE_QUERIES: 'EXECUTE_QUERIES', USE_CHAT: 'USE_CHAT', EXPORT_DATA: 'EXPORT_DATA', - // Admin-only action permissions + // Action permissions RUN_ANALYSIS: 'RUN_ANALYSIS', RUN_INGESTION: 'RUN_INGESTION', EXECUTE_PLAYBOOKS: 'EXECUTE_PLAYBOOKS', USE_INDEX_ADVISOR: 'USE_INDEX_ADVISOR', MANAGE_ALERTS: 'MANAGE_ALERTS', - // Admin permissions (ADMIN only) + // Workspaces + MANAGE_DASHBOARD_WORKSPACES: 'MANAGE_DASHBOARD_WORKSPACES', + + // Administrative permissions MANAGE_CONNECTIONS: 'MANAGE_CONNECTIONS', + MANAGE_SETTINGS: 'MANAGE_SETTINGS', MANAGE_USERS: 'MANAGE_USERS', MANAGE_INVITE_CODES: 'MANAGE_INVITE_CODES', - MANAGE_SETTINGS: 'MANAGE_SETTINGS', MANAGE_PERMISSIONS: 'MANAGE_PERMISSIONS', } /** - * Role constants - mirrors backend Role enum. + * Built-in role codes. A user's role may also be a custom role code, which will not + * appear here — never treat "not in ROLES" as "invalid role". */ export const ROLES = { - DEVELOPER: 'DEVELOPER', ADMIN: 'ADMIN', + DBA: 'DBA', + DATA_ENGINEER: 'DATA_ENGINEER', + DEVELOPER: 'DEVELOPER', } +/** Display labels for the built-in roles. */ +export const ROLE_LABELS = { + [ROLES.ADMIN]: 'Admin', + [ROLES.DBA]: 'DBA', + [ROLES.DATA_ENGINEER]: 'Data Engineer', + [ROLES.DEVELOPER]: 'Developer', +} + +/** + * The permissions each built-in role holds by default — mirrors the backend's + * Permission.defaultRoles. Used only by the legacy `minRole` guard prop to ask "does this + * user have everything role X would have"; the authoritative permission set always comes + * from the backend, never from this table. + */ +export const ROLE_BASELINE_PERMISSIONS = { + [ROLES.ADMIN]: Object.values(PERMISSIONS), + [ROLES.DBA]: [ + PERMISSIONS.VIEW_AGENT, PERMISSIONS.VIEW_DASHBOARDS, PERMISSIONS.VIEW_DIGEST, + PERMISSIONS.VIEW_BRAIN, PERMISSIONS.VIEW_PERFORMANCE, PERMISSIONS.VIEW_EDITOR, + PERMISSIONS.MANAGE_CONNECTIONS, PERMISSIONS.MANAGE_SETTINGS, + ], + [ROLES.DATA_ENGINEER]: [ + PERMISSIONS.VIEW_AGENT, PERMISSIONS.VIEW_DASHBOARDS, PERMISSIONS.VIEW_EDITOR, + ], + [ROLES.DEVELOPER]: [ + PERMISSIONS.VIEW_AGENT, PERMISSIONS.VIEW_DIGEST, PERMISSIONS.VIEW_DASHBOARDS, + PERMISSIONS.VIEW_PERFORMANCE, PERMISSIONS.VIEW_EDITOR, + ], +} + +/** Legacy role names from the two-role model still stored on old user rows. */ const ROLE_ALIASES = { - ADMIN: ROLES.ADMIN, - DEVELOPER: ROLES.DEVELOPER, EDITOR: ROLES.DEVELOPER, VIEWER: ROLES.DEVELOPER, USER: ROLES.DEVELOPER, } +/** + * Normalise a role value to its code. + * + *

Unlike the old version this does NOT collapse unknown values to DEVELOPER: a custom + * role code is a perfectly valid role, and mapping it onto a built-in one would show the + * wrong menus. Returns the uppercased code as-is for anything unrecognised. + */ export function normalizeRole(role) { if (!role) return null - return ROLE_ALIASES[String(role).trim().toUpperCase()] || null + const upper = String(role).trim().toUpperCase() + if (!upper) return null + return ROLE_ALIASES[upper] || upper } -/** - * Role hierarchy level - higher number = more permissions. - */ -export const ROLE_LEVELS = { - [ROLES.DEVELOPER]: 0, - [ROLES.ADMIN]: 1, +export function isBuiltInRole(role) { + const normalized = normalizeRole(role) + return Boolean(normalized && Object.values(ROLES).includes(normalized)) } -/** - * Check if a role is at or above another role in the hierarchy. - */ -export function roleIsAtLeast(userRole, minRole) { - const normalizedUserRole = normalizeRole(userRole) - const normalizedMinRole = normalizeRole(minRole) +/** Human label for a role code, falling back to a readable form of a custom code. */ +export function roleLabel(role, fallbackName = null) { + const normalized = normalizeRole(role) + if (!normalized) return 'Unknown' + if (ROLE_LABELS[normalized]) return ROLE_LABELS[normalized] + if (fallbackName) return fallbackName + // ANALYST_TEAM -> "Analyst Team" + return normalized + .toLowerCase() + .split('_') + .filter(Boolean) + .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) + .join(' ') +} - if (!normalizedUserRole || !normalizedMinRole) { - return false - } +export function isAdminRole(role) { + return normalizeRole(role) === ROLES.ADMIN +} - return (ROLE_LEVELS[normalizedUserRole] ?? -1) >= (ROLE_LEVELS[normalizedMinRole] ?? Number.MAX_SAFE_INTEGER) +/** + * Whether a permission set grants a permission. Admin is a fixed point on the backend + * (it holds every permission), so no special case is needed here. + */ +export function hasPermissionIn(permissions, permission) { + if (!permission) return false + if (permissions instanceof Set) return permissions.has(permission) + return Array.isArray(permissions) && permissions.includes(permission) } From 821b9c401347e68293592f2889f1c3d43fd943a1 Mon Sep 17 00:00:00 2001 From: sumit Date: Sun, 23 Aug 2026 15:54:29 +0530 Subject: [PATCH 2/5] feat(dashboards): workspaces to group dashboards with per-member access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A DashboardWorkspace groups dashboards within one connection and carries its own member list, keyed by username to match connection_access_grant so "View as" resolves membership as the target user. The access rule is an AND, and it only ever narrows: connection access is checked first and unchanged, and workspace membership is an additional gate on top. Adding someone to a workspace can therefore never grant them a connection they were not already given. saved_dashboards.workspace_id is nullable — NULL means "not grouped", governed purely by the connection ACL exactly as before. Admins bypass the membership half, as they already bypass connection grants. Non-membership reports 404, not 403: a user outside the workspace must not learn the dashboard exists. Deleting a workspace detaches its dashboards rather than cascading — deleting a grouping must never destroy the things grouped — and removing the last MANAGER is refused so a workspace cannot be orphaned. Also closes a pre-existing authorization hole this feature sat on top of: /saved-dashboards create, list, get, update and delete took a caller-supplied connectionId or id and checked nothing, so any authenticated user could read every dashboard on every connection. Verified live before the fix by reading dashboards on a connection the user held no grant on. All of them now assert connection access and the workspace gate; DashboardAlertController does the same through its single requireDashboard choke point. --- .../controller/DashboardAlertController.java | 11 +- .../DashboardWorkspaceController.java | 235 +++++++++++ .../controller/SavedDashboardController.java | 50 ++- .../dbaagent/model/DashboardWorkspace.java | 83 ++++ .../model/DashboardWorkspaceMember.java | 64 +++ .../model/DashboardWorkspaceRole.java | 30 ++ .../com/dbaagent/model/SavedDashboard.java | 8 + .../DashboardWorkspaceMemberRepository.java | 36 ++ .../DashboardWorkspaceRepository.java | 19 + .../repository/SavedDashboardRepository.java | 10 + .../service/DashboardWorkspaceService.java | 370 ++++++++++++++++++ ..._dashboard_workspaces_and_custom_roles.sql | 91 +++++ .../service/DashboardWorkspaceAccessTest.java | 212 ++++++++++ src/components/sections/DashboardsHome.jsx | 136 ++++++- src/components/sections/WorkspaceManager.jsx | 337 ++++++++++++++++ .../sections/WorkspaceManager.module.css | 366 +++++++++++++++++ 16 files changed, 2051 insertions(+), 7 deletions(-) create mode 100644 backend/src/main/java/com/dbaagent/controller/DashboardWorkspaceController.java create mode 100644 backend/src/main/java/com/dbaagent/model/DashboardWorkspace.java create mode 100644 backend/src/main/java/com/dbaagent/model/DashboardWorkspaceMember.java create mode 100644 backend/src/main/java/com/dbaagent/model/DashboardWorkspaceRole.java create mode 100644 backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceMemberRepository.java create mode 100644 backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceRepository.java create mode 100644 backend/src/main/java/com/dbaagent/service/DashboardWorkspaceService.java create mode 100644 backend/src/main/resources/db/migration/V117__create_dashboard_workspaces_and_custom_roles.sql create mode 100644 backend/src/test/java/com/dbaagent/service/DashboardWorkspaceAccessTest.java create mode 100644 src/components/sections/WorkspaceManager.jsx create mode 100644 src/components/sections/WorkspaceManager.module.css diff --git a/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java b/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java index abec91d..fb10ed7 100644 --- a/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java +++ b/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java @@ -4,6 +4,7 @@ import com.dbaagent.model.SavedDashboard; import com.dbaagent.service.DashboardAlertService; import com.dbaagent.service.SavedDashboardService; +import com.dbaagent.service.DashboardWorkspaceService; import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -25,6 +26,7 @@ public class DashboardAlertController { private final DashboardAlertService alertService; private final SavedDashboardService savedDashboardService; private final AccessControlService accessControlService; + private final DashboardWorkspaceService dashboardWorkspaceService; @PostMapping public ResponseEntity> create(@PathVariable UUID dashboardId, @RequestBody DashboardAlert draft) { @@ -93,8 +95,15 @@ public ResponseEntity> delete(@PathVariable UUID dashboardId } } + /** + * The single point every handler here resolves a dashboard through, so the workspace + * membership gate applies to all of them at once. The connection check stays with + * each caller because read and write paths need different assertions. + */ private SavedDashboard requireDashboard(UUID dashboardId) { - return savedDashboardService.getDashboardById(dashboardId) + SavedDashboard dashboard = savedDashboardService.getDashboardById(dashboardId) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); + dashboardWorkspaceService.assertCanReadDashboard(dashboard); + return dashboard; } } diff --git a/backend/src/main/java/com/dbaagent/controller/DashboardWorkspaceController.java b/backend/src/main/java/com/dbaagent/controller/DashboardWorkspaceController.java new file mode 100644 index 0000000..161cff6 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/controller/DashboardWorkspaceController.java @@ -0,0 +1,235 @@ +package com.dbaagent.controller; + +import com.dbaagent.model.DashboardWorkspace; +import com.dbaagent.model.DashboardWorkspaceMember; +import com.dbaagent.model.SavedDashboard; +import com.dbaagent.repository.SavedDashboardRepository; +import com.dbaagent.service.DashboardWorkspaceService; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.*; +import org.springframework.web.server.ResponseStatusException; + +import java.util.*; + +/** + * Dashboard workspaces: grouping dashboards with their own member list. + * + * 

+ *   GET    /dashboard-workspaces/connection/{connectionId}   workspaces I can see
+ *   POST   /dashboard-workspaces                             create
+ *   GET    /dashboard-workspaces/{id}                        one workspace
+ *   PUT    /dashboard-workspaces/{id}                        rename / recolour
+ *   DELETE /dashboard-workspaces/{id}                        delete (detaches dashboards)
+ *   GET    /dashboard-workspaces/{id}/dashboards             dashboards inside it
+ *   GET    /dashboard-workspaces/{id}/members                member list
+ *   POST   /dashboard-workspaces/{id}/members                add or change a member
+ *   DELETE /dashboard-workspaces/{id}/members/{username}     remove a member
+ *   PUT    /dashboard-workspaces/dashboards/{dashboardId}    move a dashboard in/out
+ *
+ * + *

Every method delegates its access check to {@link DashboardWorkspaceService}, which + * asserts connection access first and workspace membership second. As elsewhere in this + * codebase there is no filter doing this for you — a new endpoint here must call the + * service, never the repositories directly. + */ +@RestController +@RequestMapping("/dashboard-workspaces") +@RequiredArgsConstructor +@Slf4j +public class DashboardWorkspaceController { + + private final DashboardWorkspaceService workspaceService; + private final SavedDashboardRepository savedDashboardRepository; + + @GetMapping("/connection/{connectionId}") + public ResponseEntity listWorkspaces(@PathVariable String connectionId) { + try { + List workspaces = workspaceService.listVisibleWorkspaces(connectionId); + return ResponseEntity.ok(workspaces.stream().map(this::describe).toList()); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error listing dashboard workspaces", e); + return failure("Failed to load workspaces"); + } + } + + @PostMapping + public ResponseEntity createWorkspace(@RequestBody Map body) { + try { + String connectionId = asString(body.get("connectionId")); + if (connectionId == null) { + return ResponseEntity.badRequest().body(Map.of("success", false, "message", "connectionId is required")); + } + DashboardWorkspace workspace = workspaceService.createWorkspace( + connectionId, + asString(body.get("name")), + asString(body.get("description")), + asString(body.get("color")) + ); + return ResponseEntity.ok(describe(workspace)); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error creating dashboard workspace", e); + return failure("Failed to create workspace"); + } + } + + @GetMapping("/{id}") + public ResponseEntity getWorkspace(@PathVariable UUID id) { + try { + return ResponseEntity.ok(describe(workspaceService.getWorkspace(id))); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error loading dashboard workspace", e); + return failure("Failed to load workspace"); + } + } + + @PutMapping("/{id}") + public ResponseEntity updateWorkspace(@PathVariable UUID id, @RequestBody Map body) { + try { + DashboardWorkspace workspace = workspaceService.updateWorkspace( + id, + asString(body.get("name")), + // Distinguish "omitted" from "cleared": a present-but-blank value clears + // the field, matching updateDashboard's convention. + body.containsKey("description") ? String.valueOf(Objects.toString(body.get("description"), "")) : null, + body.containsKey("color") ? String.valueOf(Objects.toString(body.get("color"), "")) : null + ); + return ResponseEntity.ok(describe(workspace)); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error updating dashboard workspace", e); + return failure("Failed to update workspace"); + } + } + + @DeleteMapping("/{id}") + public ResponseEntity deleteWorkspace(@PathVariable UUID id) { + try { + workspaceService.deleteWorkspace(id); + return ResponseEntity.ok(Map.of("success", true)); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error deleting dashboard workspace", e); + return failure("Failed to delete workspace"); + } + } + + @GetMapping("/{id}/dashboards") + public ResponseEntity listDashboards(@PathVariable UUID id) { + try { + return ResponseEntity.ok(workspaceService.listDashboards(id)); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error listing workspace dashboards", e); + return failure("Failed to load dashboards"); + } + } + + @GetMapping("/{id}/members") + public ResponseEntity listMembers(@PathVariable UUID id) { + try { + List members = workspaceService.listMembers(id); + return ResponseEntity.ok(members.stream().map(this::describeMember).toList()); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error listing workspace members", e); + return failure("Failed to load members"); + } + } + + @PostMapping("/{id}/members") + public ResponseEntity addMember(@PathVariable UUID id, @RequestBody Map body) { + try { + DashboardWorkspaceMember member = workspaceService.addMember( + id, asString(body.get("username")), asString(body.get("workspaceRole"))); + return ResponseEntity.ok(describeMember(member)); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error adding workspace member", e); + return failure("Failed to add member"); + } + } + + @DeleteMapping("/{id}/members/{username}") + public ResponseEntity removeMember(@PathVariable UUID id, @PathVariable String username) { + try { + workspaceService.removeMember(id, username); + return ResponseEntity.ok(Map.of("success", true)); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error removing workspace member", e); + return failure("Failed to remove member"); + } + } + + /** Move a dashboard into a workspace, or out of one with a null/blank workspaceId. */ + @PutMapping("/dashboards/{dashboardId}") + public ResponseEntity moveDashboard(@PathVariable UUID dashboardId, @RequestBody Map body) { + try { + String raw = asString(body.get("workspaceId")); + UUID target = raw == null ? null : UUID.fromString(raw); + SavedDashboard dashboard = workspaceService.moveDashboard(dashboardId, target); + return ResponseEntity.ok(Map.of( + "success", true, + "dashboardId", dashboard.getId().toString(), + "workspaceId", dashboard.getWorkspaceId() == null ? "" : dashboard.getWorkspaceId().toString() + )); + } catch (IllegalArgumentException e) { + return ResponseEntity.badRequest().body(Map.of("success", false, "message", "Invalid workspace id")); + } catch (ResponseStatusException e) { + throw e; + } catch (Exception e) { + log.error("Error moving dashboard between workspaces", e); + return failure("Failed to move dashboard"); + } + } + + private Map describe(DashboardWorkspace workspace) { + Map body = new LinkedHashMap<>(); + body.put("id", workspace.getId().toString()); + body.put("connectionId", workspace.getConnectionId()); + body.put("name", workspace.getName()); + body.put("description", workspace.getDescription()); + body.put("color", workspace.getColor()); + body.put("createdBy", workspace.getCreatedBy()); + body.put("createdAt", workspace.getCreatedAt()); + body.put("updatedAt", workspace.getUpdatedAt()); + body.put("dashboardCount", savedDashboardRepository.countByWorkspaceId(workspace.getId())); + return body; + } + + private Map describeMember(DashboardWorkspaceMember member) { + Map body = new LinkedHashMap<>(); + body.put("id", member.getId().toString()); + body.put("username", member.getUsername()); + body.put("workspaceRole", member.getWorkspaceRole().name()); + body.put("addedBy", member.getAddedBy()); + body.put("createdAt", member.getCreatedAt()); + return body; + } + + private static ResponseEntity> failure(String message) { + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(Map.of("success", false, "message", message)); + } + + private static String asString(Object value) { + if (value == null) return null; + String s = String.valueOf(value).trim(); + return s.isEmpty() ? null : s; + } +} diff --git a/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java b/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java index 7dc8ca5..34bc8ec 100644 --- a/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java +++ b/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java @@ -3,6 +3,7 @@ import com.dbaagent.model.DashboardVersion; import com.dbaagent.model.SavedDashboard; import com.dbaagent.service.ConnectionChatAccessPolicyService; +import com.dbaagent.service.DashboardWorkspaceService; import com.dbaagent.service.SavedDashboardService; import com.dbaagent.service.security.AccessControlService; import lombok.extern.slf4j.Slf4j; @@ -31,6 +32,9 @@ public class SavedDashboardController { @Autowired private ConnectionChatAccessPolicyService connectionChatAccessPolicyService; + @Autowired + private DashboardWorkspaceService dashboardWorkspaceService; + // Every write method below is load-then-save on a row a background generation // turn (SavedDashboardService.beginGenerationTurn etc.) may be writing at the // same time. Without this helper, the loser's raw Hibernate message @@ -44,6 +48,17 @@ private static ResponseEntity> conflict(OptimisticLockingFai return ResponseEntity.status(HttpStatus.CONFLICT).body(body); } + /** + * A dashboard may only be created into a workspace the caller can actually manage — + * otherwise anyone could push a dashboard into someone else's workspace. + */ + private void assertWorkspaceAssignable(String connectionId, UUID workspaceId) { + if (workspaceId == null) { + return; + } + dashboardWorkspaceService.getWorkspace(workspaceId); + } + /** Publish this dashboard to the web (opt-in, revocable public link). */ @PostMapping("/{id}/share") public ResponseEntity> enableShare(@PathVariable UUID id) { @@ -51,6 +66,7 @@ public ResponseEntity> enableShare(@PathVariable UUID id) { SavedDashboard existing = savedDashboardService.getDashboardById(id) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); accessControlService.assertCanReadConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); if (connectionChatAccessPolicyService.hasActivePolicy(existing.getConnectionId())) { return ResponseEntity.status(HttpStatus.CONFLICT).body(Map.of( "success", false, @@ -81,6 +97,7 @@ public ResponseEntity> setSharePassword(@PathVariable UUID i SavedDashboard existing = savedDashboardService.getDashboardById(id) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); accessControlService.assertCanReadConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); SavedDashboard d = savedDashboardService.setSharePassword(id, body == null ? null : body.get("password")); return ResponseEntity.ok(Map.of("success", true, "sharePasswordSet", d.isSharePasswordSet())); } catch (IllegalArgumentException e) { @@ -103,6 +120,7 @@ public ResponseEntity> disableShare(@PathVariable UUID id) { SavedDashboard existing = savedDashboardService.getDashboardById(id) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); accessControlService.assertCanReadConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); savedDashboardService.disablePublicShare(id); return ResponseEntity.ok(Map.of("success", true, "isPublic", false)); } catch (IllegalArgumentException e) { @@ -125,6 +143,8 @@ public ResponseEntity> disableShare(@PathVariable UUID id) { public ResponseEntity> createDashboard(@RequestBody SavedDashboard savedDashboard) { try { log.info("Creating saved dashboard: {} for connection: {}", savedDashboard.getName(), savedDashboard.getConnectionId()); + accessControlService.assertCanManageConnectionContent(savedDashboard.getConnectionId()); + assertWorkspaceAssignable(savedDashboard.getConnectionId(), savedDashboard.getWorkspaceId()); SavedDashboard created = savedDashboardService.saveDashboard(savedDashboard); @@ -152,8 +172,10 @@ public ResponseEntity> createDashboard(@RequestBody SavedDas public ResponseEntity> getDashboardsByConnection(@PathVariable String connectionId) { try { log.info("Fetching saved dashboards for connection: {}", connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); - List dashboards = savedDashboardService.getDashboardsByConnection(connectionId); + List dashboards = dashboardWorkspaceService.filterReadable( + savedDashboardService.getDashboardsByConnection(connectionId)); Map response = new HashMap<>(); response.put("success", true); @@ -182,6 +204,8 @@ public ResponseEntity> getDashboardById(@PathVariable UUID i return savedDashboardService.getDashboardById(id) .map(dashboard -> { + accessControlService.assertCanReadConnectionContent(dashboard.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(dashboard); Map response = new HashMap<>(); response.put("success", true); response.put("savedDashboard", dashboard); @@ -211,6 +235,10 @@ public ResponseEntity> getDashboardById(@PathVariable UUID i public ResponseEntity> updateDashboard(@PathVariable UUID id, @RequestBody SavedDashboard updates) { try { log.info("Updating saved dashboard: {}", id); + SavedDashboard existing = savedDashboardService.getDashboardById(id) + .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); + accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); SavedDashboard updated = savedDashboardService.updateDashboard(id, updates); @@ -248,6 +276,7 @@ public ResponseEntity> cloneDashboard(@PathVariable UUID id) SavedDashboard existing = savedDashboardService.getDashboardById(id) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); SavedDashboard clone = savedDashboardService.cloneDashboard(id); Map response = new HashMap<>(); response.put("success", true); @@ -273,6 +302,7 @@ public ResponseEntity> getVersionHistory(@PathVariable UUID SavedDashboard existing = savedDashboardService.getDashboardById(id) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); accessControlService.assertCanReadConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); List versions = savedDashboardService.getVersionHistory(id); Map response = new HashMap<>(); response.put("success", true); @@ -298,6 +328,7 @@ public ResponseEntity> restoreVersion(@PathVariable UUID id, SavedDashboard existing = savedDashboardService.getDashboardById(id) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); SavedDashboard restored = savedDashboardService.restoreVersion(id, versionId); Map response = new HashMap<>(); response.put("success", true); @@ -323,6 +354,10 @@ public ResponseEntity> restoreVersion(@PathVariable UUID id, public ResponseEntity> deleteDashboard(@PathVariable UUID id) { try { log.info("Deleting saved dashboard: {}", id); + SavedDashboard existing = savedDashboardService.getDashboardById(id) + .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); + accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); + dashboardWorkspaceService.assertCanReadDashboard(existing); savedDashboardService.deleteDashboard(id); @@ -384,8 +419,10 @@ public ResponseEntity> toggleFavorite(@PathVariable UUID id) public ResponseEntity> getFavoriteDashboards(@PathVariable String connectionId) { try { log.info("Fetching favorite dashboards for connection: {}", connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); - List dashboards = savedDashboardService.getFavoriteDashboards(connectionId); + List dashboards = dashboardWorkspaceService.filterReadable( + savedDashboardService.getFavoriteDashboards(connectionId)); Map response = new HashMap<>(); response.put("success", true); @@ -413,8 +450,10 @@ public ResponseEntity> getDashboardsByFolder( @PathVariable String folder) { try { log.info("Fetching dashboards in folder: {} for connection: {}", folder, connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); - List dashboards = savedDashboardService.getDashboardsByFolder(connectionId, folder); + List dashboards = dashboardWorkspaceService.filterReadable( + savedDashboardService.getDashboardsByFolder(connectionId, folder)); Map response = new HashMap<>(); response.put("success", true); @@ -442,8 +481,10 @@ public ResponseEntity> searchDashboards( @RequestParam String q) { try { log.info("Searching dashboards for connection: {} with term: {}", connectionId, q); + accessControlService.assertCanReadConnectionContent(connectionId); - List dashboards = savedDashboardService.searchDashboards(connectionId, q); + List dashboards = dashboardWorkspaceService.filterReadable( + savedDashboardService.searchDashboards(connectionId, q)); Map response = new HashMap<>(); response.put("success", true); @@ -469,6 +510,7 @@ public ResponseEntity> searchDashboards( public ResponseEntity> getFolders(@PathVariable String connectionId) { try { log.info("Fetching folders for connection: {}", connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); List folders = savedDashboardService.getFolders(connectionId); diff --git a/backend/src/main/java/com/dbaagent/model/DashboardWorkspace.java b/backend/src/main/java/com/dbaagent/model/DashboardWorkspace.java new file mode 100644 index 0000000..785c53c --- /dev/null +++ b/backend/src/main/java/com/dbaagent/model/DashboardWorkspace.java @@ -0,0 +1,83 @@ +package com.dbaagent.model; + +import jakarta.persistence.*; +import org.hibernate.annotations.CreationTimestamp; +import org.hibernate.annotations.UpdateTimestamp; + +import java.time.LocalDateTime; +import java.util.UUID; + +/** + * A named group of dashboards within one connection, with its own member list. + * + *

Workspace access narrows, never widens: a dashboard in a workspace is + * visible only to someone who both has read access to the connection (the existing + * per-connection ACL, unchanged) and is a member of the workspace. Admins bypass the + * membership half, matching how they already bypass connection grants. A dashboard with + * no workspace behaves exactly as it does today. + * + *

Scoped to a connection because every dashboard already is — a workspace spanning + * connections would have to re-check the connection ACL per dashboard anyway, so it + * would group things the ACL then pulls back apart. + */ +@Entity +@Table(name = "dashboard_workspaces", indexes = { + @Index(name = "idx_dashboard_workspaces_connection", columnList = "connection_id") +}, uniqueConstraints = { + @UniqueConstraint(name = "ux_dashboard_workspaces_conn_name", columnNames = {"connection_id", "name"}) +}) +public class DashboardWorkspace { + + @Id + @GeneratedValue(strategy = GenerationType.UUID) + private UUID id; + + @Column(name = "connection_id", nullable = false) + private String connectionId; + + @Column(nullable = false, length = 128) + private String name; + + @Column(length = 500) + private String description; + + /** Short colour token the UI uses for the workspace chip. */ + @Column(length = 32) + private String color; + + /** Username of the creator; always an implicit MANAGER member. */ + @Column(name = "created_by", nullable = false, length = 255) + private String createdBy; + + @CreationTimestamp + @Column(name = "created_at", nullable = false, updatable = false) + private LocalDateTime createdAt; + + @UpdateTimestamp + @Column(name = "updated_at", nullable = false) + private LocalDateTime updatedAt; + + public UUID getId() { return id; } + public void setId(UUID id) { this.id = id; } + + public String getConnectionId() { return connectionId; } + public void setConnectionId(String connectionId) { this.connectionId = connectionId; } + + public String getName() { return name; } + public void setName(String name) { this.name = name; } + + public String getDescription() { return description; } + public void setDescription(String description) { this.description = description; } + + public String getColor() { return color; } + public void setColor(String color) { this.color = color; } + + public String getCreatedBy() { return createdBy; } + public void setCreatedBy(String createdBy) { this.createdBy = createdBy; } + + public LocalDateTime getCreatedAt() { return createdAt; } + public void setCreatedAt(LocalDateTime createdAt) { this.createdAt = createdAt; } + + public LocalDateTime getUpdatedAt() { return updatedAt; } + public void setUpdatedAt(LocalDateTime updatedAt) { this.updatedAt = updatedAt; } +} diff --git a/backend/src/main/java/com/dbaagent/model/DashboardWorkspaceMember.java b/backend/src/main/java/com/dbaagent/model/DashboardWorkspaceMember.java new file mode 100644 index 0000000..f332a8c --- /dev/null +++ b/backend/src/main/java/com/dbaagent/model/DashboardWorkspaceMember.java @@ -0,0 +1,64 @@ +package com.dbaagent.model; + +import jakarta.persistence.*; +import org.hibernate.annotations.CreationTimestamp; + +import java.time.LocalDateTime; +import java.util.UUID; + +/** + * One user's membership of one {@link DashboardWorkspace}. + * + *

Keyed by username rather than user id to match how the rest of the access layer + * identifies actors ({@code ConnectionAccessGrant}, {@code AccessControlService}), so an + * impersonated ("View as") session resolves membership as the target user without a + * second lookup. + */ +@Entity +@Table(name = "dashboard_workspace_members", indexes = { + @Index(name = "idx_dashboard_ws_members_workspace", columnList = "workspace_id"), + @Index(name = "idx_dashboard_ws_members_username", columnList = "username") +}, uniqueConstraints = { + @UniqueConstraint(name = "ux_dashboard_ws_member", columnNames = {"workspace_id", "username"}) +}) +public class DashboardWorkspaceMember { + + @Id + @GeneratedValue(strategy = GenerationType.UUID) + private UUID id; + + @Column(name = "workspace_id", nullable = false) + private UUID workspaceId; + + @Column(nullable = false, length = 255) + private String username; + + @Enumerated(EnumType.STRING) + @Column(name = "workspace_role", nullable = false, length = 32) + private DashboardWorkspaceRole workspaceRole = DashboardWorkspaceRole.VIEWER; + + @Column(name = "added_by", length = 255) + private String addedBy; + + @CreationTimestamp + @Column(name = "created_at", nullable = false, updatable = false) + private LocalDateTime createdAt; + + public UUID getId() { return id; } + public void setId(UUID id) { this.id = id; } + + public UUID getWorkspaceId() { return workspaceId; } + public void setWorkspaceId(UUID workspaceId) { this.workspaceId = workspaceId; } + + public String getUsername() { return username; } + public void setUsername(String username) { this.username = username; } + + public DashboardWorkspaceRole getWorkspaceRole() { return workspaceRole; } + public void setWorkspaceRole(DashboardWorkspaceRole workspaceRole) { this.workspaceRole = workspaceRole; } + + public String getAddedBy() { return addedBy; } + public void setAddedBy(String addedBy) { this.addedBy = addedBy; } + + public LocalDateTime getCreatedAt() { return createdAt; } + public void setCreatedAt(LocalDateTime createdAt) { this.createdAt = createdAt; } +} diff --git a/backend/src/main/java/com/dbaagent/model/DashboardWorkspaceRole.java b/backend/src/main/java/com/dbaagent/model/DashboardWorkspaceRole.java new file mode 100644 index 0000000..973f018 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/model/DashboardWorkspaceRole.java @@ -0,0 +1,30 @@ +package com.dbaagent.model; + +/** + * A member's role within one dashboard workspace. + * + *

Deliberately two values, not a copy of the product's role model: this answers only + * "can this member change the workspace itself", on top of whatever the member's product + * role and connection grant already allow. + */ +public enum DashboardWorkspaceRole { + /** Can open the workspace and the dashboards in it. */ + VIEWER, + + /** VIEWER, plus renaming the workspace, adding/removing members, and moving dashboards in or out. */ + MANAGER; + + public boolean canManage() { + return this == MANAGER; + } + + public static DashboardWorkspaceRole fromString(String value) { + if (value == null || value.isBlank()) { + return VIEWER; + } + return switch (value.trim().toUpperCase()) { + case "MANAGER", "ADMIN", "OWNER", "EDITOR" -> MANAGER; + default -> VIEWER; + }; + } +} diff --git a/backend/src/main/java/com/dbaagent/model/SavedDashboard.java b/backend/src/main/java/com/dbaagent/model/SavedDashboard.java index c1f7201..60c5fc4 100644 --- a/backend/src/main/java/com/dbaagent/model/SavedDashboard.java +++ b/backend/src/main/java/com/dbaagent/model/SavedDashboard.java @@ -15,6 +15,7 @@ @Index(name = "idx_saved_dashboards_connection_id", columnList = "connectionId"), @Index(name = "idx_saved_dashboards_user_id", columnList = "userId"), @Index(name = "idx_saved_dashboards_is_favorite", columnList = "isFavorite"), + @Index(name = "idx_saved_dashboards_workspace_id", columnList = "workspace_id"), @Index(name = "idx_saved_dashboards_created_at", columnList = "createdAt") }) @Data @@ -77,6 +78,13 @@ public boolean isSharePasswordSet() { @Column(length = 255) private String folder; + // Optional grouping with its own member list (DashboardWorkspace). Null means the + // dashboard is not in a workspace and is governed purely by the connection ACL, as + // every dashboard was before workspaces existed. When set, a viewer needs BOTH + // connection read access AND workspace membership (admins bypass the latter). + @Column(name = "workspace_id") + private UUID workspaceId; + // Server-owned "is a generation turn in flight for this dashboard" marker. // Set to RUNNING the instant a chat submit is accepted (before the slow // agent work starts) and back to IDLE when it finishes — from the backend diff --git a/backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceMemberRepository.java b/backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceMemberRepository.java new file mode 100644 index 0000000..2196203 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceMemberRepository.java @@ -0,0 +1,36 @@ +package com.dbaagent.repository; + +import com.dbaagent.model.DashboardWorkspaceMember; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; +import org.springframework.transaction.annotation.Transactional; + +import java.util.Collection; +import java.util.List; +import java.util.Optional; +import java.util.UUID; + +@Repository +public interface DashboardWorkspaceMemberRepository extends JpaRepository { + + List findByWorkspaceIdOrderByUsernameAsc(UUID workspaceId); + + Optional findByWorkspaceIdAndUsernameIgnoreCase(UUID workspaceId, String username); + + List findByUsernameIgnoreCase(String username); + + /** + * Memberships this user holds among the given workspaces. Used to resolve visibility + * for a whole dashboard list in one query rather than one per dashboard. + */ + List findByUsernameIgnoreCaseAndWorkspaceIdIn(String username, Collection workspaceIds); + + // Derived deletes need their own transaction; a self-invoked @Transactional caller + // does not supply one (Spring proxies are bypassed by this::), which is the same + // trap McpTokenRepository.deleteByUserId documents. + @Transactional + void deleteByWorkspaceId(UUID workspaceId); + + @Transactional + void deleteByWorkspaceIdAndUsernameIgnoreCase(UUID workspaceId, String username); +} diff --git a/backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceRepository.java b/backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceRepository.java new file mode 100644 index 0000000..0169978 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/repository/DashboardWorkspaceRepository.java @@ -0,0 +1,19 @@ +package com.dbaagent.repository; + +import com.dbaagent.model.DashboardWorkspace; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; + +import java.util.List; +import java.util.Optional; +import java.util.UUID; + +@Repository +public interface DashboardWorkspaceRepository extends JpaRepository { + + List findByConnectionIdOrderByNameAsc(String connectionId); + + Optional findByConnectionIdAndNameIgnoreCase(String connectionId, String name); + + void deleteByConnectionId(String connectionId); +} diff --git a/backend/src/main/java/com/dbaagent/repository/SavedDashboardRepository.java b/backend/src/main/java/com/dbaagent/repository/SavedDashboardRepository.java index 2fb0f58..653932f 100644 --- a/backend/src/main/java/com/dbaagent/repository/SavedDashboardRepository.java +++ b/backend/src/main/java/com/dbaagent/repository/SavedDashboardRepository.java @@ -55,6 +55,16 @@ List findByTag(@Param("connectionId") String connectionId, */ long countByConnectionIdAndFolder(String connectionId, String folder); + /** + * Dashboards inside one workspace, newest activity first. + */ + List findByWorkspaceIdOrderByUpdatedAtDesc(UUID workspaceId); + + /** + * Count dashboards grouped into a workspace (shown on the workspace chip). + */ + long countByWorkspaceId(UUID workspaceId); + /** * Get distinct folders for a connection */ diff --git a/backend/src/main/java/com/dbaagent/service/DashboardWorkspaceService.java b/backend/src/main/java/com/dbaagent/service/DashboardWorkspaceService.java new file mode 100644 index 0000000..7c1260e --- /dev/null +++ b/backend/src/main/java/com/dbaagent/service/DashboardWorkspaceService.java @@ -0,0 +1,370 @@ +package com.dbaagent.service; + +import com.dbaagent.model.DashboardWorkspace; +import com.dbaagent.model.DashboardWorkspaceMember; +import com.dbaagent.model.DashboardWorkspaceRole; +import com.dbaagent.model.SavedDashboard; +import com.dbaagent.repository.DashboardWorkspaceMemberRepository; +import com.dbaagent.repository.DashboardWorkspaceRepository; +import com.dbaagent.repository.SavedDashboardRepository; +import com.dbaagent.service.security.AccessControlService; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; +import org.springframework.web.server.ResponseStatusException; + +import java.util.*; +import java.util.stream.Collectors; + +import static org.springframework.http.HttpStatus.FORBIDDEN; +import static org.springframework.http.HttpStatus.NOT_FOUND; + +/** + * Dashboard workspaces: named groups of dashboards with their own member lists. + * + *

The access rule is an AND, deliberately. Connection access is checked first and + * unchanged — {@code AccessControlService.assertCanReadConnectionContent} — and workspace + * membership is an additional gate on top. Membership therefore can only ever + * narrow what a user sees, never widen it, so introducing a workspace cannot hand anyone + * access to a connection they were not already granted. + * + *

Admins bypass the membership half, matching how they already bypass connection + * grants ({@code isCurrentUserAdmin}). Under "View as", {@code ImpersonationContext} has + * already replaced the principal, so membership resolves as the target user and an admin + * viewing as someone else correctly sees only that user's workspaces. + */ +@Service +@RequiredArgsConstructor +@Slf4j +public class DashboardWorkspaceService { + + private final DashboardWorkspaceRepository workspaceRepository; + private final DashboardWorkspaceMemberRepository memberRepository; + private final SavedDashboardRepository savedDashboardRepository; + private final AccessControlService accessControlService; + + // ==================== Queries ==================== + + /** Workspaces on this connection that the caller can see. */ + public List listVisibleWorkspaces(String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); + List all = workspaceRepository.findByConnectionIdOrderByNameAsc(connectionId); + if (accessControlService.isCurrentUserAdmin()) { + return all; + } + Set memberOf = memberWorkspaceIds(currentUsername(), all); + return all.stream() + .filter(ws -> memberOf.contains(ws.getId())) + .collect(Collectors.toList()); + } + + /** + * The workspace ids the caller may read on this connection, plus whether the caller + * sees everything. Callers filtering a dashboard list use this to avoid a membership + * query per dashboard. + */ + public WorkspaceVisibility resolveVisibility(String connectionId) { + if (accessControlService.isCurrentUserAdmin()) { + return new WorkspaceVisibility(true, Set.of()); + } + List all = workspaceRepository.findByConnectionIdOrderByNameAsc(connectionId); + return new WorkspaceVisibility(false, memberWorkspaceIds(currentUsername(), all)); + } + + public DashboardWorkspace getWorkspace(UUID workspaceId) { + DashboardWorkspace workspace = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + assertCanView(workspace); + return workspace; + } + + public List listMembers(UUID workspaceId) { + DashboardWorkspace workspace = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + assertCanView(workspace); + return memberRepository.findByWorkspaceIdOrderByUsernameAsc(workspaceId); + } + + /** Dashboards in a workspace the caller can see. */ + public List listDashboards(UUID workspaceId) { + DashboardWorkspace workspace = getWorkspace(workspaceId); + return savedDashboardRepository.findByWorkspaceIdOrderByUpdatedAtDesc(workspace.getId()); + } + + // ==================== Mutations ==================== + + @Transactional + public DashboardWorkspace createWorkspace(String connectionId, String name, String description, String color) { + accessControlService.assertCanReadConnectionContent(connectionId); + String cleanName = requireName(name); + + workspaceRepository.findByConnectionIdAndNameIgnoreCase(connectionId, cleanName).ifPresent(existing -> { + throw new ResponseStatusException(org.springframework.http.HttpStatus.CONFLICT, + "A workspace named \"" + cleanName + "\" already exists on this connection"); + }); + + String creator = accessControlService.requireCurrentUsername(); + + DashboardWorkspace workspace = new DashboardWorkspace(); + workspace.setConnectionId(connectionId); + workspace.setName(cleanName); + workspace.setDescription(trimToNull(description)); + workspace.setColor(trimToNull(color)); + workspace.setCreatedBy(creator); + DashboardWorkspace saved = workspaceRepository.save(workspace); + + // The creator is a MANAGER member outright rather than relying on a + // createdBy check at read time, so ownership survives if the workspace is + // later handed to someone else. + DashboardWorkspaceMember owner = new DashboardWorkspaceMember(); + owner.setWorkspaceId(saved.getId()); + owner.setUsername(creator); + owner.setWorkspaceRole(DashboardWorkspaceRole.MANAGER); + owner.setAddedBy(creator); + memberRepository.save(owner); + + log.info("Dashboard workspace created: id={}, connection={}, by={}", saved.getId(), connectionId, creator); + return saved; + } + + @Transactional + public DashboardWorkspace updateWorkspace(UUID workspaceId, String name, String description, String color) { + DashboardWorkspace workspace = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + assertCanManage(workspace); + + if (name != null && !name.isBlank()) { + String cleanName = requireName(name); + workspaceRepository.findByConnectionIdAndNameIgnoreCase(workspace.getConnectionId(), cleanName) + .filter(other -> !other.getId().equals(workspaceId)) + .ifPresent(other -> { + throw new ResponseStatusException(org.springframework.http.HttpStatus.CONFLICT, + "A workspace named \"" + cleanName + "\" already exists on this connection"); + }); + workspace.setName(cleanName); + } + // Null means "field omitted"; blank is the explicit clear signal, matching the + // convention updateDashboard/setSharePassword already use. + if (description != null) { + workspace.setDescription(trimToNull(description)); + } + if (color != null) { + workspace.setColor(trimToNull(color)); + } + return workspaceRepository.save(workspace); + } + + @Transactional + public void deleteWorkspace(UUID workspaceId) { + DashboardWorkspace workspace = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + assertCanManage(workspace); + + // Dashboards outlive their workspace: detach them rather than cascading a delete. + // Deleting a grouping must never destroy the things grouped. + List dashboards = savedDashboardRepository.findByWorkspaceIdOrderByUpdatedAtDesc(workspaceId); + for (SavedDashboard dashboard : dashboards) { + dashboard.setWorkspaceId(null); + } + if (!dashboards.isEmpty()) { + savedDashboardRepository.saveAll(dashboards); + } + + memberRepository.deleteByWorkspaceId(workspaceId); + workspaceRepository.delete(workspace); + log.info("Dashboard workspace deleted: id={}, detached {} dashboard(s)", workspaceId, dashboards.size()); + } + + @Transactional + public DashboardWorkspaceMember addMember(UUID workspaceId, String username, String workspaceRole) { + DashboardWorkspace workspace = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + assertCanManage(workspace); + + String cleanUsername = username == null ? "" : username.trim(); + if (cleanUsername.isEmpty()) { + throw new ResponseStatusException(org.springframework.http.HttpStatus.BAD_REQUEST, "Username is required"); + } + + DashboardWorkspaceMember member = memberRepository + .findByWorkspaceIdAndUsernameIgnoreCase(workspaceId, cleanUsername) + .orElseGet(() -> { + DashboardWorkspaceMember fresh = new DashboardWorkspaceMember(); + fresh.setWorkspaceId(workspaceId); + fresh.setUsername(cleanUsername); + return fresh; + }); + member.setWorkspaceRole(DashboardWorkspaceRole.fromString(workspaceRole)); + member.setAddedBy(accessControlService.requireCurrentUsername()); + return memberRepository.save(member); + } + + @Transactional + public void removeMember(UUID workspaceId, String username) { + DashboardWorkspace workspace = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + assertCanManage(workspace); + + List members = memberRepository.findByWorkspaceIdOrderByUsernameAsc(workspaceId); + boolean removingLastManager = members.stream() + .filter(m -> m.getWorkspaceRole() == DashboardWorkspaceRole.MANAGER) + .allMatch(m -> m.getUsername().equalsIgnoreCase(username)); + boolean anyManager = members.stream().anyMatch(m -> m.getWorkspaceRole() == DashboardWorkspaceRole.MANAGER); + if (anyManager && removingLastManager) { + // A workspace with no manager can never be changed again by anyone but an + // admin — refuse rather than create that dead end. + throw new ResponseStatusException(org.springframework.http.HttpStatus.CONFLICT, + "Cannot remove the last manager. Promote another member first."); + } + memberRepository.deleteByWorkspaceIdAndUsernameIgnoreCase(workspaceId, username); + } + + /** + * Move a dashboard into a workspace, or out of one when {@code workspaceId} is null. + * + *

Both ends are checked: the caller must be able to manage the dashboard's + * connection content, and must be able to manage the destination workspace. Without + * the second check, anyone could push a dashboard into a workspace they cannot see. + */ + @Transactional + public SavedDashboard moveDashboard(UUID dashboardId, UUID workspaceId) { + SavedDashboard dashboard = savedDashboardRepository.findById(dashboardId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Dashboard not found")); + accessControlService.assertCanManageConnectionContent(dashboard.getConnectionId()); + assertCanReadDashboard(dashboard); + + if (workspaceId == null) { + dashboard.setWorkspaceId(null); + return savedDashboardRepository.save(dashboard); + } + + DashboardWorkspace target = workspaceRepository.findById(workspaceId) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + if (!target.getConnectionId().equals(dashboard.getConnectionId())) { + throw new ResponseStatusException(org.springframework.http.HttpStatus.BAD_REQUEST, + "Workspace belongs to a different connection"); + } + assertCanManage(target); + + dashboard.setWorkspaceId(workspaceId); + return savedDashboardRepository.save(dashboard); + } + + // ==================== Access checks ==================== + + /** True when the caller may read a dashboard given its workspace, if any. */ + public boolean canReadDashboard(SavedDashboard dashboard) { + if (dashboard == null) { + return false; + } + UUID workspaceId = dashboard.getWorkspaceId(); + if (workspaceId == null) { + return true; + } + if (accessControlService.isCurrentUserAdmin()) { + return true; + } + return memberRepository + .findByWorkspaceIdAndUsernameIgnoreCase(workspaceId, currentUsername()) + .isPresent(); + } + + /** + * Assert workspace membership for a dashboard the caller already passed the + * connection check on. Reports 404, not 403: a user outside the workspace should not + * learn that a dashboard with that id exists. + */ + public void assertCanReadDashboard(SavedDashboard dashboard) { + if (!canReadDashboard(dashboard)) { + throw new ResponseStatusException(NOT_FOUND, "Dashboard not found"); + } + } + + /** Filter a dashboard list to what the caller may see, in one membership query. */ + public List filterReadable(List dashboards) { + if (dashboards == null || dashboards.isEmpty()) { + return List.of(); + } + if (accessControlService.isCurrentUserAdmin()) { + return dashboards; + } + Set workspaceIds = dashboards.stream() + .map(SavedDashboard::getWorkspaceId) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); + if (workspaceIds.isEmpty()) { + return dashboards; + } + Set memberOf = memberRepository + .findByUsernameIgnoreCaseAndWorkspaceIdIn(currentUsername(), workspaceIds) + .stream() + .map(DashboardWorkspaceMember::getWorkspaceId) + .collect(Collectors.toSet()); + return dashboards.stream() + .filter(d -> d.getWorkspaceId() == null || memberOf.contains(d.getWorkspaceId())) + .collect(Collectors.toList()); + } + + private void assertCanView(DashboardWorkspace workspace) { + accessControlService.assertCanReadConnectionContent(workspace.getConnectionId()); + if (accessControlService.isCurrentUserAdmin()) { + return; + } + memberRepository.findByWorkspaceIdAndUsernameIgnoreCase(workspace.getId(), currentUsername()) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + } + + private void assertCanManage(DashboardWorkspace workspace) { + accessControlService.assertCanReadConnectionContent(workspace.getConnectionId()); + if (accessControlService.isCurrentUserAdmin()) { + return; + } + DashboardWorkspaceMember member = memberRepository + .findByWorkspaceIdAndUsernameIgnoreCase(workspace.getId(), currentUsername()) + .orElseThrow(() -> new ResponseStatusException(NOT_FOUND, "Workspace not found")); + if (!member.getWorkspaceRole().canManage()) { + throw new ResponseStatusException(FORBIDDEN, "Only a workspace manager can change this workspace"); + } + } + + private Set memberWorkspaceIds(String username, List candidates) { + if (candidates.isEmpty()) { + return Set.of(); + } + Set ids = candidates.stream().map(DashboardWorkspace::getId).collect(Collectors.toSet()); + return memberRepository.findByUsernameIgnoreCaseAndWorkspaceIdIn(username, ids).stream() + .map(DashboardWorkspaceMember::getWorkspaceId) + .collect(Collectors.toSet()); + } + + private String currentUsername() { + return accessControlService.requireCurrentUsername(); + } + + private static String requireName(String name) { + String clean = name == null ? "" : name.trim(); + if (clean.isEmpty()) { + throw new ResponseStatusException(org.springframework.http.HttpStatus.BAD_REQUEST, + "Workspace name is required"); + } + if (clean.length() > 128) { + throw new ResponseStatusException(org.springframework.http.HttpStatus.BAD_REQUEST, + "Workspace name must be 128 characters or fewer"); + } + return clean; + } + + private static String trimToNull(String value) { + if (value == null) return null; + String trimmed = value.trim(); + return trimmed.isEmpty() ? null : trimmed; + } + + /** Whether the caller sees every workspace, and if not, which ones they belong to. */ + public record WorkspaceVisibility(boolean seesAll, Set memberWorkspaceIds) { + public boolean canSee(UUID workspaceId) { + return workspaceId == null || seesAll || memberWorkspaceIds.contains(workspaceId); + } + } +} diff --git a/backend/src/main/resources/db/migration/V117__create_dashboard_workspaces_and_custom_roles.sql b/backend/src/main/resources/db/migration/V117__create_dashboard_workspaces_and_custom_roles.sql new file mode 100644 index 0000000..dc67cab --- /dev/null +++ b/backend/src/main/resources/db/migration/V117__create_dashboard_workspaces_and_custom_roles.sql @@ -0,0 +1,91 @@ +-- Dashboard workspaces + custom roles. +-- +-- NOTE: this repository has no Flyway runtime (see CLAUDE.md). Schema is managed by +-- spring.jpa.hibernate.ddl-auto=update, which creates these tables and the new +-- saved_dashboards.workspace_id column from the JPA entities on startup. This file is +-- the hand-maintained changelog: apply it with psql only if you need the schema without +-- letting Hibernate touch the database. + +-- ── Dashboard workspaces ───────────────────────────────────────────────────── +CREATE TABLE IF NOT EXISTS dashboard_workspaces ( + id UUID PRIMARY KEY, + connection_id VARCHAR(255) NOT NULL, + name VARCHAR(128) NOT NULL, + description VARCHAR(500), + color VARCHAR(32), + created_by VARCHAR(255) NOT NULL, + created_at TIMESTAMP NOT NULL DEFAULT NOW(), + updated_at TIMESTAMP NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_dashboard_workspaces_connection + ON dashboard_workspaces (connection_id); + +-- One workspace name per connection, so the folder chips stay unambiguous. +CREATE UNIQUE INDEX IF NOT EXISTS ux_dashboard_workspaces_conn_name + ON dashboard_workspaces (connection_id, LOWER(name)); + +-- ── Membership ─────────────────────────────────────────────────────────────── +-- Keyed by username to match connection_access_grant, so an impersonated ("View as") +-- session resolves membership as the target user with no extra lookup. +CREATE TABLE IF NOT EXISTS dashboard_workspace_members ( + id UUID PRIMARY KEY, + workspace_id UUID NOT NULL REFERENCES dashboard_workspaces (id) ON DELETE CASCADE, + username VARCHAR(255) NOT NULL, + workspace_role VARCHAR(32) NOT NULL DEFAULT 'VIEWER', + added_by VARCHAR(255), + created_at TIMESTAMP NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_dashboard_ws_members_workspace + ON dashboard_workspace_members (workspace_id); +CREATE INDEX IF NOT EXISTS idx_dashboard_ws_members_username + ON dashboard_workspace_members (username); +CREATE UNIQUE INDEX IF NOT EXISTS ux_dashboard_ws_member + ON dashboard_workspace_members (workspace_id, LOWER(username)); + +-- ── Dashboards join a workspace ────────────────────────────────────────────── +-- Nullable on purpose: NULL means "not grouped", governed purely by the connection ACL +-- exactly as every dashboard was before workspaces existed. Deliberately NOT a cascading +-- FK — deleting a workspace detaches its dashboards rather than destroying them +-- (DashboardWorkspaceService.deleteWorkspace). +ALTER TABLE saved_dashboards + ADD COLUMN IF NOT EXISTS workspace_id UUID; + +CREATE INDEX IF NOT EXISTS idx_saved_dashboards_workspace_id + ON saved_dashboards (workspace_id); + +-- ── Custom roles ───────────────────────────────────────────────────────────── +-- code shares a namespace with the built-in Role names because both are written to +-- users.role; CustomRoleService refuses a code that collides with a built-in one. +CREATE TABLE IF NOT EXISTS custom_roles ( + id BIGSERIAL PRIMARY KEY, + code VARCHAR(64) NOT NULL, + name VARCHAR(128) NOT NULL, + description VARCHAR(500), + permission_codes TEXT, + created_at TIMESTAMP NOT NULL DEFAULT NOW(), + updated_at TIMESTAMP NOT NULL DEFAULT NOW(), + created_by VARCHAR(255), + updated_by VARCHAR(255) +); + +CREATE UNIQUE INDEX IF NOT EXISTS ux_custom_roles_code ON custom_roles (UPPER(code)); + +-- ── role_permission_overrides widens from the old Role/Permission enums ────── +-- The column is already a string; this only relaxes any length limit so a custom role +-- code fits. Existing rows ('ADMIN', 'DEVELOPER') keep working untouched. +ALTER TABLE role_permission_overrides + ALTER COLUMN role TYPE VARCHAR(64); + +-- Hibernate generated CHECK constraints from the ORIGINAL two-role / 20-permission +-- enums and ddl-auto=update never drops a stale constraint. Left in place they reject +-- every new value: inserting ('DBA','VIEW_AGENT') fails with +-- violates check constraint "role_permission_overrides_permission_code_check" +-- (observed on a live install, not inferred). Dropping them is what lets an override be +-- recorded for DBA/DATA_ENGINEER or a custom role. The application enum is the +-- authority for these values; SchemaConstraintRefreshInitializer applies this at boot. +ALTER TABLE role_permission_overrides + DROP CONSTRAINT IF EXISTS role_permission_overrides_role_check; +ALTER TABLE role_permission_overrides + DROP CONSTRAINT IF EXISTS role_permission_overrides_permission_code_check; diff --git a/backend/src/test/java/com/dbaagent/service/DashboardWorkspaceAccessTest.java b/backend/src/test/java/com/dbaagent/service/DashboardWorkspaceAccessTest.java new file mode 100644 index 0000000..f453e7b --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/DashboardWorkspaceAccessTest.java @@ -0,0 +1,212 @@ +package com.dbaagent.service; + +import com.dbaagent.model.DashboardWorkspace; +import com.dbaagent.model.DashboardWorkspaceMember; +import com.dbaagent.model.DashboardWorkspaceRole; +import com.dbaagent.model.SavedDashboard; +import com.dbaagent.repository.DashboardWorkspaceMemberRepository; +import com.dbaagent.repository.DashboardWorkspaceRepository; +import com.dbaagent.repository.SavedDashboardRepository; +import com.dbaagent.service.security.AccessControlService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.web.server.ResponseStatusException; + +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.*; + +/** + * The workspace access rule: connection access AND workspace membership. + * + *

The load-bearing property is that membership can only ever narrow access. + * A dashboard with no workspace must stay visible to everyone who could already see it, + * and a dashboard in a workspace must be invisible to a non-member even though that user + * still has full read access to the connection. + */ +class DashboardWorkspaceAccessTest { + + private static final String CONNECTION = "conn-1"; + private static final UUID WORKSPACE = UUID.randomUUID(); + + private DashboardWorkspaceRepository workspaceRepository; + private DashboardWorkspaceMemberRepository memberRepository; + private SavedDashboardRepository savedDashboardRepository; + private AccessControlService accessControlService; + private DashboardWorkspaceService service; + + @BeforeEach + void setUp() { + workspaceRepository = mock(DashboardWorkspaceRepository.class); + memberRepository = mock(DashboardWorkspaceMemberRepository.class); + savedDashboardRepository = mock(SavedDashboardRepository.class); + accessControlService = mock(AccessControlService.class); + + when(accessControlService.requireCurrentUsername()).thenReturn("analyst"); + when(accessControlService.isCurrentUserAdmin()).thenReturn(false); + + service = new DashboardWorkspaceService( + workspaceRepository, memberRepository, savedDashboardRepository, accessControlService); + } + + private static SavedDashboard dashboard(UUID workspaceId) { + SavedDashboard d = new SavedDashboard(); + d.setId(UUID.randomUUID()); + d.setConnectionId(CONNECTION); + d.setWorkspaceId(workspaceId); + return d; + } + + @Test + @DisplayName("A dashboard with no workspace stays readable — workspaces never widen or narrow the ungrouped case") + void ungroupedDashboardIsReadable() { + assertThat(service.canReadDashboard(dashboard(null))).isTrue(); + } + + @Test + @DisplayName("A non-member cannot read a dashboard inside a workspace, despite connection access") + void nonMemberCannotReadGroupedDashboard() { + when(memberRepository.findByWorkspaceIdAndUsernameIgnoreCase(WORKSPACE, "analyst")) + .thenReturn(Optional.empty()); + + SavedDashboard grouped = dashboard(WORKSPACE); + + assertThat(service.canReadDashboard(grouped)).isFalse(); + // 404, not 403: a non-member must not learn the dashboard exists. + assertThatThrownBy(() -> service.assertCanReadDashboard(grouped)) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("404"); + } + + @Test + @DisplayName("A member can read a dashboard inside their workspace") + void memberCanReadGroupedDashboard() { + DashboardWorkspaceMember member = new DashboardWorkspaceMember(); + member.setWorkspaceId(WORKSPACE); + member.setUsername("analyst"); + member.setWorkspaceRole(DashboardWorkspaceRole.VIEWER); + when(memberRepository.findByWorkspaceIdAndUsernameIgnoreCase(WORKSPACE, "analyst")) + .thenReturn(Optional.of(member)); + + assertThat(service.canReadDashboard(dashboard(WORKSPACE))).isTrue(); + } + + @Test + @DisplayName("An admin reads every workspace without a membership row") + void adminBypassesMembership() { + when(accessControlService.isCurrentUserAdmin()).thenReturn(true); + + assertThat(service.canReadDashboard(dashboard(WORKSPACE))).isTrue(); + verify(memberRepository, never()).findByWorkspaceIdAndUsernameIgnoreCase(any(), anyString()); + } + + @Test + @DisplayName("filterReadable keeps ungrouped dashboards and drops non-member workspaces") + void filterReadableSplitsCorrectly() { + UUID otherWorkspace = UUID.randomUUID(); + SavedDashboard ungrouped = dashboard(null); + SavedDashboard mine = dashboard(WORKSPACE); + SavedDashboard theirs = dashboard(otherWorkspace); + + DashboardWorkspaceMember member = new DashboardWorkspaceMember(); + member.setWorkspaceId(WORKSPACE); + member.setUsername("analyst"); + when(memberRepository.findByUsernameIgnoreCaseAndWorkspaceIdIn(eq("analyst"), any())) + .thenReturn(List.of(member)); + + List visible = service.filterReadable(List.of(ungrouped, mine, theirs)); + + assertThat(visible).containsExactly(ungrouped, mine); + assertThat(visible).doesNotContain(theirs); + } + + @Test + @DisplayName("A viewer cannot rename the workspace; only a manager can") + void viewerCannotManageWorkspace() { + DashboardWorkspace workspace = new DashboardWorkspace(); + workspace.setId(WORKSPACE); + workspace.setConnectionId(CONNECTION); + workspace.setName("Finance"); + when(workspaceRepository.findById(WORKSPACE)).thenReturn(Optional.of(workspace)); + + DashboardWorkspaceMember viewer = new DashboardWorkspaceMember(); + viewer.setWorkspaceId(WORKSPACE); + viewer.setUsername("analyst"); + viewer.setWorkspaceRole(DashboardWorkspaceRole.VIEWER); + when(memberRepository.findByWorkspaceIdAndUsernameIgnoreCase(WORKSPACE, "analyst")) + .thenReturn(Optional.of(viewer)); + + assertThatThrownBy(() -> service.updateWorkspace(WORKSPACE, "Renamed", null, null)) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("403"); + } + + @Test + @DisplayName("Deleting a workspace detaches its dashboards instead of deleting them") + void deleteDetachesDashboards() { + when(accessControlService.isCurrentUserAdmin()).thenReturn(true); + DashboardWorkspace workspace = new DashboardWorkspace(); + workspace.setId(WORKSPACE); + workspace.setConnectionId(CONNECTION); + when(workspaceRepository.findById(WORKSPACE)).thenReturn(Optional.of(workspace)); + + SavedDashboard grouped = dashboard(WORKSPACE); + when(savedDashboardRepository.findByWorkspaceIdOrderByUpdatedAtDesc(WORKSPACE)) + .thenReturn(List.of(grouped)); + + service.deleteWorkspace(WORKSPACE); + + // The dashboard survives, merely ungrouped. Deleting a grouping must never + // destroy the things grouped. + assertThat(grouped.getWorkspaceId()).isNull(); + verify(savedDashboardRepository).saveAll(any()); + verify(savedDashboardRepository, never()).delete(any()); + verify(workspaceRepository).delete(workspace); + } + + @Test + @DisplayName("Removing the last manager is refused so the workspace cannot be orphaned") + void cannotRemoveLastManager() { + when(accessControlService.isCurrentUserAdmin()).thenReturn(true); + DashboardWorkspace workspace = new DashboardWorkspace(); + workspace.setId(WORKSPACE); + workspace.setConnectionId(CONNECTION); + when(workspaceRepository.findById(WORKSPACE)).thenReturn(Optional.of(workspace)); + + DashboardWorkspaceMember onlyManager = new DashboardWorkspaceMember(); + onlyManager.setWorkspaceId(WORKSPACE); + onlyManager.setUsername("owner"); + onlyManager.setWorkspaceRole(DashboardWorkspaceRole.MANAGER); + when(memberRepository.findByWorkspaceIdOrderByUsernameAsc(WORKSPACE)) + .thenReturn(List.of(onlyManager)); + + assertThatThrownBy(() -> service.removeMember(WORKSPACE, "owner")) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("409"); + } + + @Test + @DisplayName("A dashboard cannot be moved into a workspace on a different connection") + void cannotMoveAcrossConnections() { + when(accessControlService.isCurrentUserAdmin()).thenReturn(true); + SavedDashboard d = dashboard(null); + when(savedDashboardRepository.findById(d.getId())).thenReturn(Optional.of(d)); + + DashboardWorkspace foreign = new DashboardWorkspace(); + foreign.setId(WORKSPACE); + foreign.setConnectionId("some-other-connection"); + when(workspaceRepository.findById(WORKSPACE)).thenReturn(Optional.of(foreign)); + + assertThatThrownBy(() -> service.moveDashboard(d.getId(), WORKSPACE)) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("400"); + } +} diff --git a/src/components/sections/DashboardsHome.jsx b/src/components/sections/DashboardsHome.jsx index 6b48bc0..9f27bd4 100644 --- a/src/components/sections/DashboardsHome.jsx +++ b/src/components/sections/DashboardsHome.jsx @@ -1,6 +1,8 @@ import { useState, useEffect, useCallback, useRef, useMemo } from 'react' -import { Plus, Sparkles, Clock, RefreshCw, Trash2, Loader2, LayoutDashboard, AlertTriangle, Search, Star, Copy, Folder, X } from 'lucide-react' +import { Plus, Sparkles, Clock, RefreshCw, Trash2, Loader2, LayoutDashboard, AlertTriangle, Search, Star, Copy, Folder, X, Users, Settings2 } from 'lucide-react' import { savedDashboardsAPI } from '@/lib/api/client' +import { dashboardWorkspacesAPI } from '@/lib/api/client' +import WorkspaceManager from './WorkspaceManager' import styles from './DashboardsHome.module.css' // A dashboard is "live" once it's published to the web (has a public link). @@ -74,11 +76,17 @@ export default function DashboardsHome({ connectionId, onOpen }) { // some embedded/webview hosts suppress it outright, silently no-opping the delete). const [confirmId, setConfirmId] = useState(null) const [deleteError, setDeleteError] = useState(null) + const [workspaces, setWorkspaces] = useState([]) + const [workspaceFilter, setWorkspaceFilter] = useState(null) // null = all workspaces + const [showWorkspaces, setShowWorkspaces] = useState(false) + const [workspaceMenuId, setWorkspaceMenuId] = useState(null) + const [movingWorkspaceId, setMovingWorkspaceId] = useState(null) const [folderMenuId, setFolderMenuId] = useState(null) const [folderInput, setFolderInput] = useState('') const [movingFolderId, setMovingFolderId] = useState(null) const confirmRef = useRef(null) const folderMenuRef = useRef(null) + const workspaceMenuRef = useRef(null) useEffect(() => { if (!folderMenuId) return @@ -89,6 +97,15 @@ export default function DashboardsHome({ connectionId, onOpen }) { return () => { document.removeEventListener('mousedown', onDocClick); document.removeEventListener('keydown', onKey) } }, [folderMenuId]) + useEffect(() => { + if (!workspaceMenuId) return + const onDocClick = (e) => { if (workspaceMenuRef.current && !workspaceMenuRef.current.contains(e.target)) setWorkspaceMenuId(null) } + const onKey = (e) => { if (e.key === 'Escape') setWorkspaceMenuId(null) } + document.addEventListener('mousedown', onDocClick) + document.addEventListener('keydown', onKey) + return () => { document.removeEventListener('mousedown', onDocClick); document.removeEventListener('keydown', onKey) } + }, [workspaceMenuId]) + useEffect(() => { if (!confirmId) return const onDocClick = (e) => { if (confirmRef.current && !confirmRef.current.contains(e.target)) setConfirmId(null) } @@ -162,6 +179,36 @@ export default function DashboardsHome({ connectionId, onOpen }) { } }, [movingFolderId]) + const loadWorkspaces = useCallback(async () => { + if (!connectionId) return + try { + const list = await dashboardWorkspacesAPI.listByConnection(connectionId) + setWorkspaces(Array.isArray(list) ? list : []) + } catch { + // A user with no workspace membership simply has none to show; the + // dashboards list itself is already filtered server-side. + setWorkspaces([]) + } + }, [connectionId]) + + // Move a dashboard into a workspace, or out of one with an empty workspaceId. + const moveToWorkspace = useCallback(async (d, workspaceId) => { + if (movingWorkspaceId) return + setMovingWorkspaceId(d.id) + try { + await dashboardWorkspacesAPI.moveDashboard(d.id, workspaceId || '') + setDashboards((list) => list.map((x) => ( + x.id === d.id ? { ...x, workspaceId: workspaceId || null } : x + ))) + setWorkspaceMenuId(null) + loadWorkspaces() + } catch (err) { + setDeleteError(err?.response?.data?.message || err?.message || 'Couldn’t move this dashboard.') + } finally { + setMovingWorkspaceId(null) + } + }, [movingWorkspaceId, loadWorkspaces]) + const load = useCallback(async () => { setLoading(true) try { @@ -180,6 +227,9 @@ export default function DashboardsHome({ connectionId, onOpen }) { // the new fetch is in flight. useEffect(() => { setHasLoadedOnce(false); setLoading(true) }, [connectionId]) useEffect(() => { load() }, [load]) + useEffect(() => { loadWorkspaces() }, [loadWorkspaces]) + // A workspace filter from a previous connection must not survive the switch. + useEffect(() => { setWorkspaceFilter(null) }, [connectionId]) const folders = useMemo(() => { const set = new Set(dashboards.map((d) => d.folder).filter(Boolean)) @@ -191,10 +241,11 @@ export default function DashboardsHome({ connectionId, onOpen }) { return dashboards.filter((d) => { if (filter !== 'all' && statusOf(d) !== filter) return false if (folder && d.folder !== folder) return false + if (workspaceFilter && d.workspaceId !== workspaceFilter) return false if (q && !(d.name || '').toLowerCase().includes(q) && !(d.description || '').toLowerCase().includes(q)) return false return true }) - }, [dashboards, filter, folder, search]) + }, [dashboards, filter, folder, search, workspaceFilter]) return (

@@ -206,6 +257,14 @@ export default function DashboardsHome({ connectionId, onOpen }) { )}
+ @@ -254,6 +313,28 @@ export default function DashboardsHome({ connectionId, onOpen }) { {f} ))} + {workspaces.length > 0 && } + {workspaces.map((w) => ( + + ))}
{!hasLoadedOnce ? ( @@ -335,6 +416,50 @@ export default function DashboardsHome({ connectionId, onOpen }) { )} + + {workspaceMenuId === d.id && ( +
e.stopPropagation()}> +

Move to workspace

+ {workspaces.length === 0 ? ( +

+ No workspaces yet — create one from the Workspaces button above. +

+ ) : ( +
+ + {workspaces.map((w) => ( + + ))} +
+ )} +
+ )} + + + {error && ( +
+ + {error} +
+ )} + +
+
+
+ setNewName(e.target.value)} + onKeyDown={(e) => { if (e.key === 'Enter') create() }} + /> + setNewDescription(e.target.value)} + /> +
+ {COLORS.map((c) => ( + +
+
+ + {loading ? ( +
Loading…
+ ) : workspaces.length === 0 ? ( +
No workspaces yet. Create one above.
+ ) : ( +
    + {workspaces.map((w) => ( +
  • + +
    • + ))} +
+ )} +
+ +
+ {!selected ? ( +
+ + Select a workspace to manage its members. +
+ ) : ( + <> +
+
+

{selected.name}

+ {selected.description &&

{selected.description}

} +

Created by {selected.createdBy}

+
+ {confirmDeleteId === selected.id ? ( +
+ Delete? Dashboards stay, ungrouped. + + +
+ ) : ( + + )} +
+ +
+ {isAdmin && users.length > 0 ? ( + + ) : ( + setAddUsername(e.target.value)} + onKeyDown={(e) => { if (e.key === 'Enter') addMember() }} + /> + )} + + +
+ + {membersLoading ? ( +
Loading members…
+ ) : ( +
    + {members.map((m) => ( +
  • + + {m.workspaceRole === 'MANAGER' ? : } + + + {m.username} + {m.username?.toLowerCase() === username?.toLowerCase() && ( + you + )} + + + {m.workspaceRole === 'MANAGER' ? 'Manager' : 'Viewer'} + + +
    • + ))} + {members.length === 0 && ( +
  • No members yet — only admins can see this workspace.
    • + )} +
+ )} + + )} +
+
+ + + ) +} diff --git a/src/components/sections/WorkspaceManager.module.css b/src/components/sections/WorkspaceManager.module.css new file mode 100644 index 0000000..a9158bd --- /dev/null +++ b/src/components/sections/WorkspaceManager.module.css @@ -0,0 +1,366 @@ +.backdrop { + position: fixed; + inset: 0; + background: rgba(15, 15, 17, 0.45); + display: flex; + align-items: center; + justify-content: center; + z-index: 60; + padding: 24px; +} + +.modal { + width: min(880px, 100%); + max-height: min(86vh, 720px); + display: flex; + flex-direction: column; + background: #fff; + border: 1px solid #e5e5e5; + border-radius: 12px; + box-shadow: 0 18px 48px rgba(0, 0, 0, 0.18); + overflow: hidden; +} + +.header { + display: flex; + align-items: flex-start; + justify-content: space-between; + gap: 16px; + padding: 18px 20px 14px; + border-bottom: 1px solid #f0f0f0; +} + +.title { + margin: 0; + font-size: 15px; + font-weight: 600; + color: #111; + letter-spacing: -0.01em; +} + +.subtitle { + margin: 4px 0 0; + font-size: 12px; + color: #6b7280; + line-height: 1.5; + max-width: 60ch; +} + +.body { + display: grid; + grid-template-columns: minmax(240px, 320px) 1fr; + min-height: 0; + flex: 1; +} + +.listPane { + border-right: 1px solid #f0f0f0; + padding: 14px; + overflow-y: auto; + display: flex; + flex-direction: column; + gap: 12px; +} + +.detailPane { + padding: 16px 18px; + overflow-y: auto; + display: flex; + flex-direction: column; + gap: 14px; +} + +.createBox { + display: flex; + flex-direction: column; + gap: 7px; + padding: 12px; + border: 1px solid #eee; + border-radius: 9px; + background: #fafafa; +} + +.input, +.roleSelect { + width: 100%; + padding: 7px 9px; + font-size: 12.5px; + color: #111; + background: #fff; + border: 1px solid #e0e0e0; + border-radius: 7px; + outline: none; + transition: border-color 0.15s ease; +} + +.input:focus, +.roleSelect:focus { + border-color: #534ab7; +} + +.roleSelect { + width: auto; + min-width: 108px; +} + +.colorRow { + display: flex; + align-items: center; + gap: 6px; + flex-wrap: wrap; +} + +.swatch { + width: 18px; + height: 18px; + border-radius: 50%; + border: 2px solid transparent; + cursor: pointer; + padding: 0; + transition: transform 0.12s ease, border-color 0.12s ease; +} + +.swatch:hover { transform: scale(1.12); } +.swatchActive { border-color: #111; } + +.createBtn { + margin-left: auto; + display: inline-flex; + align-items: center; + gap: 5px; + padding: 6px 11px; + font-size: 12px; + font-weight: 500; + color: #fff; + background: #111; + border: 1px solid #111; + border-radius: 7px; + cursor: pointer; + transition: opacity 0.15s ease; +} + +.createBtn:disabled { opacity: 0.45; cursor: not-allowed; } + +.list { + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: 3px; +} + +.listItem { + width: 100%; + display: flex; + align-items: center; + gap: 8px; + padding: 8px 9px; + font-size: 12.5px; + color: #333; + background: transparent; + border: 1px solid transparent; + border-radius: 7px; + cursor: pointer; + text-align: left; + transition: background 0.12s ease; +} + +.listItem:hover { background: #f6f6f6; } + +.listItemActive { + background: #f2f1fb; + border-color: #ddd9f5; + color: #111; + font-weight: 500; +} + +.dot { + width: 9px; + height: 9px; + border-radius: 50%; + flex-shrink: 0; +} + +.listName { + flex: 1; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.listCount { + font-size: 11px; + color: #9ca3af; + font-variant-numeric: tabular-nums; +} + +.detailHead { + display: flex; + align-items: flex-start; + justify-content: space-between; + gap: 12px; +} + +.detailTitle { + margin: 0; + font-size: 14px; + font-weight: 600; + color: #111; +} + +.detailSub { + margin: 3px 0 0; + font-size: 12px; + color: #6b7280; +} + +.detailMeta { + margin: 5px 0 0; + font-size: 11px; + color: #9ca3af; +} + +.addMemberRow { + display: flex; + align-items: center; + gap: 7px; + padding-bottom: 12px; + border-bottom: 1px solid #f0f0f0; +} + +.memberList { + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: 2px; +} + +.memberRow { + display: flex; + align-items: center; + gap: 9px; + padding: 7px 6px; + border-radius: 7px; + font-size: 12.5px; + color: #333; +} + +.memberRow:hover { background: #fafafa; } + +.memberIcon { + display: inline-flex; + color: #6b7280; +} + +.memberName { + flex: 1; + display: inline-flex; + align-items: center; + gap: 6px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.youTag { + font-size: 10px; + text-transform: uppercase; + letter-spacing: 0.04em; + color: #6b7280; + background: #f0f0f0; + border-radius: 4px; + padding: 1px 5px; +} + +.memberRole { + font-size: 11.5px; + color: #6b7280; +} + +.iconBtn { + display: inline-flex; + align-items: center; + justify-content: center; + width: 26px; + height: 26px; + color: #6b7280; + background: transparent; + border: 1px solid transparent; + border-radius: 6px; + cursor: pointer; + transition: background 0.12s ease, color 0.12s ease; +} + +.iconBtn:hover { background: #f2f2f2; color: #111; } +.iconBtn:disabled { opacity: 0.5; cursor: not-allowed; } + +.ghostBtn { + padding: 4px 9px; + font-size: 11.5px; + color: #444; + background: #fff; + border: 1px solid #e0e0e0; + border-radius: 6px; + cursor: pointer; +} + +.dangerBtn { + padding: 4px 10px; + font-size: 11.5px; + color: #fff; + background: #b91c1c; + border: 1px solid #b91c1c; + border-radius: 6px; + cursor: pointer; +} + +.confirmRow { + display: flex; + align-items: center; + gap: 7px; + flex-wrap: wrap; + justify-content: flex-end; +} + +.confirmText { + font-size: 11.5px; + color: #6b7280; +} + +.placeholder { + display: flex; + align-items: center; + gap: 8px; + padding: 18px 10px; + font-size: 12.5px; + color: #9ca3af; + justify-content: center; + text-align: center; +} + +.error { + display: flex; + align-items: center; + gap: 8px; + margin: 12px 20px 0; + padding: 8px 11px; + font-size: 12px; + color: #991b1b; + background: #fef2f2; + border: 1px solid #fecaca; + border-radius: 7px; +} + +.spin { animation: wm-spin 0.9s linear infinite; } + +@keyframes wm-spin { + to { transform: rotate(360deg); } +} + +@media (max-width: 720px) { + .body { grid-template-columns: 1fr; } + .listPane { border-right: none; border-bottom: 1px solid #f0f0f0; } +} From 3257403d0030a873c6758a3bbbc1d010e7682814 Mon Sep 17 00:00:00 2001 From: sumit Date: Sun, 23 Aug 2026 15:54:41 +0530 Subject: [PATCH 3/5] refactor(access): collapse connection access levels to a single tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../dbaagent/model/ConnectionAccessLevel.java | 22 ++- .../security/ConnectionAccessService.java | 8 +- .../security/AccessControlServiceTest.java | 10 ++ .../ConnectionAccessLevelCollapseTest.java | 131 ++++++++++++++++++ src/components/tabs/admin/UsersTab.jsx | 97 ++++++++----- 5 files changed, 224 insertions(+), 44 deletions(-) create mode 100644 backend/src/test/java/com/dbaagent/service/security/ConnectionAccessLevelCollapseTest.java diff --git a/backend/src/main/java/com/dbaagent/model/ConnectionAccessLevel.java b/backend/src/main/java/com/dbaagent/model/ConnectionAccessLevel.java index 6f198db..9b8e9c3 100644 --- a/backend/src/main/java/com/dbaagent/model/ConnectionAccessLevel.java +++ b/backend/src/main/java/com/dbaagent/model/ConnectionAccessLevel.java @@ -1,16 +1,32 @@ package com.dbaagent.model; +/** + * The access level stored on a {@link ConnectionAccessGrant}. + * + *

There is now only one level: {@link #FULL_CONTENT}. Assigning a connection to a user + * grants full content access to it — the old two-tier split (chat/editor only vs. full) + * was a distinction users had to reason about for little benefit, and it silently hid the + * Dashboards section from anyone on the lower tier. + * + *

{@code CHAT_EDITOR} is retained purely so existing rows written before this change + * still parse; {@link #fromString} folds it into FULL_CONTENT rather than failing, and + * nothing writes it any more. Do not reintroduce it as a distinct level without also + * restoring the UI that explains it. + */ public enum ConnectionAccessLevel { + /** @deprecated legacy value; resolves to {@link #FULL_CONTENT}. Retained for old rows. */ + @Deprecated CHAT_EDITOR, FULL_CONTENT; public static ConnectionAccessLevel fromString(String value) { if (value == null || value.isBlank()) { - throw new IllegalArgumentException("Access level is required"); + // Assignment now implies full access, so an omitted level is not an error. + return FULL_CONTENT; } return switch (value.trim().toUpperCase()) { - case "CHAT_EDITOR" -> CHAT_EDITOR; - case "FULL_CONTENT", "FULL_ACCESS" -> FULL_CONTENT; + // Legacy values all collapse to the single remaining level. + case "CHAT_EDITOR", "FULL_CONTENT", "FULL_ACCESS" -> FULL_CONTENT; default -> throw new IllegalArgumentException("Unsupported access level: " + value); }; } diff --git a/backend/src/main/java/com/dbaagent/service/security/ConnectionAccessService.java b/backend/src/main/java/com/dbaagent/service/security/ConnectionAccessService.java index c352804..adb4a3a 100644 --- a/backend/src/main/java/com/dbaagent/service/security/ConnectionAccessService.java +++ b/backend/src/main/java/com/dbaagent/service/security/ConnectionAccessService.java @@ -59,12 +59,14 @@ public ResolvedConnectionAccess resolveAccess(DatabaseConnection connection, Str } return grantRepository.findByConnectionIdAndUsernameIgnoreCase(connection.getId(), username) + // A grant is a grant: assignment implies full content access. Legacy + // CHAT_EDITOR rows resolve here too rather than to a lesser tier, so an + // existing user gains Dashboards/Brain write access instead of silently + // keeping a level the UI no longer explains. .map(grant -> buildResolved( connection, ConnectionOwnershipType.ASSIGNED, - grant.getAccessLevel() == ConnectionAccessLevel.FULL_CONTENT - ? EffectiveConnectionAccess.FULL_CONTENT - : EffectiveConnectionAccess.CHAT_EDITOR + EffectiveConnectionAccess.FULL_CONTENT )) .orElseGet(() -> buildResolved(connection, null, EffectiveConnectionAccess.NONE)); } diff --git a/backend/src/test/java/com/dbaagent/service/security/AccessControlServiceTest.java b/backend/src/test/java/com/dbaagent/service/security/AccessControlServiceTest.java index 1590e3a..2c07f30 100644 --- a/backend/src/test/java/com/dbaagent/service/security/AccessControlServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/security/AccessControlServiceTest.java @@ -270,6 +270,16 @@ void feedbackAccessUsesUnderlyingConnectionOwnership() { assertDoesNotThrow(() -> accessControlService.assertCanAccessFeedback("fb-1")); } + /** + * Guards the CHAT_EDITOR branch of {@link EffectiveConnectionAccess} itself. + * + *

Note this state is no longer reachable from a real grant: connection access + * levels collapsed to a single tier, so {@code ConnectionAccessService.resolveAccess} + * returns FULL_CONTENT for every grant (see + * {@code ConnectionAccessLevelCollapseTest}). This test stubs the resolver directly, + * so it exercises the enum's semantics, not the resolution path — keep it for the + * former, do not read it as evidence about the latter. + */ @Test void assignedChatEditorUserCanUseChatEditor() { SecurityContextHolder.getContext().setAuthentication( diff --git a/backend/src/test/java/com/dbaagent/service/security/ConnectionAccessLevelCollapseTest.java b/backend/src/test/java/com/dbaagent/service/security/ConnectionAccessLevelCollapseTest.java new file mode 100644 index 0000000..8447adc --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/security/ConnectionAccessLevelCollapseTest.java @@ -0,0 +1,131 @@ +package com.dbaagent.service.security; + +import com.dbaagent.model.*; +import com.dbaagent.repository.ConnectionAccessGrantRepository; +import com.dbaagent.repository.CredentialRepository; +import com.dbaagent.repository.UserRepository; +import com.dbaagent.service.ConnectionChatAccessPolicyService; +import com.dbaagent.service.SecurityEventService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * The connection access tiers collapsed into a single level: any grant means full + * content access. + * + *

These exercise the real {@link ConnectionAccessService#resolveAccess} + * path. {@code AccessControlServiceTest} cannot cover this: it stubs + * {@code resolveAccess} to hand back a fixed {@link EffectiveConnectionAccess}, so it + * keeps passing no matter what the resolution logic does — it still contains a + * CHAT_EDITOR case that passes vacuously. A mock cannot catch a change to the thing it + * replaces. + */ +class ConnectionAccessLevelCollapseTest { + + private static final String CONN = "conn-1"; + + private CredentialRepository credentialRepository; + private ConnectionAccessGrantRepository grantRepository; + private UserRepository userRepository; + private ConnectionAccessService service; + + @BeforeEach + void setUp() { + credentialRepository = mock(CredentialRepository.class); + grantRepository = mock(ConnectionAccessGrantRepository.class); + userRepository = mock(UserRepository.class); + + service = new ConnectionAccessService( + credentialRepository, + grantRepository, + userRepository, + mock(SecurityEventService.class), + mock(ConnectionChatAccessPolicyService.class) + ); + + // The connection must be admin-owned to be assignable at all. + User owner = new User(); + owner.setUsername("admin"); + owner.setRole("ADMIN"); + when(userRepository.findByUsername("admin")).thenReturn(Optional.of(owner)); + } + + private DatabaseConnection connection() { + DatabaseConnection c = new DatabaseConnection(); + c.setId(CONN); + c.setOwnerUsername("admin"); + return c; + } + + private void grantWith(ConnectionAccessLevel level) { + ConnectionAccessGrant grant = new ConnectionAccessGrant(); + grant.setConnectionId(CONN); + grant.setUsername("dave"); + grant.setAccessLevel(level); + when(grantRepository.findByConnectionIdAndUsernameIgnoreCase(CONN, "dave")) + .thenReturn(Optional.of(grant)); + } + + @Test + @DisplayName("A legacy CHAT_EDITOR grant row now resolves to FULL_CONTENT") + void legacyChatEditorRowIsUpgraded() { + // Installs predating the collapse still have CHAT_EDITOR rows on disk. They must + // resolve to full access rather than a tier the UI no longer explains — this is + // what makes the change work with no DB migration. + grantWith(ConnectionAccessLevel.CHAT_EDITOR); + + var access = service.resolveAccess(connection(), "dave", false).getEffectiveAccess(); + + assertThat(access).isEqualTo(EffectiveConnectionAccess.FULL_CONTENT); + assertThat(access.canManageContent()).isTrue(); + assertThat(access.canReadContent()).isTrue(); + } + + @Test + @DisplayName("A FULL_CONTENT grant still resolves to FULL_CONTENT") + void fullContentUnchanged() { + grantWith(ConnectionAccessLevel.FULL_CONTENT); + + assertThat(service.resolveAccess(connection(), "dave", false).getEffectiveAccess()) + .isEqualTo(EffectiveConnectionAccess.FULL_CONTENT); + } + + @Test + @DisplayName("No grant still means NONE — the collapse must not hand access to strangers") + void noGrantStillDenied() { + when(grantRepository.findByConnectionIdAndUsernameIgnoreCase(CONN, "dave")) + .thenReturn(Optional.empty()); + + var access = service.resolveAccess(connection(), "dave", false).getEffectiveAccess(); + + assertThat(access).isEqualTo(EffectiveConnectionAccess.NONE); + assertThat(access.canUseConnection()).isFalse(); + assertThat(access.canManageContent()).isFalse(); + } + + @Test + @DisplayName("The owner is still OWNER, not merely FULL_CONTENT") + void ownerUnchanged() { + assertThat(service.resolveAccess(connection(), "admin", false).getEffectiveAccess()) + .isEqualTo(EffectiveConnectionAccess.OWNER); + } + + @Test + @DisplayName("Every stored access-level string parses to FULL_CONTENT") + void fromStringCollapses() { + assertThat(ConnectionAccessLevel.fromString("CHAT_EDITOR")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT); + assertThat(ConnectionAccessLevel.fromString("FULL_CONTENT")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT); + assertThat(ConnectionAccessLevel.fromString("FULL_ACCESS")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT); + // Blank is no longer an error: assignment implies full access. + assertThat(ConnectionAccessLevel.fromString(null)).isEqualTo(ConnectionAccessLevel.FULL_CONTENT); + assertThat(ConnectionAccessLevel.fromString(" ")).isEqualTo(ConnectionAccessLevel.FULL_CONTENT); + } +} diff --git a/src/components/tabs/admin/UsersTab.jsx b/src/components/tabs/admin/UsersTab.jsx index 1ab4a5a..eabeeb5 100644 --- a/src/components/tabs/admin/UsersTab.jsx +++ b/src/components/tabs/admin/UsersTab.jsx @@ -22,9 +22,13 @@ import { } from 'lucide-react' import { adminAPI } from '@/lib/api/client' import { useAuth, ROLES } from '@/hooks/useAuth' +import { PERMISSIONS, roleLabel } from '@/lib/permissions' +import RoleManager from './RoleManager' const ROLE_BADGE_CLASSES = { [ROLES.ADMIN]: 'bg-red-100 text-red-700 border-red-200', + [ROLES.DBA]: 'bg-purple-100 text-purple-700 border-purple-200', + [ROLES.DATA_ENGINEER]: 'bg-teal-100 text-teal-700 border-teal-200', [ROLES.DEVELOPER]: 'bg-blue-100 text-blue-700 border-blue-200', } @@ -35,18 +39,28 @@ const STATUS_BADGE_CLASSES = { DISABLED: 'bg-gray-100 text-gray-700 border-gray-200', } -const ACCESS_MATRIX = [ - { area: 'Chat', developer: 'Own + Assigned', admin: 'Full' }, - { area: 'Editor', developer: 'Own + Assigned', admin: 'Full' }, - { area: 'Brain', developer: 'Own + Full Access', admin: 'Full' }, - { area: 'Schema Docs', developer: 'Own + Full Access', admin: 'Full' }, - { area: 'Company Knowledge', developer: 'Own + Full Access', admin: 'Full' }, - { area: 'Performance', developer: '—', admin: 'Full' }, +/** + * The sidebar sections, and the permission that opens each. Rendered as a live matrix + * against whatever roles the backend reports, so a new custom role appears here without + * a code change — the previous hardcoded two-column table silently went stale the moment + * a third role existed. + */ +const SECTION_MATRIX = [ + { area: 'Agent', permission: PERMISSIONS.VIEW_AGENT }, + { area: 'Dashboards', permission: PERMISSIONS.VIEW_DASHBOARDS }, + { area: 'Digest', permission: PERMISSIONS.VIEW_DIGEST }, + { area: 'Brain', permission: PERMISSIONS.VIEW_BRAIN }, + { area: 'Performance', permission: PERMISSIONS.VIEW_PERFORMANCE }, + { area: 'Editor', permission: PERMISSIONS.VIEW_EDITOR }, + { area: 'Connection settings', permission: PERMISSIONS.MANAGE_CONNECTIONS }, + { area: 'User management', permission: PERMISSIONS.MANAGE_USERS }, ] const FALLBACK_ROLES = [ - { name: ROLES.DEVELOPER, description: 'Access to Chat and the SQL Editor' }, - { name: ROLES.ADMIN, description: 'Access to all product areas and administrative controls' }, + { name: ROLES.ADMIN, description: 'Full access to all product areas and administrative controls' }, + { name: ROLES.DBA, description: 'All product areas and connection settings, except user management' }, + { name: ROLES.DATA_ENGINEER, description: 'Agent, Dashboards, and the SQL Editor' }, + { name: ROLES.DEVELOPER, description: 'Agent, Digest, Dashboards, Performance, and the SQL Editor' }, ] function badgeClassForRole(role) { @@ -552,26 +566,48 @@ export default function UsersTab() {

Role Permissions

-
+
- - + {availableRoles.map((role) => ( + + ))} - {ACCESS_MATRIX.map((row) => ( + {SECTION_MATRIX.map((row) => ( - - - + + {availableRoles.map((role) => { + const granted = (role.permissions || []).some( + (p) => (typeof p === 'string' ? p : p?.name) === row.permission, + ) + return ( + + ) + })} ))}
AreaDeveloperAdmin + {role.displayName || roleLabel(role.name)} +
{row.area}{row.developer}{row.admin}{row.area} + {granted ? ( + + ) : ( + — + )} +
+

+ Built-in roles are fixed. Create a custom role below to define your own combination. +

+ +
+ +
@@ -668,7 +704,7 @@ function RoleSelector({ user, roles, onRoleChange, loading, disabled }) { > {roles.map((role) => ( ))} @@ -921,7 +957,6 @@ function ChangePasswordModal({ user, loading, onClose, onSubmit }) { } function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke, onSavePolicy, onPreviewPolicy }) { - const [drafts, setDrafts] = useState({}) const [policyDrafts, setPolicyDrafts] = useState({}) const [policyPreviews, setPolicyPreviews] = useState({}) const [previewLoading, setPreviewLoading] = useState({}) @@ -960,15 +995,14 @@ function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke,
{assignableConnections.map((connection) => { const assignment = assignmentsByConnection.get(connection.connectionId) - const draft = drafts[connection.connectionId] || assignment?.accessLevel || 'CHAT_EDITOR' + // Assignment implies full access now, so there is no level to choose. + const draft = 'FULL_CONTENT' const policyDraft = policyDrafts[connection.connectionId] ?? assignment?.chatAccessPolicy?.plainEnglishPolicy ?? '' const preview = policyPreviews[connection.connectionId] || assignment?.chatAccessPolicy const isAssigned = Boolean(assignment) - const isFull = assignment?.accessLevel === 'FULL_CONTENT' - const isDirty = isAssigned && draft !== assignment.accessLevel return (
{connection.connectionName} {isAssigned ? ( - + - {isFull ? 'Full Access' : 'Chat + Editor'} + Assigned ) : ( @@ -1003,22 +1033,13 @@ function ConnectionAccessModal({ user, data, loading, onClose, onSave, onRevoke,
- + {canManageConnections && ( + + )}
- + {canOpenSettings && ( + + )}