shahondin1624/Mitgliederverwaltung
Public Access
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 16 Wiki Activity

security: enforce access control on CalendarSync and ContactsSync (Closes #170)

Browse Source
This commit was merged in pull request #188.
This commit is contained in:
shahondin1624
2026-04-10 18:56:16 +02:00
parent 58968331eb
commit b6a7854a1b
4 changed files with 454 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/Service/CalendarSyncService.php
+50
View File
@@ -8,6 +8,7 @@ use OCA\Mitgliederverwaltung\Db\Member;
use OCA\Mitgliederverwaltung\Db\MemberMapper;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\IUserSession;
use Psr\Log\LoggerInterface;
use Sabre\VObject\Component\VCalendar;
@@ -17,6 +18,11 @@ use Sabre\VObject\Component\VCalendar;
* Creates/manages the "Pfadfinder Geburtstage" calendar with recurring yearly
* all-day events for each active member (excluding Erziehungsberechtigte).
*
* Note: Uses a custom staging table (mv_calendar_events) because
* OCP\Calendar\IManager does not provide write methods. All sync
* operations are guarded by PermissionService to enforce Nextcloud
* access control. See Issue #170.
*
* Part of Issue #50.
*/
class CalendarSyncService {
@@ -26,18 +32,56 @@ class CalendarSyncService {
private MemberMapper $memberMapper;
private IDBConnection $db;
private PermissionService $permissionService;
private IUserSession $userSession;
private LoggerInterface $logger;
public function __construct(
MemberMapper $memberMapper,
IDBConnection $db,
PermissionService $permissionService,
IUserSession $userSession,
LoggerInterface $logger
) {
$this->memberMapper = $memberMapper;
$this->db = $db;
$this->permissionService = $permissionService;
$this->userSession = $userSession;
$this->logger = $logger;
}
/**
* Assert that the current user has write-level access.
*
* Sync operations modify data (calendar events linked to members),
* so at minimum write permission is required.
*
* In background job context (no user session), this check is skipped
* as background jobs are system-level operations.
*
* @throws \RuntimeException if a user is logged in but lacks permission
*/
private function assertWritePermission(): void {
$user = $this->userSession->getUser();
if ($user === null) {
// Background job context — no user session available.
// Background jobs are trusted system-level operations.
$this->logger->debug('Calendar sync running in background context (no user session)', [
'app' => 'mitgliederverwaltung',
]);
return;
}
$userId = $user->getUID();
if (!$this->permissionService->canWrite($userId)) {
$this->logger->warning('Calendar sync denied: insufficient permissions', [
'userId' => $userId,
'app' => 'mitgliederverwaltung',
]);
throw new \RuntimeException('Calendar sync requires write permission');
}
}
/**
* Sync a single member's birthday event.
*
@@ -48,6 +92,8 @@ class CalendarSyncService {
* @param Member $member The member to sync
*/
public function syncMember(Member $member): void {
$this->assertWritePermission();
$shouldHaveEvent = $this->shouldHaveEvent($member);
$hasEvent = $member->getCalendarEventUri() !== null;
@@ -68,6 +114,8 @@ class CalendarSyncService {
* @return array{created: int, updated: int, deleted: int}
*/
public function fullSync(): array {
$this->assertWritePermission();
$stats = ['created' => 0, 'updated' => 0, 'deleted' => 0];
try {
@@ -214,6 +262,8 @@ class CalendarSyncService {
* Delete a birthday calendar event for a member.
*/
public function deleteEvent(Member $member): void {
$this->assertWritePermission();
$eventUri = $member->getCalendarEventUri();
if ($eventUri !== null) {
lib/Service/ContactsSyncService.php
+194 -4
View File
@@ -9,8 +9,10 @@ use OCA\Mitgliederverwaltung\Db\EmailMapper;
use OCA\Mitgliederverwaltung\Db\Member;
use OCA\Mitgliederverwaltung\Db\MemberMapper;
use OCA\Mitgliederverwaltung\Db\PhoneMapper;
use OCP\Contacts\IManager as IContactsManager;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\IUserSession;
use Psr\Log\LoggerInterface;
use Sabre\VObject\Component\VCard;
@@ -20,6 +22,11 @@ use Sabre\VObject\Component\VCard;
* Creates/manages the "Pfadfinder Mitglieder" address book with VCards
* for all active members (including Erziehungsberechtigte).
*
* Uses OCP\Contacts\IManager for Nextcloud-native contacts integration
* with proper access control. Falls back to the staging table
* (mv_contact_cards) when the Contacts app is not available.
* See Issue #170.
*
* Part of Issue #51.
*/
class ContactsSyncService {
@@ -31,6 +38,9 @@ class ContactsSyncService {
private AddressMapper $addressMapper;
private PhoneMapper $phoneMapper;
private EmailMapper $emailMapper;
private IContactsManager $contactsManager;
private PermissionService $permissionService;
private IUserSession $userSession;
private IDBConnection $db;
private LoggerInterface $logger;
@@ -39,6 +49,9 @@ class ContactsSyncService {
AddressMapper $addressMapper,
PhoneMapper $phoneMapper,
EmailMapper $emailMapper,
IContactsManager $contactsManager,
PermissionService $permissionService,
IUserSession $userSession,
IDBConnection $db,
LoggerInterface $logger
) {
@@ -46,10 +59,65 @@ class ContactsSyncService {
$this->addressMapper = $addressMapper;
$this->phoneMapper = $phoneMapper;
$this->emailMapper = $emailMapper;
$this->contactsManager = $contactsManager;
$this->permissionService = $permissionService;
$this->userSession = $userSession;
$this->db = $db;
$this->logger = $logger;
}
/**
* Assert that the current user has write-level access.
*
* Sync operations modify contact data linked to members,
* so at minimum write permission is required.
*
* In background job context (no user session), this check is skipped
* as background jobs are system-level operations.
*
* @throws \RuntimeException if a user is logged in but lacks permission
*/
private function assertWritePermission(): void {
$user = $this->userSession->getUser();
if ($user === null) {
// Background job context — no user session available.
// Background jobs are trusted system-level operations.
$this->logger->debug('Contacts sync running in background context (no user session)', [
'app' => 'mitgliederverwaltung',
]);
return;
}
$userId = $user->getUID();
if (!$this->permissionService->canWrite($userId)) {
$this->logger->warning('Contacts sync denied: insufficient permissions', [
'userId' => $userId,
'app' => 'mitgliederverwaltung',
]);
throw new \RuntimeException('Contacts sync requires write permission');
}
}
/**
* Find the Nextcloud address book key for our dedicated address book.
*
* @return string|null The address book key, or null if not found
*/
private function findAddressBookKey(): ?string {
if (!$this->contactsManager->isEnabled()) {
return null;
}
$addressBooks = $this->contactsManager->getUserAddressBooks();
foreach ($addressBooks as $book) {
if ($book->getUri() === self::ADDRESSBOOK_URI) {
return $book->getKey();
}
}
return null;
}
/**
* Sync a single member's VCard.
*
@@ -58,6 +126,8 @@ class ContactsSyncService {
* - Inactive or deleted => delete VCard
*/
public function syncMember(Member $member): void {
$this->assertWritePermission();
$shouldHaveContact = $this->shouldHaveContact($member);
$hasContact = $member->getContactVcardUri() !== null;
@@ -76,6 +146,8 @@ class ContactsSyncService {
* @return array{created: int, updated: int, deleted: int}
*/
public function fullSync(): array {
$this->assertWritePermission();
$stats = ['created' => 0, 'updated' => 0, 'deleted' => 0];
try {
@@ -185,12 +257,29 @@ class ContactsSyncService {
private function createContact(Member $member): void {
$vcardUri = $this->generateVcardUri($member);
$vcardData = $this->buildVCard($member);
// Try Nextcloud Contacts API first for proper access control integration
$addressBookKey = $this->findAddressBookKey();
if ($addressBookKey !== null) {
$properties = $this->buildContactProperties($member);
$result = $this->contactsManager->createOrUpdate($properties, $addressBookKey);
if ($result !== null && isset($result['id'])) {
$this->updateMemberContactUri($member->getId(), (string)$result['id']);
$this->logger->debug('Created contact via Nextcloud Contacts API', [
'memberId' => $member->getId(),
'contactId' => $result['id'],
'app' => 'mitgliederverwaltung',
]);
return;
}
}
// Fallback to staging table when Contacts app is not available
$vcardData = $this->buildVCard($member);
$this->updateMemberContactUri($member->getId(), $vcardUri);
$this->storeContactData($vcardUri, $vcardData);
$this->logger->debug('Created contact VCard', [
$this->logger->debug('Created contact VCard (staging table fallback)', [
'memberId' => $member->getId(),
'vcardUri' => $vcardUri,
'app' => 'mitgliederverwaltung',
@@ -199,11 +288,28 @@ class ContactsSyncService {
private function updateContact(Member $member): void {
$vcardUri = $member->getContactVcardUri();
$vcardData = $this->buildVCard($member);
// Try Nextcloud Contacts API first for proper access control integration
$addressBookKey = $this->findAddressBookKey();
if ($addressBookKey !== null) {
$properties = $this->buildContactProperties($member);
// Include the existing contact ID for update
$properties['id'] = $vcardUri;
$result = $this->contactsManager->createOrUpdate($properties, $addressBookKey);
if ($result !== null) {
$this->logger->debug('Updated contact via Nextcloud Contacts API', [
'memberId' => $member->getId(),
'app' => 'mitgliederverwaltung',
]);
return;
}
}
// Fallback to staging table
$vcardData = $this->buildVCard($member);
$this->storeContactData($vcardUri, $vcardData);
$this->logger->debug('Updated contact VCard', [
$this->logger->debug('Updated contact VCard (staging table fallback)', [
'memberId' => $member->getId(),
'vcardUri' => $vcardUri,
'app' => 'mitgliederverwaltung',
@@ -211,6 +317,8 @@ class ContactsSyncService {
}
public function deleteContact(Member $member): void {
$this->assertWritePermission();
$vcardUri = $member->getContactVcardUri();
if ($vcardUri !== null) {
@@ -227,6 +335,88 @@ class ContactsSyncService {
// ── Helpers ──────────────────────────────────────────────────────
/**
* Build contact properties array for use with OCP\Contacts\IManager::createOrUpdate().
*
* @param Member $member The member to build properties for
* @return array Key-value pairs matching RFC 2426 property names
*/
private function buildContactProperties(Member $member): array {
$properties = [
'FN' => $member->getVorname() . ' ' . $member->getNachname(),
'N' => [
$member->getNachname(),
$member->getVorname(),
'', '', '',
],
'NOTE' => 'Rolle: ' . $member->getRolle(),
];
if ($member->getGeburtsdatum()) {
$properties['BDAY'] = $member->getGeburtsdatum();
}
// Addresses
try {
$addresses = $this->addressMapper->findByMemberId($member->getId());
$adrList = [];
foreach ($addresses as $address) {
$adrList[] = [
'type' => $address->getLabel() ?? 'HOME',
'value' => [
'', '',
$address->getStrasse(),
$address->getOrt(),
'',
$address->getPlz(),
$address->getLand() ?? 'Deutschland',
],
];
}
if (!empty($adrList)) {
$properties['ADR'] = $adrList;
}
} catch (\Exception $e) {
// Skip addresses on error
}
// Phones
try {
$phones = $this->phoneMapper->findByMemberId($member->getId());
$telList = [];
foreach ($phones as $phone) {
$telList[] = [
'type' => $phone->getLabel() ?? 'CELL',
'value' => $phone->getNumberE164(),
];
}
if (!empty($telList)) {
$properties['TEL'] = $telList;
}
} catch (\Exception $e) {
// Skip phones on error
}
// Emails
try {
$emails = $this->emailMapper->findByMemberId($member->getId());
$emailList = [];
foreach ($emails as $email) {
$emailList[] = [
'type' => $email->getLabel() ?? 'HOME',
'value' => $email->getEmail(),
];
}
if (!empty($emailList)) {
$properties['EMAIL'] = $emailList;
}
} catch (\Exception $e) {
// Skip emails on error
}
return $properties;
}
private function shouldHaveContact(Member $member): bool {
return $member->getStatus() === 'aktiv'
&& $member->getDeletedAt() === null;
tests/Unit/CalendarSyncServiceTest.php
+95
View File
@@ -7,9 +7,12 @@ namespace OCA\Mitgliederverwaltung\Tests\Unit;
use OCA\Mitgliederverwaltung\Db\Member;
use OCA\Mitgliederverwaltung\Db\MemberMapper;
use OCA\Mitgliederverwaltung\Service\CalendarSyncService;
use OCA\Mitgliederverwaltung\Service\PermissionService;
use OCP\DB\QueryBuilder\IExpressionBuilder;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\IUser;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
@@ -19,16 +22,47 @@ class CalendarSyncServiceTest extends TestCase {
private CalendarSyncService $service;
private MemberMapper&MockObject $memberMapper;
private IDBConnection&MockObject $db;
private PermissionService&MockObject $permissionService;
private IUserSession&MockObject $userSession;
private LoggerInterface&MockObject $logger;
protected function setUp(): void {
$this->memberMapper = $this->createMock(MemberMapper::class);
$this->db = $this->createMock(IDBConnection::class);
$this->permissionService = $this->createMock(PermissionService::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->logger = $this->createMock(LoggerInterface::class);
// Default: no user session (background job context — permission check skipped)
$this->userSession->method('getUser')->willReturn(null);
$this->service = new CalendarSyncService(
$this->memberMapper,
$this->db,
$this->permissionService,
$this->userSession,
$this->logger
);
}
/**
* Helper to create a service instance with a specific user context.
*/
private function createServiceWithUser(string $userId, bool $canWrite): CalendarSyncService {
$user = $this->createMock(IUser::class);
$user->method('getUID')->willReturn($userId);
$userSession = $this->createMock(IUserSession::class);
$userSession->method('getUser')->willReturn($user);
$permissionService = $this->createMock(PermissionService::class);
$permissionService->method('canWrite')->with($userId)->willReturn($canWrite);
return new CalendarSyncService(
$this->memberMapper,
$this->db,
$permissionService,
$userSession,
$this->logger
);
}
@@ -437,4 +471,65 @@ class CalendarSyncServiceTest extends TestCase {
$this->service->enqueueSync(42);
$this->assertTrue(true);
}
// ── Permission checks (Issue #170) ──────────────────────────
public function testSyncMemberDeniedWithoutWritePermission(): void {
$service = $this->createServiceWithUser('nowrite-user', false);
$member = $this->createMemberEntity(1, 'aktiv', 'mitglied');
$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Calendar sync requires write permission');
$service->syncMember($member);
}
public function testFullSyncDeniedWithoutWritePermission(): void {
$service = $this->createServiceWithUser('nowrite-user', false);
$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Calendar sync requires write permission');
$service->fullSync();
}
public function testDeleteEventDeniedWithoutWritePermission(): void {
$service = $this->createServiceWithUser('nowrite-user', false);
$member = $this->createMemberEntity(1, 'inaktiv', 'mitglied', 'mv-birthday-1.ics');
$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Calendar sync requires write permission');
$service->deleteEvent($member);
}
public function testSyncMemberAllowedWithWritePermission(): void {
if (!class_exists(\Sabre\VObject\Component\VCalendar::class)) {
$this->markTestSkipped('Sabre VObject not available');
}
$service = $this->createServiceWithUser('writer-user', true);
$qb = $this->createQueryBuilderMock();
$this->db->method('getQueryBuilder')->willReturn($qb);
$member = $this->createMemberEntity(1, 'aktiv', 'mitglied');
// Should not throw
$service->syncMember($member);
$this->assertTrue(true);
}
public function testSyncMemberAllowedInBackgroundContext(): void {
if (!class_exists(\Sabre\VObject\Component\VCalendar::class)) {
$this->markTestSkipped('Sabre VObject not available');
}
// Default setUp has no user session (background context)
$qb = $this->createQueryBuilderMock();
$this->db->method('getQueryBuilder')->willReturn($qb);
$member = $this->createMemberEntity(1, 'aktiv', 'mitglied');
// Should not throw - background context skips permission check
$this->service->syncMember($member);
$this->assertTrue(true);
}
}
tests/Unit/ContactsSyncServiceTest.php
+115
View File
@@ -13,9 +13,13 @@ use OCA\Mitgliederverwaltung\Db\MemberMapper;
use OCA\Mitgliederverwaltung\Db\Phone;
use OCA\Mitgliederverwaltung\Db\PhoneMapper;
use OCA\Mitgliederverwaltung\Service\ContactsSyncService;
use OCA\Mitgliederverwaltung\Service\PermissionService;
use OCP\Contacts\IManager as IContactsManager;
use OCP\DB\QueryBuilder\IExpressionBuilder;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\IUser;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
@@ -27,6 +31,9 @@ class ContactsSyncServiceTest extends TestCase {
private AddressMapper&MockObject $addressMapper;
private PhoneMapper&MockObject $phoneMapper;
private EmailMapper&MockObject $emailMapper;
private IContactsManager&MockObject $contactsManager;
private PermissionService&MockObject $permissionService;
private IUserSession&MockObject $userSession;
private IDBConnection&MockObject $db;
private LoggerInterface&MockObject $logger;
@@ -35,14 +42,55 @@ class ContactsSyncServiceTest extends TestCase {
$this->addressMapper = $this->createMock(AddressMapper::class);
$this->phoneMapper = $this->createMock(PhoneMapper::class);
$this->emailMapper = $this->createMock(EmailMapper::class);
$this->contactsManager = $this->createMock(IContactsManager::class);
$this->permissionService = $this->createMock(PermissionService::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->db = $this->createMock(IDBConnection::class);
$this->logger = $this->createMock(LoggerInterface::class);
// Default: no user session (background job context — permission check skipped)
$this->userSession->method('getUser')->willReturn(null);
// Default: contacts manager not enabled (falls back to staging table)
$this->contactsManager->method('isEnabled')->willReturn(false);
$this->service = new ContactsSyncService(
$this->memberMapper,
$this->addressMapper,
$this->phoneMapper,
$this->emailMapper,
$this->contactsManager,
$this->permissionService,
$this->userSession,
$this->db,
$this->logger
);
}
/**
* Helper to create a service instance with a specific user context.
*/
private function createServiceWithUser(string $userId, bool $canWrite): ContactsSyncService {
$user = $this->createMock(IUser::class);
$user->method('getUID')->willReturn($userId);
$userSession = $this->createMock(IUserSession::class);
$userSession->method('getUser')->willReturn($user);
$permissionService = $this->createMock(PermissionService::class);
$permissionService->method('canWrite')->with($userId)->willReturn($canWrite);
$contactsManager = $this->createMock(IContactsManager::class);
$contactsManager->method('isEnabled')->willReturn(false);
return new ContactsSyncService(
$this->memberMapper,
$this->addressMapper,
$this->phoneMapper,
$this->emailMapper,
$contactsManager,
$permissionService,
$userSession,
$this->db,
$this->logger
);
@@ -542,4 +590,71 @@ class ContactsSyncServiceTest extends TestCase {
$this->service->enqueueSync(42);
$this->assertTrue(true);
}
// ── Permission checks (Issue #170) ──────────────────────────
public function testSyncMemberDeniedWithoutWritePermission(): void {
$service = $this->createServiceWithUser('nowrite-user', false);
$member = $this->createMemberEntity(1, 'aktiv');
$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Contacts sync requires write permission');
$service->syncMember($member);
}
public function testFullSyncDeniedWithoutWritePermission(): void {
$service = $this->createServiceWithUser('nowrite-user', false);
$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Contacts sync requires write permission');
$service->fullSync();
}
public function testDeleteContactDeniedWithoutWritePermission(): void {
$service = $this->createServiceWithUser('nowrite-user', false);
$member = $this->createMemberEntity(1, 'inaktiv', 'mv-contact-1.vcf');
$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Contacts sync requires write permission');
$service->deleteContact($member);
}
public function testSyncMemberAllowedWithWritePermission(): void {
if (!class_exists(\Sabre\VObject\Component\VCard::class)) {
$this->markTestSkipped('Sabre VObject not available');
}
$service = $this->createServiceWithUser('writer-user', true);
$qb = $this->createQueryBuilderMock();
$this->db->method('getQueryBuilder')->willReturn($qb);
$member = $this->createMemberEntity(1, 'aktiv');
$this->addressMapper->method('findByMemberId')->willReturn([]);
$this->phoneMapper->method('findByMemberId')->willReturn([]);
$this->emailMapper->method('findByMemberId')->willReturn([]);
// Should not throw
$service->syncMember($member);
$this->assertTrue(true);
}
public function testSyncMemberAllowedInBackgroundContext(): void {
if (!class_exists(\Sabre\VObject\Component\VCard::class)) {
$this->markTestSkipped('Sabre VObject not available');
}
// Default setUp has no user session (background context)
$qb = $this->createQueryBuilderMock();
$this->db->method('getQueryBuilder')->willReturn($qb);
$member = $this->createMemberEntity(1, 'aktiv');
$this->addressMapper->method('findByMemberId')->willReturn([]);
$this->phoneMapper->method('findByMemberId')->willReturn([]);
$this->emailMapper->method('findByMemberId')->willReturn([]);
// Should not throw - background context skips permission check
$this->service->syncMember($member);
$this->assertTrue(true);
}
}