Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: redirect to login when validation page is not public #4491

Merged
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
29 changes: 22 additions & 7 deletions lib/Controller/PageController.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

namespace OCA\Libresign\Controller;

use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException;
use OCA\Libresign\AppInfo\Application;
use OCA\Libresign\Exception\LibresignException;
use OCA\Libresign\Helper\JSActions;
Expand All @@ -34,7 +33,6 @@
use OCP\AppFramework\Http\ContentSecurityPolicy;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\FileDisplayResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\EventDispatcher\IEventDispatcher;
Expand All @@ -61,7 +59,7 @@ public function __construct(
private FileService $fileService,
private ValidateHelper $validateHelper,
private IEventDispatcher $eventDispatcher,
private IURLGenerator $url,
private IURLGenerator $urlGenerator,
) {
parent::__construct(
request: $request,
Expand Down Expand Up @@ -496,7 +494,7 @@ public function validation(): TemplateResponse {
* The path is used only by frontend
*
* @param string $uuid Sign request uuid
* @return RedirectResponse<Http::STATUS_SEE_OTHER, array{}>
* @return TemplateResponse<Http::STATUS_OK, array{}>
*
* 303: Redirected to validation page
* 401: Validation page not accessible if unauthenticated
Expand All @@ -507,9 +505,9 @@ public function validation(): TemplateResponse {
#[PublicPage]
#[AnonRateLimit(limit: 30, period: 60)]
#[FrontpageRoute(verb: 'GET', url: '/validation/{uuid}')]
public function validationFileWithShortUrl(): RedirectResponse {
public function validationFileWithShortUrl(): TemplateResponse {
$this->throwIfValidationPageNotAccessible();
return new RedirectResponse($this->url->linkToRoute('libresign.page.validationFilePublic', ['uuid' => $this->request->getParam('uuid')]));
return $this->validationFilePublic($this->request->getParam('uuid'));
}

/**
Expand Down Expand Up @@ -607,7 +605,24 @@ private function throwIfValidationPageNotAccessible(): void {
return;
}
if ($isValidationUrlPrivate) {
throw new NotLoggedInException();
if ($uuid = $this->request->getParam('uuid')) {
$redirectUrl = $this->urlGenerator->linkToRoute(
'libresign.page.validationFilePublic',
['uuid' => $uuid]
);
} else {
$redirectUrl = $this->urlGenerator->linkToRoute(
'libresign.page.validation',
);
}

throw new LibresignException(json_encode([
'action' => JSActions::ACTION_REDIRECT,
'errors' => [$this->l10n->t('You are not logged in. Please log in.')],
'redirect' => $this->urlGenerator->linkToRoute('core.login.showLoginForm', [
'redirect_url' => $redirectUrl,
]),
]), Http::STATUS_UNAUTHORIZED);
}
}
}
14 changes: 13 additions & 1 deletion lib/Middleware/InjectionMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@
use OCP\AppFramework\Http\ContentSecurityPolicy;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Middleware;
use OCP\AppFramework\Services\IInitialState;
use OCP\IL10N;
use OCP\IRequest;
use OCP\ISession;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Util;
Expand All @@ -45,6 +47,7 @@ class InjectionMiddleware extends Middleware {

public function __construct(
private IRequest $request,
private ISession $session,
private IUserSession $userSession,
private ValidateHelper $validateHelper,
private SignRequestMapper $signRequestMapper,
Expand Down Expand Up @@ -173,7 +176,16 @@ public function afterException($controller, $methodName, \Exception $exception):
if (str_contains($this->request->getHeader('Accept'), 'html')) {
$template = 'external';
if ($this->isJson($exception->getMessage())) {
foreach (json_decode($exception->getMessage(), true) as $key => $value) {
$settings = json_decode($exception->getMessage(), true);
if (isset($settings['action']) && $settings['action'] === JSActions::ACTION_REDIRECT && isset($settings['redirect'])) {
if (isset($settings['errors'])) {
$this->session->set('loginMessages', [
[], $settings['errors'],
]);
}
return new RedirectResponse($settings['redirect']);
}
foreach ($settings as $key => $value) {
if ($key === 'template') {
$template = $value;
continue;
Expand Down
26 changes: 26 additions & 0 deletions tests/Unit/Middleware/InjectionMiddlewareTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
use OCA\Libresign\Service\SignFileService;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IServerContainer;
use OCP\ISession;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
Expand All @@ -31,6 +33,7 @@

final class InjectionMiddlewareTest extends \OCA\Libresign\Tests\Unit\TestCase {
private IRequest&MockObject $request;
private ISession&MockObject $session;
private IUserSession&MockObject $userSession;
private ValidateHelper&MockObject $validateHelper;
private SignRequestMapper&MockObject $signRequestMapper;
Expand All @@ -45,6 +48,7 @@ final class InjectionMiddlewareTest extends \OCA\Libresign\Tests\Unit\TestCase {

public function setUp(): void {
$this->request = $this->createMock(IRequest::class);
$this->session = $this->createMock(ISession::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->validateHelper = $this->createMock(ValidateHelper::class);
$this->signRequestMapper = $this->createMock(SignRequestMapper::class);
Expand All @@ -65,6 +69,7 @@ public function setUp(): void {
public function getInjectionMiddleware(): InjectionMiddleware {
return new InjectionMiddleware(
$this->request,
$this->session,
$this->userSession,
$this->validateHelper,
$this->signRequestMapper,
Expand Down Expand Up @@ -189,6 +194,27 @@ function (self $self, $message, int $code, $actual):void {
);
},
],
[
json_encode(['action' => 1000, 'redirect' => 'http://fake.url']), 1, PageException::class,
function (self $self, $message, int $code, $actual):void {
/** @var RedirectResponse $actual */
$self->assertInstanceOf(
RedirectResponse::class,
$actual,
'The response need to be RedirectResponse'
);
$self->assertEquals(
'http://fake.url',
$actual->getRedirectURL(),
'Invalid redirect URL'
);
$self->assertEquals(
303,
$actual->getStatus(),
'Invalid response status code'
);
},
],
];
}
}
7 changes: 5 additions & 2 deletions tests/integration/features/page/validate.feature
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Feature: page/validate
Background: Make setup ok
Given run the command "config:app:set libresign authkey --value=dummy" with result code 0
And run the command "libresign:configure:openssl --cn test" with result code 0

Scenario: Unauthenticated user can see sign page
Given as user "admin"
Expand All @@ -16,5 +17,7 @@ Feature: page/validate
And as user ""
When sending "get" to "/apps/libresign/p/validation"
And the response should be a JSON array with the following mandatory values
| key | value |
| message | Current user is not logged in |
| key | value |
| errors | ["You are not logged in. Please log in."] |
| action | 1000 |
| redirect | /index.php/login?redirect_url=/index.php/apps/libresign/p/validation |
5 changes: 0 additions & 5 deletions tests/psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@
<code><![CDATA[$storage]]></code>
</UndefinedDocblockClass>
</file>
<file src="lib/Controller/PageController.php">
<InvalidThrow>
<code><![CDATA[throw new NotLoggedInException();]]></code>
</InvalidThrow>
</file>
<file src="lib/Db/PagerFantaQueryAdapter.php">
<MissingTemplateParam>
<code><![CDATA[AdapterInterface]]></code>
Expand Down
Loading