Skip to content

Commit

Permalink
perf: delay getting (sub)admin status for user in the security middle…
Browse files Browse the repository at this point in the history
…ware untill we need it

Signed-off-by: Robin Appelman <[email protected]>
  • Loading branch information
icewind1991 committed Jun 20, 2024
1 parent 392ee62 commit d86fd20
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 61 deletions.
5 changes: 3 additions & 2 deletions lib/private/AppFramework/DependencyInjection/DIContainer.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use OCP\Group\ISubAdmin;
use OCP\IConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IInitialStateService;
use OCP\IL10N;
use OCP\ILogger;
Expand Down Expand Up @@ -226,8 +227,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
$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),
Expand Down
96 changes: 39 additions & 57 deletions lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
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;
Expand All @@ -47,60 +49,40 @@
* check fails
*/
class SecurityMiddleware extends Middleware {
/** @var INavigationManager */
private $navigationManager;
/** @var IRequest */
private $request;
/** @var ControllerMethodReflector */
private $reflector;
/** @var string */
private $appName;
/** @var IURLGenerator */
private $urlGenerator;
/** @var LoggerInterface */
private $logger;
/** @var bool */
private $isLoggedIn;
/** @var bool */
private $isAdminUser;
/** @var bool */
private $isSubAdmin;
/** @var IAppManager */
private $appManager;
/** @var IL10N */
private $l10n;
/** @var AuthorizedGroupMapper */
private $groupAuthorizationMapper;
/** @var IUserSession */
private $userSession;
private ?bool $isAdminUser = null;
private ?bool $isSubAdmin = null;

public function __construct(IRequest $request,
ControllerMethodReflector $reflector,
INavigationManager $navigationManager,
IURLGenerator $urlGenerator,
LoggerInterface $logger,
string $appName,
bool $isLoggedIn,
bool $isAdminUser,
bool $isSubAdmin,
IAppManager $appManager,
IL10N $l10n,
AuthorizedGroupMapper $mapper,
IUserSession $userSession
public function __construct(
private IRequest $request,
private ControllerMethodReflector $reflector,
private INavigationManager $navigationManager,
private IURLGenerator $urlGenerator,
private LoggerInterface $logger,
private string $appName,
private bool $isLoggedIn,
private IGroupManager $groupManager,
private ISubAdmin $subAdminManager,
private IAppManager $appManager,
private IL10N $l10n,
private AuthorizedGroupMapper $groupAuthorizationMapper,
private IUserSession $userSession,
) {
$this->navigationManager = $navigationManager;
$this->request = $request;
$this->reflector = $reflector;
$this->appName = $appName;
$this->urlGenerator = $urlGenerator;
$this->logger = $logger;
$this->isLoggedIn = $isLoggedIn;
$this->isAdminUser = $isAdminUser;
$this->isSubAdmin = $isSubAdmin;
$this->appManager = $appManager;
$this->l10n = $l10n;
$this->groupAuthorizationMapper = $mapper;
$this->userSession = $userSession;
}

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;
}

/**
Expand Down Expand Up @@ -133,10 +115,10 @@ public function beforeController($controller, $methodName) {
}
$authorized = false;
if ($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) {
Expand All @@ -155,14 +137,14 @@ public function beforeController($controller, $methodName) {
}
}
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'));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@
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\IURLGenerator;
use OCP\IUser;
use OCP\IUserSession;
use Psr\Log\LoggerInterface;
use Test\AppFramework\Middleware\Security\Mock\NormalController;
Expand Down Expand Up @@ -67,6 +70,9 @@ protected function setUp(): void {

$this->authorizedGroupMapper = $this->createMock(AuthorizedGroupMapper::class);
$this->userSession = $this->createMock(IUserSession::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',
Expand All @@ -88,6 +94,13 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA
->method('isEnabledForUser')
->willReturn($isAppEnabledForUser);

$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,
Expand All @@ -96,8 +109,8 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA
$this->logger,
'files',
$isLoggedIn,
$isAdminUser,
$isSubAdmin,
$groupManager,
$subAdminManager,
$this->appManager,
$this->l10n,
$this->authorizedGroupMapper,
Expand Down

0 comments on commit d86fd20

Please sign in to comment.