sascha
/
nextcloud-server
mirror of https://github.com/nextcloud/server.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

perf: delay getting (sub)admin status for user in the security middleware untill we need it 

Signed-off-by: Robin Appelman <robin@icewind.nl>
pull/46021/head
Robin Appelman 2024-06-20 18:44:52 +07:00
parent 0cab17bfe7
commit 8b60df1600
No known key found for this signature in database
GPG Key ID: 42B69D8A64526EFB
3 changed files with 46 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
lib/private/AppFramework/DependencyInjection/DIContainer.php
Unescape Escape View File

@ -36,6 +36,7 @@ use OCP\Files\IAppData;
use OCP\Group\ISubAdmin;
use OCP\IConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IInitialStateService;
use OCP\IL10N;
use OCP\ILogger;
@ -228,8 +229,8 @@ class DIContainer extends SimpleContainer implements IAppContainer {
$server->get(LoggerInterface::class),
$c->get('AppName'),
$server->getUserSession()->isLoggedIn(),
$this->getUserId() !== null && $server->getGroupManager()->isAdmin($this->getUserId()),
$server->getUserSession()->getUser() !== null && $server->query(ISubAdmin::class)->isSubAdmin($server->getUserSession()->getUser()),
$c->get(IGroupManager::class),
$c->get(ISubAdmin::class),
$server->getAppManager(),
$server->getL10N('lib'),
$c->get(AuthorizedGroupMapper::class),

35
lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
Unescape Escape View File

@ -36,6 +36,8 @@ use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Middleware;
use OCP\AppFramework\OCSController;
use OCP\Group\ISubAdmin;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\INavigationManager;
use OCP\IRequest;
@ -53,6 +55,9 @@ use ReflectionMethod;
* check fails
*/
class SecurityMiddleware extends Middleware {
private ?bool $isAdminUser = null;
private ?bool $isSubAdmin = null;
public function __construct(
private IRequest $request,
private ControllerMethodReflector $reflector,
@ -61,8 +66,8 @@ class SecurityMiddleware extends Middleware {
private LoggerInterface $logger,
private string $appName,
private bool $isLoggedIn,
private bool $isAdminUser,
private bool $isSubAdmin,
private IGroupManager $groupManager,
private ISubAdmin $subAdminManager,
private IAppManager $appManager,
private IL10N $l10n,
private AuthorizedGroupMapper $groupAuthorizationMapper,
@ -71,6 +76,22 @@ class SecurityMiddleware extends Middleware {
) {
}
private function isAdminUser(): bool {
if ($this->isAdminUser === null) {
$user = $this->userSession->getUser();
$this->isAdminUser = $user && $this->groupManager->isAdmin($user->getUID());
}
return $this->isAdminUser;
}
private function isSubAdmin(): bool {
if ($this->isSubAdmin === null) {
$user = $this->userSession->getUser();
$this->isSubAdmin = $user && $this->subAdminManager->isSubAdmin($user);
}
return $this->isSubAdmin;
}
/**
* This runs all the security checks before a method call. The
* security checks are determined by inspecting the controller method
@ -114,10 +135,10 @@ class SecurityMiddleware extends Middleware {
}
if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
$authorized = $this->isAdminUser;
$authorized = $this->isAdminUser();
if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) {
$authorized = $this->isSubAdmin;
$authorized = $this->isSubAdmin();
}
if (!$authorized) {
@ -139,14 +160,14 @@ class SecurityMiddleware extends Middleware {
}
}
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
&& !$this->isSubAdmin
&& !$this->isAdminUser
&& !$this->isSubAdmin()
&& !$this->isAdminUser()
&& !$authorized) {
throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin'));
}
if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
&& !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class)
&& !$this->isAdminUser
&& !$this->isAdminUser()
&& !$authorized) {
throw new NotAdminException($this->l10n->t('Logged in account must be an admin'));
}

17
tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
Unescape Escape View File

@ -24,13 +24,16 @@ use OCP\App\IAppManager;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\Group\ISubAdmin;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\INavigationManager;
use OCP\IRequest;
use OCP\IRequestId;
use OCP\ISession;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Security\Ip\IRemoteAddress;
use Psr\Log\LoggerInterface;
@ -71,6 +74,9 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->authorizedGroupMapper = $this->createMock(AuthorizedGroupMapper::class);
$this->userSession = $this->createMock(Session::class);
$user = $this->createMock(IUser::class);
$user->method('getUID')->willReturn('test');
$this->userSession->method('getUser')->willReturn($user);
$this->request = $this->createMock(IRequest::class);
$this->controller = new SecurityMiddlewareController(
'test',
@ -94,6 +100,13 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$remoteIpAddress = $this->createMock(IRemoteAddress::class);
$remoteIpAddress->method('allowsAdminActions')->willReturn(true);
$groupManager = $this->createMock(IGroupManager::class);
$groupManager->method('isAdmin')
->willReturn($isAdminUser);
$subAdminManager = $this->createMock(ISubAdmin::class);
$subAdminManager->method('isSubAdmin')
->willReturn($isSubAdmin);
return new SecurityMiddleware(
$this->request,
$this->reader,
@ -102,8 +115,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->logger,
'files',
$isLoggedIn,
$isAdminUser,
$isSubAdmin,
$groupManager,
$subAdminManager,
$this->appManager,
$this->l10n,
$this->authorizedGroupMapper,