tbh/breadcrumb-the-shire
1
0
Fork 0
forked from fa/breadcrumb-the-shire
Code Pull Requests Activity

fix(security): enforce atomic role/permission writes and user assignment rollback

Browse Source
This commit is contained in:
Felix Schneider
2026-03-09 19:54:58 +01:00
parent 11ca546eae
commit 01c05d997f
5 changed files with 228 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
lib/Service/User/UserAccountService.php
View File

@@ -221,9 +221,14 @@ class UserAccountService
$createdUser = $this->userReadRepository->findByEmail($form['email']);
$uuid = $createdUser['uuid'] ?? null;
$userId = (int) ($createdUser['id'] ?? 0);
if ($userId <= 0) {
$this->rollbackQuietly();
return ['ok' => false, 'errors' => [t('User can not be registered')], 'form' => $form];
}
if ($userId > 0 && $tenantIds) {
$this->userAssignmentService->syncTenants($userId, $tenantIds);
if ($tenantIds && !$this->userAssignmentService->syncTenants($userId, $tenantIds)) {
$this->rollbackQuietly();
return ['ok' => false, 'errors' => [t('User can not be registered')], 'form' => $form];
}
$roleIds = $this->userAssignmentService->normalizeIdInput($input['role_ids'] ?? []);
@@ -233,8 +238,9 @@ class UserAccountService
$roleIds = [$defaultRoleId];
}
}
if ($userId > 0 && $roleIds) {
$this->userAssignmentService->syncRoles($userId, $roleIds);
if ($roleIds && !$this->userAssignmentService->syncRoles($userId, $roleIds)) {
$this->rollbackQuietly();
return ['ok' => false, 'errors' => [t('User can not be registered')], 'form' => $form];
}
$departmentIds = $this->userAssignmentService->normalizeIdInput($input['department_ids'] ?? []);
@@ -244,8 +250,9 @@ class UserAccountService
$departmentIds = [$defaultDepartmentId];
}
}
if ($userId > 0 && $departmentIds) {
$this->userAssignmentService->syncDepartments($userId, $departmentIds);
if ($departmentIds && !$this->userAssignmentService->syncDepartments($userId, $departmentIds)) {
$this->rollbackQuietly();
return ['ok' => false, 'errors' => [t('User can not be registered')], 'form' => $form];
}
$this->databaseSessionRepository->commitTransaction();
@@ -260,7 +267,7 @@ class UserAccountService
$this->systemAuditService->record('admin.users.create', 'success', [
'actor_user_id' => $currentUserId > 0 ? $currentUserId : null,
'target_type' => 'user',
'target_id' => $userId > 0 ? $userId : null,
'target_id' => $userId,
'target_uuid' => is_string($uuid) ? $uuid : '',
'metadata' => [
'assigned_tenant_ids' => $tenantIds,
@@ -522,18 +529,24 @@ class UserAccountService
$createdUser = $this->userReadRepository->findByEmail($form['email']);
$userId = (int) ($createdUser['id'] ?? 0);
if ($userId > 0) {
if ($defaultTenantId) {
$this->userAssignmentService->syncTenants($userId, [$defaultTenantId]);
}
$defaultRoleId = $this->settingsGateway->getDefaultRoleId();
if ($defaultRoleId) {
$this->userAssignmentService->syncRoles($userId, [$defaultRoleId]);
}
$defaultDepartmentId = $this->settingsGateway->getDefaultDepartmentId();
if ($defaultDepartmentId) {
$this->userAssignmentService->syncDepartments($userId, [$defaultDepartmentId]);
}
if ($userId <= 0) {
$this->rollbackQuietly();
return ['ok' => false, 'error' => t('User can not be registered')];
}
if ($defaultTenantId && !$this->userAssignmentService->syncTenants($userId, [$defaultTenantId])) {
$this->rollbackQuietly();
return ['ok' => false, 'error' => t('User can not be registered')];
}
$defaultRoleId = $this->settingsGateway->getDefaultRoleId();
if ($defaultRoleId && !$this->userAssignmentService->syncRoles($userId, [$defaultRoleId])) {
$this->rollbackQuietly();
return ['ok' => false, 'error' => t('User can not be registered')];
}
$defaultDepartmentId = $this->settingsGateway->getDefaultDepartmentId();
if ($defaultDepartmentId && !$this->userAssignmentService->syncDepartments($userId, [$defaultDepartmentId])) {
$this->rollbackQuietly();
return ['ok' => false, 'error' => t('User can not be registered')];
}
$this->databaseSessionRepository->commitTransaction();

18
pages/admin/permissions/edit($id).php
View File

@@ -68,6 +68,7 @@ if ($request->hasBody('key')) {
$roleIds = [$roleIds];
}
$roleIds = array_values(array_unique(array_map('intval', $roleIds)));
$syncSucceeded = false;
$dbSession = app(\MintyPHP\Repository\Support\DatabaseSessionRepository::class);
$dbSession->beginTransaction();
try {
@@ -78,6 +79,7 @@ if ($request->hasBody('key')) {
$userWriteRepository->bumpAuthzVersionByUserIds($affectedUserIds);
}
$dbSession->commitTransaction();
$syncSucceeded = true;
} catch (\Throwable) {
try {
$dbSession->rollbackTransaction();
@@ -85,13 +87,15 @@ if ($request->hasBody('key')) {
}
$errorBag->merge([t('Failed to update permission roles')]);
}
$action = (string) $request->body('action', 'save');
if ($action === 'save_close') {
Flash::success('Permission updated', 'admin/permissions', 'permission_updated');
Router::redirect('admin/permissions');
} else {
Flash::success('Permission updated', "admin/permissions/edit/{$id}", 'permission_updated');
Router::redirect("admin/permissions/edit/{$id}");
if ($syncSucceeded) {
$action = (string) $request->body('action', 'save');
if ($action === 'save_close') {
Flash::success('Permission updated', 'admin/permissions', 'permission_updated');
Router::redirect('admin/permissions');
} else {
Flash::success('Permission updated', "admin/permissions/edit/{$id}", 'permission_updated');
Router::redirect("admin/permissions/edit/{$id}");
}
}
}
}

86
pages/admin/roles/create().php
View File

@@ -33,50 +33,62 @@ $permissions = $permissionRepository->listActive();
$selectedPermissionIds = [];
if ($request->hasBody('description')) {
$result = app(\MintyPHP\Service\Access\RoleService::class)->createFromAdmin($request->bodyAll(), $currentUserId);
$form = $result['form'] ?? $form;
$errorBag->merge($result['errors'] ?? []);
$selectedPermissionIds = $request->body('permission_ids', $selectedPermissionIds);
if (!is_array($selectedPermissionIds)) {
$selectedPermissionIds = [$selectedPermissionIds];
}
$selectedPermissionIds = array_values(array_unique(array_map('intval', $selectedPermissionIds)));
$dbSession = app(\MintyPHP\Repository\Support\DatabaseSessionRepository::class);
$transactionStarted = false;
if (($result['ok'] ?? false) && !$errorBag->hasAny()) {
$permissionIds = $request->body('permission_ids', []);
if (!is_array($permissionIds)) {
$permissionIds = [$permissionIds];
try {
$dbSession->beginTransaction();
$transactionStarted = true;
$result = app(\MintyPHP\Service\Access\RoleService::class)->createFromAdmin($request->bodyAll(), $currentUserId);
$form = $result['form'] ?? $form;
$errorBag->merge($result['errors'] ?? []);
$selectedPermissionIds = $request->body('permission_ids', $selectedPermissionIds);
if (!is_array($selectedPermissionIds)) {
$selectedPermissionIds = [$selectedPermissionIds];
}
$permissionIds = array_values(array_unique(array_map('intval', $permissionIds)));
$createdId = (int) ($result['id'] ?? 0);
if ($createdId > 0) {
$dbSession = app(\MintyPHP\Repository\Support\DatabaseSessionRepository::class);
$dbSession->beginTransaction();
try {
$rolePermissionRepository->replaceForRole($createdId, $permissionIds);
$dbSession->commitTransaction();
} catch (\Throwable) {
try {
$dbSession->rollbackTransaction();
} catch (\Throwable) {
$selectedPermissionIds = array_values(array_unique(array_map('intval', $selectedPermissionIds)));
if (($result['ok'] ?? false) && !$errorBag->hasAny()) {
$permissionIds = $request->body('permission_ids', []);
if (!is_array($permissionIds)) {
$permissionIds = [$permissionIds];
}
$permissionIds = array_values(array_unique(array_map('intval', $permissionIds)));
$createdId = (int) ($result['id'] ?? 0);
if ($createdId <= 0) {
throw new \RuntimeException('role_create_missing_id');
}
$rolePermissionRepository->replaceForRole($createdId, $permissionIds);
$dbSession->commitTransaction();
$transactionStarted = false;
$action = (string) $request->body('action', 'create');
if ($action === 'create_close') {
Flash::success('Role created', 'admin/roles', 'role_created');
Router::redirect('admin/roles');
} else {
$uuid = (string) ($result['uuid'] ?? '');
if ($uuid !== '') {
$target = "admin/roles/edit/{$uuid}";
Flash::success('Role created', $target, 'role_created');
Router::redirect($target);
} else {
Flash::success('Role created', 'admin/roles', 'role_created');
Router::redirect('admin/roles');
}
}
}
$action = (string) $request->body('action', 'create');
if ($action === 'create_close') {
Flash::success('Role created', 'admin/roles', 'role_created');
Router::redirect('admin/roles');
} else {
$uuid = (string) ($result['uuid'] ?? '');
if ($uuid !== '') {
$target = "admin/roles/edit/{$uuid}";
Flash::success('Role created', $target, 'role_created');
Router::redirect($target);
} else {
Flash::success('Role created', 'admin/roles', 'role_created');
Router::redirect('admin/roles');
} catch (\Throwable) {
if ($transactionStarted) {
try {
$dbSession->rollbackTransaction();
} catch (\Throwable) {
}
}
if (!$errorBag->hasAny()) {
$errorBag->merge([t('Failed to update role permissions')]);
}
}
}

18
pages/admin/roles/edit($id).php
View File

@@ -86,6 +86,7 @@ if ($request->hasBody('description')) {
$permissionIds = [$permissionIds];
}
$permissionIds = array_values(array_unique(array_map('intval', $permissionIds)));
$syncSucceeded = false;
$dbSession = app(\MintyPHP\Repository\Support\DatabaseSessionRepository::class);
$dbSession->beginTransaction();
try {
@@ -95,6 +96,7 @@ if ($request->hasBody('description')) {
$userWriteRepository->bumpAuthzVersionByUserIds($affectedUserIds);
}
$dbSession->commitTransaction();
$syncSucceeded = true;
} catch (\Throwable) {
try {
$dbSession->rollbackTransaction();
@@ -102,13 +104,15 @@ if ($request->hasBody('description')) {
}
$errorBag->merge([t('Failed to update role permissions')]);
}
$action = (string) $request->body('action', 'save');
if ($action === 'save_close') {
Flash::success('Role updated', 'admin/roles', 'role_updated');
Router::redirect('admin/roles');
} else {
Flash::success('Role updated', "admin/roles/edit/{$uuid}", 'role_updated');
Router::redirect("admin/roles/edit/{$uuid}");
if ($syncSucceeded) {
$action = (string) $request->body('action', 'save');
if ($action === 'save_close') {
Flash::success('Role updated', 'admin/roles', 'role_updated');
Router::redirect('admin/roles');
} else {
Flash::success('Role updated', "admin/roles/edit/{$uuid}", 'role_updated');
Router::redirect("admin/roles/edit/{$uuid}");
}
}
}
}

128
tests/Service/User/UserAccountServiceTest.php
View File

@@ -260,9 +260,9 @@ class UserAccountServiceTest extends TestCase
);
$assignmentService = $this->createMock(UserAssignmentService::class);
$assignmentService->expects($this->once())->method('syncTenants')->with(77, [3]);
$assignmentService->expects($this->once())->method('syncRoles')->with(77, [4]);
$assignmentService->expects($this->once())->method('syncDepartments')->with(77, [5]);
$assignmentService->expects($this->once())->method('syncTenants')->with(77, [3])->willReturn(true);
$assignmentService->expects($this->once())->method('syncRoles')->with(77, [4])->willReturn(true);
$assignmentService->expects($this->once())->method('syncDepartments')->with(77, [5])->willReturn(true);
$service = $this->newService(
$readRepository,
@@ -284,6 +284,128 @@ class UserAccountServiceTest extends TestCase
$this->assertTrue($result['ok']);
}
public function testCreateFromAdminRollsBackTransactionWhenTenantSyncFails(): void
{
$passwordService = $this->createMock(UserPasswordService::class);
$passwordService->method('validatePassword')->willReturn([]);
$settingsGateway = $this->createMock(UserSettingsGateway::class);
$settingsGateway->method('getDefaultTenantId')->willReturn(null);
$settingsGateway->method('getDefaultRoleId')->willReturn(null);
$settingsGateway->method('getDefaultDepartmentId')->willReturn(null);
$settingsGateway->method('getAppTheme')->willReturn('light');
$writeRepository = $this->createMock(UserWriteRepositoryInterface::class);
$writeRepository->expects($this->once())->method('create')->willReturn(88);
$readRepository = $this->createMock(UserReadRepositoryInterface::class);
$readRepository->expects($this->exactly(2))
->method('findByEmail')
->with('create@example.com')
->willReturnOnConsecutiveCalls(
null,
['id' => 88, 'uuid' => 'user-uuid-88']
);
$assignmentService = $this->createMock(UserAssignmentService::class);
$assignmentService->expects($this->once())->method('normalizeTenantIds')->with([3])->willReturn([3]);
$assignmentService->expects($this->once())->method('syncTenants')->with(88, [3])->willReturn(false);
$assignmentService->expects($this->never())->method('syncRoles');
$assignmentService->expects($this->never())->method('syncDepartments');
$auditService = $this->createMock(SystemAuditService::class);
$auditService->expects($this->never())->method('record');
$databaseSessionRepository = $this->createMock(DatabaseSessionRepository::class);
$databaseSessionRepository->expects($this->once())->method('beginTransaction');
$databaseSessionRepository->expects($this->never())->method('commitTransaction');
$databaseSessionRepository->expects($this->once())->method('rollbackTransaction');
$service = $this->newService(
$readRepository,
$writeRepository,
null,
$assignmentService,
$passwordService,
$settingsGateway,
null,
null,
$auditService,
$databaseSessionRepository
);
$result = $service->createFromAdmin([
'first_name' => 'Max',
'last_name' => 'Mustermann',
'email' => 'create@example.com',
'password' => 'StrongPass1!',
'password2' => 'StrongPass1!',
'tenant_ids' => [3],
'active' => 1,
], 42);
$this->assertFalse($result['ok']);
$this->assertNotEmpty($result['errors'] ?? []);
}
public function testRegisterRollsBackTransactionWhenRoleSyncFails(): void
{
$passwordService = $this->createMock(UserPasswordService::class);
$passwordService->method('validatePassword')->willReturn([]);
$settingsGateway = $this->createMock(UserSettingsGateway::class);
$settingsGateway->method('getAppTheme')->willReturn('dark');
$settingsGateway->method('getDefaultTenantId')->willReturn(3);
$settingsGateway->method('getDefaultRoleId')->willReturn(4);
$settingsGateway->method('getDefaultDepartmentId')->willReturn(5);
$writeRepository = $this->createMock(UserWriteRepositoryInterface::class);
$writeRepository->expects($this->once())->method('create')->willReturn(77);
$readRepository = $this->createMock(UserReadRepositoryInterface::class);
$readRepository->expects($this->exactly(2))
->method('findByEmail')
->with('user@example.com')
->willReturnOnConsecutiveCalls(
null,
['id' => 77, 'uuid' => 'user-uuid']
);
$assignmentService = $this->createMock(UserAssignmentService::class);
$assignmentService->expects($this->once())->method('syncTenants')->with(77, [3])->willReturn(true);
$assignmentService->expects($this->once())->method('syncRoles')->with(77, [4])->willReturn(false);
$assignmentService->expects($this->never())->method('syncDepartments');
$databaseSessionRepository = $this->createMock(DatabaseSessionRepository::class);
$databaseSessionRepository->expects($this->once())->method('beginTransaction');
$databaseSessionRepository->expects($this->never())->method('commitTransaction');
$databaseSessionRepository->expects($this->once())->method('rollbackTransaction');
$service = $this->newService(
$readRepository,
$writeRepository,
null,
$assignmentService,
$passwordService,
$settingsGateway,
null,
null,
null,
$databaseSessionRepository
);
$result = $service->register([
'first_name' => 'Max',
'last_name' => 'Mustermann',
'email' => 'user@example.com',
'password' => 'StrongPass1!',
'password2' => 'StrongPass1!',
]);
$this->assertFalse($result['ok']);
$this->assertNotSame('', trim((string) ($result['error'] ?? '')));
}
public function testUpdateSelfProfileReturnsErrorWhenRequestedTenantIsNotAssigned(): void
{
$readRepository = $this->createMock(UserReadRepositoryInterface::class);