Skip to content

Commit 3fc6502

Browse files
CarlSchwanCarlSchwan
authored
Merge pull request #1076 from nextcloud/carl/attribute
refactor: Use modern attribute instead of annotation
2 parents f5a5509 + b0c39e1 commit 3fc6502

5 files changed

Lines changed: 52 additions & 43 deletions

File tree

‎lib/Attributes/OnlyUnauthenticatedUsers.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
// SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
6+
// SPDX-License-Identifier: AGPL-3.0-or-later
7+
8+
namespace OCA\User_SAML\Attributes;
9+
10+
use Attribute;
11+
12+
#[Attribute(Attribute::TARGET_METHOD)]
13+
class OnlyUnauthenticatedUsers {
14+
}

‎lib/Controller/SAMLController.php‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OC\Core\Controller\ClientFlowLoginController;
1414
use OC\Core\Controller\ClientFlowLoginV2Controller;
1515
use OC\Security\CSRF\CsrfTokenManager;
16+
use OCA\User_SAML\Attributes\OnlyUnauthenticatedUsers;
1617
use OCA\User_SAML\Exceptions\NoUserFoundException;
1718
use OCA\User_SAML\Exceptions\UserFilterViolationException;
1819
use OCA\User_SAML\Helper\TXmlHelper;
@@ -145,12 +146,12 @@ protected function assertGroupMemberships(): void {
145146
}
146147

