Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OCA\Provisioning_API\Middleware\Exceptions\NotSubAdminException;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Middleware;
use OCP\AppFramework\OCS\OCSException;
Expand Down Expand Up @@ -40,7 +41,7 @@ public function __construct(
*/
public function beforeController($controller, $methodName) {
// If AuthorizedAdminSetting, the check will be done in the SecurityMiddleware
if (!$this->isAdmin && !$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->isSubAdmin && !$this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
if (!$this->isAdmin && !$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->isSubAdmin && !$this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
throw new NotSubAdminException();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,14 @@ public function testBeforeController(bool $subadminRequired, bool $isAdmin, bool
);

$this->reflector->method('hasAnnotation')
->willReturnCallback(function ($annotation) use ($subadminRequired, $hasSettingAuthorizationAnnotation) {
->willReturnCallback(function ($annotation) use ($subadminRequired) {
if ($annotation === 'NoSubAdminRequired') {
return !$subadminRequired;
}
return false;
});
$this->reflector->method('hasAnnotationOrAttribute')
->willReturnCallback(function ($annotation, $attribute) use ($hasSettingAuthorizationAnnotation) {
if ($annotation === 'AuthorizedAdminSetting') {
return $hasSettingAuthorizationAnnotation;
}
Expand Down
3 changes: 2 additions & 1 deletion apps/settings/lib/Middleware/SubadminMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException;
use OC\AppFramework\Utility\ControllerMethodReflector;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Middleware;
use OCP\Group\ISubAdmin;
Expand Down Expand Up @@ -48,7 +49,7 @@ private function isSubAdmin(): bool {
* @throws \Exception
*/
public function beforeController($controller, $methodName) {
if (!$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
if (!$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
if (!$this->isSubAdmin()) {
throw new NotAdminException($this->l10n->t('Logged in account must be a sub admin'));
}
Expand Down
19 changes: 15 additions & 4 deletions apps/settings/tests/Middleware/SubadminMiddlewareTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OC\AppFramework\Utility\ControllerMethodReflector;
use OCA\Settings\Middleware\SubadminMiddleware;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\Group\ISubAdmin;
use OCP\IL10N;
Expand Down Expand Up @@ -62,11 +63,16 @@ public function testBeforeControllerAsUserWithoutAnnotation(): void {
$this->expectException(NotAdminException::class);

$this->reflector
->expects($this->exactly(2))
->expects($this->exactly(1))
->method('hasAnnotation')
->willReturnMap([
['NoSubAdminRequired', false],
['AuthorizedAdminSetting', false],
]);
$this->reflector
->expects($this->exactly(1))
->method('hasAnnotationOrAttribute')
->willReturnMap([
['AuthorizedAdminSetting', AuthorizedAdminSetting::class, false],
]);

$this->subAdminManager
Expand Down Expand Up @@ -94,11 +100,16 @@ public function testBeforeControllerWithAnnotation(): void {

public function testBeforeControllerAsSubAdminWithoutAnnotation(): void {
$this->reflector
->expects($this->exactly(2))
->expects($this->exactly(1))
->method('hasAnnotation')
->willReturnMap([
['NoSubAdminRequired', false],
['AuthorizedAdminSetting', false],
]);
$this->reflector
->expects($this->exactly(1))
->method('hasAnnotationOrAttribute')
->willReturnMap([
['AuthorizedAdminSetting', AuthorizedAdminSetting::class, false],
]);

$this->subAdminManager
Expand Down
9 changes: 0 additions & 9 deletions build/psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1657,9 +1657,6 @@
</DeprecatedMethod>
</file>
<file src="apps/files_sharing/lib/Middleware/SharingCheckMiddleware.php">
<DeprecatedInterface>
<code><![CDATA[protected]]></code>
</DeprecatedInterface>
<DeprecatedMethod>
<code><![CDATA[getAppValue]]></code>
<code><![CDATA[getAppValue]]></code>
Expand Down Expand Up @@ -2010,9 +2007,6 @@
</DeprecatedMethod>
</file>
<file src="apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php">
<DeprecatedInterface>
<code><![CDATA[IControllerMethodReflector]]></code>
</DeprecatedInterface>
<DeprecatedMethod>
<code><![CDATA[hasAnnotation]]></code>
<code><![CDATA[hasAnnotation]]></code>
Expand Down Expand Up @@ -3213,9 +3207,6 @@
</DeprecatedMethod>
</file>
<file src="core/Middleware/TwoFactorMiddleware.php">
<DeprecatedInterface>
<code><![CDATA[private]]></code>
</DeprecatedInterface>
<DeprecatedMethod>
<code><![CDATA[hasAnnotation]]></code>
<code><![CDATA[hasAnnotation]]></code>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
use OCP\ISession;
use OCP\IUserSession;
use Psr\Log\LoggerInterface;
use ReflectionMethod;

// Will close the session if the user session is ephemeral.
// Happens when the user logs in via the login flow v2.
Expand Down Expand Up @@ -61,12 +60,7 @@ public function beforeController(Controller $controller, string $methodName) {
return;
}

$reflectionMethod = new ReflectionMethod($controller, $methodName);
if (!empty($reflectionMethod->getAttributes(PublicPage::class))) {
return;
}

if ($this->reflector->hasAnnotation('PublicPage')) {
if ($this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class)) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ public function __construct(
* @throws NotConfirmedException
*/
public function beforeController(Controller $controller, string $methodName) {
$reflectionMethod = new ReflectionMethod($controller, $methodName);

if (!$this->needsPasswordConfirmation($reflectionMethod)) {
if (!$this->needsPasswordConfirmation()) {
return;
}

Expand Down Expand Up @@ -78,6 +76,7 @@ public function beforeController(Controller $controller, string $methodName) {
return;
}

$reflectionMethod = new ReflectionMethod($controller, $methodName);
if ($this->isPasswordConfirmationStrict($reflectionMethod)) {
$authHeader = $this->request->getHeader('Authorization');
if (!str_starts_with(strtolower($authHeader), 'basic ')) {
Expand All @@ -100,18 +99,8 @@ public function beforeController(Controller $controller, string $methodName) {
}
}

private function needsPasswordConfirmation(ReflectionMethod $reflectionMethod): bool {
$attributes = $reflectionMethod->getAttributes(PasswordConfirmationRequired::class);
if (!empty($attributes)) {
return true;
}

if ($this->reflector->hasAnnotation('PasswordConfirmationRequired')) {
$this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . 'PasswordConfirmationRequired' . ' annotation and should use the #[PasswordConfirmationRequired] attribute instead');
return true;
}

return false;
private function needsPasswordConfirmation(): bool {
return $this->reflector->hasAnnotationOrAttribute('PasswordConfirmationRequired', PasswordConfirmationRequired::class);
}

private function isPasswordConfirmationStrict(ReflectionMethod $reflectionMethod): bool {
Expand Down
27 changes: 2 additions & 25 deletions lib/private/AppFramework/Middleware/SessionMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Middleware;
use OCP\ISession;
use ReflectionMethod;

class SessionMiddleware extends Middleware {
/** @var ControllerMethodReflector */
Expand All @@ -34,18 +33,7 @@ public function __construct(ControllerMethodReflector $reflector,
* @param string $methodName
*/
public function beforeController($controller, $methodName) {
/**
* Annotation deprecated with Nextcloud 26
*/
$hasAnnotation = $this->reflector->hasAnnotation('UseSession');
if ($hasAnnotation) {
$this->session->reopen();
return;
}

$reflectionMethod = new ReflectionMethod($controller, $methodName);
$hasAttribute = !empty($reflectionMethod->getAttributes(UseSession::class));
if ($hasAttribute) {
if ($this->reflector->hasAnnotationOrAttribute('UseSession', UseSession::class)) {
$this->session->reopen();
}
}
Expand All @@ -57,18 +45,7 @@ public function beforeController($controller, $methodName) {
* @return Response
*/
public function afterController($controller, $methodName, Response $response) {
/**
* Annotation deprecated with Nextcloud 26
*/
$hasAnnotation = $this->reflector->hasAnnotation('UseSession');
if ($hasAnnotation) {
$this->session->close();
return $response;
}

$reflectionMethod = new ReflectionMethod($controller, $methodName);
$hasAttribute = !empty($reflectionMethod->getAttributes(UseSession::class));
if ($hasAttribute) {
if ($this->reflector->hasAnnotationOrAttribute('UseSession', UseSession::class)) {
$this->session->close();
}

Expand Down
41 changes: 35 additions & 6 deletions lib/private/AppFramework/Utility/ControllerMethodReflector.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,34 @@
namespace OC\AppFramework\Utility;

use OCP\AppFramework\Utility\IControllerMethodReflector;
use Psr\Log\LoggerInterface;

/**
* Reads and parses annotations from doc comments
*/
class ControllerMethodReflector implements IControllerMethodReflector {
public $annotations = [];
private $types = [];
private $parameters = [];
public array $annotations = [];
private array $types = [];
private array $parameters = [];
private array $ranges = [];
private ?\ReflectionMethod $reflectionMethod = null;

public function __construct(
private readonly LoggerInterface $logger,
) {
}

/**
* @param object $object an object or classname
* @param string $method the method which we want to inspect
*/
public function reflect($object, string $method) {
$reflection = new \ReflectionMethod($object, $method);
$docs = $reflection->getDocComment();
$this->annotations = [];
$this->types = [];
$this->parameters = [];
$this->ranges = [];
$this->reflectionMethod = new \ReflectionMethod($object, $method);
$docs = $this->reflectionMethod->getDocComment();

if ($docs !== false) {
// extract everything prefixed by @ and first letter uppercase
Expand Down Expand Up @@ -64,7 +75,7 @@ public function reflect($object, string $method) {
}
}

foreach ($reflection->getParameters() as $param) {
foreach ($this->reflectionMethod->getParameters() as $param) {
// extract type information from PHP 7 scalar types and prefer them over phpdoc annotations
$type = $param->getType();
if ($type instanceof \ReflectionNamedType) {
Expand Down Expand Up @@ -109,6 +120,24 @@ public function getParameters(): array {
return $this->parameters;
}

/**
* @template T
*
* @param class-string<T> $attributeClass
*/
public function hasAnnotationOrAttribute(?string $annotationName, string $attributeClass): bool {
if (!empty($this->reflectionMethod->getAttributes($attributeClass))) {
return true;
}

if ($annotationName && $this->hasAnnotation($annotationName)) {
$this->logger->debug($this->reflectionMethod->getDeclaringClass()->getName() . '::' . $this->reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead');
return true;
}

return false;
}

/**
* Check if a method contains an annotation
* @param string $name the name of the annotation
Expand Down
26 changes: 14 additions & 12 deletions lib/public/AppFramework/Utility/IControllerMethodReflector.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,13 @@
/**
* Interface ControllerMethodReflector
*
* Reads and parses annotations from doc comments
* You can inject this interface in your Middleware, and it will be prefilled with information related to the called controller method
*
* Reads and parses annotations from doc comments (deprecated) and PHP attributes
*
* @since 8.0.0
* @deprecated 22.0.0 will be obsolete with native attributes in PHP8
* @see https://help.nextcloud.com/t/how-should-we-use-php8-attributes/104278
*/
interface IControllerMethodReflector {
/**
* @param object $object an object or classname
* @param string $method the method which we want to inspect
* @return void
* @since 8.0.0
* @deprecated 17.0.0 Reflect should not be called multiple times and only be used internally. This will be removed in Nextcloud 18
*/
public function reflect($object, string $method);

/**
* Inspects the PHPDoc parameters for types
*
Expand Down Expand Up @@ -56,4 +47,15 @@ public function getParameters(): array;
* @see https://help.nextcloud.com/t/how-should-we-use-php8-attributes/104278
*/
public function hasAnnotation(string $name): bool;

/**
* @template T
*
* Check if a method contains an annotation or an attribute.
* Log a debug line if the annotation is used.
*
* @param class-string<T> $attributeClass
* @since 34.0.0
*/
public function hasAnnotationOrAttribute(?string $annotationName, string $attributeClass): bool;
}
2 changes: 1 addition & 1 deletion tests/lib/AppFramework/Http/DispatcherTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ protected function setUp(): void {

$this->request = $this->createMock(Request::class);

$this->reflector = new ControllerMethodReflector();
$this->reflector = new ControllerMethodReflector(\OCP\Server::get(LoggerInterface::class));

$this->dispatcher = new Dispatcher(
$this->http,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class BruteForceMiddlewareTest extends TestCase {
protected function setUp(): void {
parent::setUp();

$this->reflector = new ControllerMethodReflector();
$this->reflector = new ControllerMethodReflector(\OCP\Server::get(LoggerInterface::class));
$this->throttler = $this->createMock(IThrottler::class);
$this->request = $this->createMock(IRequest::class);
$this->logger = $this->createMock(LoggerInterface::class);
Expand Down
Loading
Loading