From 1720710d47ad419b10209a46b0083b0815f63ba4 Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Wed, 12 Aug 2026 15:56:49 +0200 Subject: [PATCH] fix(hr): let HR record their own absence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An HR member could not pick themselves in "Record absence", so they could not record their own sick leave at all. Not a permission problem — the server has always allowed it. create() defaults employeeUid to the actor, onBehalf is false for yourself, and both update() and cancel() carry explicit branches for "HR editing their own HR-recorded leave" (§5.6), which were unreachable for a record HR could never create. The picker was the blocker. It calls the app's employee search, which wraps core's collaborator search — and that deliberately drops the searching user from its own results, because it answers "who can I share with" and nobody shares with themselves. This picker asks "whose absence is this?", where you are a valid answer. The dialog then refuses to submit until an employee is chosen, and the only other route, "New request", offers just the self-requestable types, which sick leave deliberately is not. So there was no way through. The search now adds the signed-in user when they match what was typed, listed first because somebody typing their own name has matched it exactly. Guests are still excluded, their own account included. This one result is deliberately not gated on user-enumeration settings: those exist to stop people discovering colleagues, and returning you to yourself discloses nothing you do not know. Two of the five new tests fail without the change. Co-Authored-By: Claude Opus 5 (1M context) --- SPECIFICATION.md | 9 +- lib/Controller/EmployeeController.php | 49 ++++++++ .../Controller/EmployeeControllerTest.php | 106 ++++++++++++++++++ 3 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/Controller/EmployeeControllerTest.php diff --git a/SPECIFICATION.md b/SPECIFICATION.md index b8a05a3..158222c 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -103,7 +103,14 @@ calendar forever, with an empty allowance and nothing to show. `GET /api/employees/search` rather than core's autocomplete, because only the server can tell a guest from a colleague. That endpoint wraps the same collaborator search, so the admin's user-enumeration settings still apply - exactly as elsewhere; guests are removed from what it returns. + exactly as elsewhere; guests are removed from what it returns. It does add one + person that search always withholds — **the searching user themselves**, whom the + collaborator search drops because it was built to answer "who can I share with". + Here the question is "whose absence is this?", and there you are a valid answer: + without it an HR member cannot record their own sick leave, since the dialog will + not submit without an employee and the self-service route offers only the + self-requestable types (§5.6). Returning you to yourself discloses nothing, so + this one result is not gated on enumeration settings. - Existing records for someone who later becomes a guest are left untouched in the database — they simply stop being listed. diff --git a/lib/Controller/EmployeeController.php b/lib/Controller/EmployeeController.php index 5633b64..29b9278 100644 --- a/lib/Controller/EmployeeController.php +++ b/lib/Controller/EmployeeController.php @@ -15,6 +15,7 @@ use OCP\AppFramework\Http\DataResponse; use OCP\Collaboration\Collaborators\ISearch; use OCP\IRequest; +use OCP\IUserManager; use OCP\Share\IShare; class EmployeeController extends Controller { @@ -23,6 +24,8 @@ class EmployeeController extends Controller { public function __construct( string $appName, IRequest $request, + private ?string $userId, + private IUserManager $userManager, private ISearch $collaboratorSearch, private EmployeeDirectory $employees, ) { @@ -42,6 +45,15 @@ public function __construct( * enumeration settings continue to apply exactly as they do everywhere else * — with enumeration restricted, that search returns nothing and so does * this. Guests are then removed from whatever it did return. + * + * The one thing that search gets wrong here is *you*: it drops the searching + * user from its own results ({@see \OC\Collaboration\Collaborators\UserPlugin}), + * because it was built to answer "who can I share with" and nobody shares with + * themselves. This picker asks a different question — "whose absence is this?" — + * and there you are a valid answer. Without this, an HR member could not record + * their own sick leave at all: the dialog refuses to submit until an employee is + * chosen, and the only other route, "New request", offers just the + * self-requestable types, which sick leave deliberately is not (§5.6). */ #[NoAdminRequired] #[UserRateLimit(limit: 60, period: 60)] @@ -62,6 +74,12 @@ public function search(string $search = '', int $limit = 20): DataResponse { ); $employees = []; + // First, because somebody typing their own name has matched it exactly and + // should not be pushed below looser matches on it. + $self = $this->matchingSelf($search); + if ($self !== null) { + $employees[$self['uid']] = $self; + } foreach ($candidates as $candidate) { $uid = (string)($candidate['value']['shareWith'] ?? ''); // Exact and wide matches overlap, so the same person can appear twice. @@ -82,4 +100,35 @@ public function search(string $search = '', int $limit = 20): DataResponse { return array_values($employees); }); } + + /** + * The signed-in user, when they match what was typed and may hold leave. + * + * Deliberately not gated on the admin's user-enumeration settings, unlike every + * other result: those exist to stop people discovering *colleagues* they have no + * business seeing, and returning you to yourself discloses nothing you do not + * already know. + * + * @return ?array{uid:string,displayName:string} + */ + private function matchingSelf(string $search): ?array { + $uid = (string)$this->userId; + if ($uid === '') { + return null; + } + $user = $this->userManager->get($uid); + // A guest is not an employee and takes no leave, even when it is their own + // account doing the searching (§2.2). + if ($user === null || !$this->employees->isEmployee($uid)) { + return null; + } + $needle = mb_strtolower($search); + $displayName = $user->getDisplayName(); + foreach ([$uid, $displayName] as $haystack) { + if (str_contains(mb_strtolower($haystack), $needle)) { + return ['uid' => $uid, 'displayName' => $displayName]; + } + } + return null; + } } diff --git a/tests/Unit/Controller/EmployeeControllerTest.php b/tests/Unit/Controller/EmployeeControllerTest.php new file mode 100644 index 0000000..fff7746 --- /dev/null +++ b/tests/Unit/Controller/EmployeeControllerTest.php @@ -0,0 +1,106 @@ +collaboratorSearch = $this->createMock(ISearch::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->employees = $this->createMock(EmployeeDirectory::class); + $this->controller = new EmployeeController( + 'absence', + $this->createMock(IRequest::class), + 'hr-user', + $this->userManager, + $this->collaboratorSearch, + $this->employees, + ); + } + + /** Core's collaborator search never returns the searching user. */ + private function collaboratorSearchReturns(array $users): void { + $this->collaboratorSearch->method('search')->willReturn([['exact' => ['users' => []], 'users' => $users], false]); + } + + private function selfIs(string $displayName): void { + $user = $this->createMock(IUser::class); + $user->method('getDisplayName')->willReturn($displayName); + $this->userManager->method('get')->with('hr-user')->willReturn($user); + } + + /** + * The reported bug: HR could not pick themselves in "Record absence", so they + * could not record their own sick leave — the dialog will not submit without an + * employee, and sick leave is not offered by the self-service route (§5.6). + */ + public function testSearchIncludesTheSigningInUserWhenTheyMatch(): void { + $this->collaboratorSearchReturns([]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + $data = $this->controller->search('frank')->getData(); + + self::assertSame([['uid' => 'hr-user', 'displayName' => 'Frank Karlitschek']], $data); + } + + public function testSearchMatchesTheSigningInUserByUid(): void { + $this->collaboratorSearchReturns([]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + self::assertSame('hr-user', $this->controller->search('hr-us')->getData()[0]['uid']); + } + + public function testSearchLeavesTheSigningInUserOutWhenTheyDoNotMatch(): void { + $this->collaboratorSearchReturns([ + ['label' => 'Lea Meyer', 'value' => ['shareWith' => 'lea']], + ]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + $data = $this->controller->search('lea')->getData(); + + self::assertSame([['uid' => 'lea', 'displayName' => 'Lea Meyer']], $data); + } + + public function testSearchNeverOffersTheSigningInUserWhenTheyAreAGuest(): void { + $this->collaboratorSearchReturns([]); + $this->selfIs('Guest Person'); + // Guests hold no entitlement and take no leave, their own account included. + $this->employees->method('isEmployee')->willReturn(false); + + self::assertSame([], $this->controller->search('guest')->getData()); + } + + public function testSearchDoesNotListTheSigningInUserTwice(): void { + // Should core ever stop filtering self out, the dedup must still hold. + $this->collaboratorSearchReturns([ + ['label' => 'Frank Karlitschek', 'value' => ['shareWith' => 'hr-user']], + ]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + self::assertCount(1, $this->controller->search('frank')->getData()); + } +}