147148
/**
148-
* @OnlyUnauthenticatedUsers
149149
* @throws Exception
150150
*/
151151
#[PublicPage]
152152
#[UseSession]
153153
#[NoCSRFRequired]
154+
#[OnlyUnauthenticatedUsers]
154155
public function login(int $idp = 1): Http\RedirectResponse|Http\TemplateResponse {
155156
$originalUrl = (string)$this->request->getParam('originalUrl', '');
156157
if (!$this->trustedDomainHelper->isTrustedUrl($originalUrl)) {
@@ -287,6 +288,7 @@ public function login(int $idp = 1): Http\RedirectResponse|Http\TemplateResponse
287288
*/
288289
#[PublicPage]
289290
#[NoCSRFRequired]
291+
#[OnlyUnauthenticatedUsers]
290292
public function getMetadata(int $idp = 1): Http\DataDownloadResponse {
291293
$settings = new Settings($this->samlSettings->getOneLoginSettingsArray($idp));
292294
$metadata = $settings->getSPMetadata();
@@ -302,7 +304,6 @@ public function getMetadata(int $idp = 1): Http\DataDownloadResponse {
302304
}
303305

304306
/**
305-
* @OnlyUnauthenticatedUsers
306307
* @NoSameSiteCookieRequired
307308
*
308309
* @return Http\RedirectResponse
@@ -312,6 +313,7 @@ public function getMetadata(int $idp = 1): Http\DataDownloadResponse {
312313
#[PublicPage]
313314
#[NoCSRFRequired]
314315
#[UseSession]
316+
#[OnlyUnauthenticatedUsers]
315317
public function assertionConsumerService(): Http\RedirectResponse {
316318
// Fetch and decrypt the cookie
317319
$cookie = $this->request->getCookie('saml_data');
@@ -536,41 +538,33 @@ private function tryProcessSLOResponse(?int $idp): array {
536538
return [null, null];
537539
}
538540

539-
/**
540-
* @OnlyUnauthenticatedUsers
541-
*/
542541
#[PublicPage]
543542
#[NoCSRFRequired]
543+
#[OnlyUnauthenticatedUsers]
544544
public function notProvisioned(): Http\TemplateResponse {
545545
return new Http\TemplateResponse($this->appName, 'notProvisioned', [], 'guest');
546546
}
547547

548-
/**
549-
* @OnlyUnauthenticatedUsers
550-
*/
551548
#[PublicPage]
552549
#[NoCSRFRequired]
550+
#[OnlyUnauthenticatedUsers]
553551
public function notPermitted(): Http\TemplateResponse {
554552
return new Http\TemplateResponse($this->appName, 'notPermitted', [], 'guest');
555553
}
556554

557-
/**
558-
* @OnlyUnauthenticatedUsers
559-
*/
560555
#[PublicPage]
561556
#[NoCSRFRequired]
557+
#[OnlyUnauthenticatedUsers]
562558
public function genericError(string $message): Http\TemplateResponse {
563559
if (empty($message)) {
564560
$message = $this->l->t('Unknown error, please check the log file for more details.');
565561
}
566562
return new Http\TemplateResponse($this->appName, 'error', ['message' => $message], 'guest');
567563
}
568564

569-
/**
570-
* @OnlyUnauthenticatedUsers
571-
*/
572565
#[PublicPage]
573566
#[NoCSRFRequired]
567+
#[OnlyUnauthenticatedUsers]
574568
public function selectUserBackEnd(string $redirectUrl = ''): Http\TemplateResponse {
575569
$attributes = ['loginUrls' => []];
576570

‎lib/Middleware/OnlyLoggedInMiddleware.php‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77

88
namespace OCA\User_SAML\Middleware;
99

10+
use OCA\User_SAML\Attributes\OnlyUnauthenticatedUsers;
1011
use OCP\AppFramework\Http\RedirectResponse;
1112
use OCP\AppFramework\Middleware;
12-
use OCP\AppFramework\Utility\IControllerMethodReflector;
1313
use OCP\IURLGenerator;
1414
use OCP\IUserSession;
1515
use Override;
@@ -23,7 +23,6 @@
2323
class OnlyLoggedInMiddleware extends Middleware {
2424

2525
public function __construct(
26-
private readonly IControllerMethodReflector $reflector,
2726
private readonly IUserSession $userSession,
2827
private readonly IURLGenerator $urlGenerator,
2928
) {
@@ -36,7 +35,7 @@ public function __construct(
3635
*/
3736
#[Override]
3837
public function beforeController($controller, $methodName): void {
39-
if ($this->reflector->hasAnnotation('OnlyUnauthenticatedUsers') && $this->userSession->isLoggedIn()) {
38+
if ($this->hasAttribute($controller, $methodName) && $this->userSession->isLoggedIn()) {
4039
throw new \Exception('User is already logged-in');
4140
}
4241
}
@@ -55,4 +54,9 @@ public function afterException($controller, $methodName, \Exception $exception):
5554

5655
throw $exception;
5756
}
57+
58+
protected function hasAttribute(object $controller, string $methodName): bool {
59+
$reflectionMethod = new \ReflectionMethod($controller, $methodName);
60+
return !empty($reflectionMethod->getAttributes(OnlyUnauthenticatedUsers::class));
61+
}
5862
}

‎tests/psalm-baseline.xml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,6 @@
99
<code><![CDATA[GroupBackend]]></code>
1010
</DeprecatedInterface>
1111
</file>
12-
<file src="lib/Middleware/OnlyLoggedInMiddleware.php">
13-
<DeprecatedMethod>
14-
<code><![CDATA[hasAnnotation]]></code>
15-
</DeprecatedMethod>
16-
</file>
1712
<file src="lib/UserBackend.php">
1813
<DeprecatedInterface>
1914
<code><![CDATA[UserBackend]]></code>

‎tests/unit/Middleware/OnlyLoggedInMiddlewareTest.php‎

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,64 +11,66 @@
1111
use OCA\User_SAML\Middleware\OnlyLoggedInMiddleware;
1212
use OCP\AppFramework\Controller;
1313
use OCP\AppFramework\Http\RedirectResponse;
14-
use OCP\AppFramework\Utility\IControllerMethodReflector;
1514
use OCP\IURLGenerator;
1615
use OCP\IUserSession;
1716
use Override;
1817
use PHPUnit\Framework\MockObject\MockObject;
1918

2019
class OnlyLoggedInMiddlewareTest extends \Test\TestCase {
2120
protected IURLGenerator&MockObject $urlGenerator;
22-
private IControllerMethodReflector&MockObject $reflector;
2321
private IUserSession&MockObject $userSession;
24-
private OnlyLoggedInMiddleware $onlyLoggedInMiddleware;
22+
private OnlyLoggedInMiddleware&MockObject $onlyLoggedInMiddleware;
2523

2624
#[Override]
2725
protected function setUp(): void {
28-
$this->reflector = $this->createMock(IControllerMethodReflector::class);
2926
$this->userSession = $this->createMock(IUserSession::class);
3027
$this->urlGenerator = $this->createMock(IURLGenerator::class);
31-
$this->onlyLoggedInMiddleware = new OnlyLoggedInMiddleware(
32-
$this->reflector,
33-
$this->userSession,
34-
$this->urlGenerator
35-
);
28+
$this->onlyLoggedInMiddleware = $this->getMockBuilder(OnlyLoggedInMiddleware::class)
29+
->setConstructorArgs([
30+
$this->userSession,
31+
$this->urlGenerator,
32+
])
33+
->onlyMethods(['hasAttribute'])
34+
->getMock();
3635

3736
parent::setUp();
3837
}
3938

4039
public function testBeforeControllerWithoutAnnotation(): void {
41-
$this->reflector
40+
$controller = $this->createMock(Controller::class);
41+
$this->onlyLoggedInMiddleware
4242
->expects($this->once())
43-
->method('hasAnnotation')
44-
->with('OnlyUnauthenticatedUsers')
43+
->method('hasAttribute')
44+
->with($controller, 'bar')
4545
->willReturn(false);
4646
$this->userSession
4747
->expects($this->never())
4848
->method('isLoggedIn');
4949

50-
$this->onlyLoggedInMiddleware->beforeController($this->createMock(Controller::class), 'bar');
50+
$this->onlyLoggedInMiddleware->beforeController($controller, 'bar');
5151
}
5252

5353
public function testBeforeControllerWithAnnotationAndNotLoggedIn(): void {
54-
$this->reflector
54+
$controller = $this->createMock(Controller::class);
55+
$this->onlyLoggedInMiddleware
5556
->expects($this->once())
56-
->method('hasAnnotation')
57-
->with('OnlyUnauthenticatedUsers')
57+
->method('hasAttribute')
58+
->with($controller, 'bar')
5859
->willReturn(true);
5960
$this->userSession
6061
->expects($this->once())
6162
->method('isLoggedIn')
6263
->willReturn(false);
6364

64-
$this->onlyLoggedInMiddleware->beforeController($this->createMock(Controller::class), 'bar');
65+
$this->onlyLoggedInMiddleware->beforeController($controller, 'bar');
6566
}
6667

6768
public function testBeforeControllerWithAnnotationAndLoggedIn(): void {
68-
$this->reflector
69+
$controller = $this->createMock(Controller::class);
70+
$this->onlyLoggedInMiddleware
6971
->expects($this->once())
70-
->method('hasAnnotation')
71-
->with('OnlyUnauthenticatedUsers')
72+
->method('hasAttribute')
73+
->with($controller, 'bar')
7274
->willReturn(true);
7375
$this->userSession
7476
->expects($this->once())
@@ -78,7 +80,7 @@ public function testBeforeControllerWithAnnotationAndLoggedIn(): void {
7880
$this->expectException(Exception::class);
7981
$this->expectExceptionMessage('User is already logged-in');
8082

81-
$this->onlyLoggedInMiddleware->beforeController($this->createMock(Controller::class), 'bar');
83+
$this->onlyLoggedInMiddleware->beforeController($controller, 'bar');
8284
}
8385

8486
public function testAfterExceptionWithNormalException(): void {

0 commit comments

Comments
 (0)