Skip to content

Commit 6872749

Browse files
skjnldsvskjnldsv
committed
fix(dav): allow multiple link shares token in session
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
1 parent 082425b commit 6872749

9 files changed

Lines changed: 94 additions & 38 deletions

File tree

‎apps/dav/lib/Connector/Sabre/Auth.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,11 @@ public function __construct(
5454
* @see https://github.com/owncloud/core/issues/13245
5555
*/
5656
public function isDavAuthenticated(string $username): bool {
57-
return !is_null($this->session->get(self::DAV_AUTHENTICATED)) &&
58-
$this->session->get(self::DAV_AUTHENTICATED) === $username;
57+
if (is_null($this->session->get(self::DAV_AUTHENTICATED))) {
58+
return false;
59+
}
60+
61+
return $this->session->get(self::DAV_AUTHENTICATED) === $username;
5962
}
6063

6164
/**

‎apps/dav/lib/Connector/Sabre/PublicAuth.php‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,7 @@ private function checkToken(): array {
134134
\OC_User::setIncognitoMode(true);
135135

136136
// If already authenticated
137-
if ($this->session->exists(self::DAV_AUTHENTICATED)
138-
&& $this->session->get(self::DAV_AUTHENTICATED) === $share->getId()) {
137+
if ($this->isShareInSession($share)) {
139138
return [true, $this->principalPrefix . $token];
140139
}
141140

@@ -177,17 +176,17 @@ protected function validateUserPass($username, $password) {
177176
if ($share->getShareType() === IShare::TYPE_LINK
178177
|| $share->getShareType() === IShare::TYPE_EMAIL
179178
|| $share->getShareType() === IShare::TYPE_CIRCLE) {
179+
// Validate password if provided
180180
if ($this->shareManager->checkPassword($share, $password)) {
181181
// If not set, set authenticated session cookie
182-
if (!$this->session->exists(self::DAV_AUTHENTICATED)
183-
|| $this->session->get(self::DAV_AUTHENTICATED) !== $share->getId()) {
184-
$this->session->set(self::DAV_AUTHENTICATED, $share->getId());
182+
if (!$this->isShareInSession($share)) {
183+
$this->addShareToSession($share);
185184
}
186185
return true;
187186
}
188187

189-
if ($this->session->exists(PublicAuth::DAV_AUTHENTICATED)
190-
&& $this->session->get(PublicAuth::DAV_AUTHENTICATED) === $share->getId()) {
188+
// We are already authenticated for this share in the session
189+
if ($this->isShareInSession($share)) {
191190
return true;
192191
}
193192

@@ -221,4 +220,27 @@ public function getShare(): IShare {
221220

222221
return $this->share;
223222
}
223+
224+
private function addShareToSession(IShare $share): void {
225+
$allowedShareIds = $this->session->get(self::DAV_AUTHENTICATED) ?? [];
226+
if (!is_array($allowedShareIds)) {
227+
$allowedShareIds = [];
228+
}
229+
230+
$allowedShareIds[] = $share->getId();
231+
$this->session->set(self::DAV_AUTHENTICATED, $allowedShareIds);
232+
}
233+
234+
private function isShareInSession(IShare $share): bool {
235+
if (!$this->session->exists(self::DAV_AUTHENTICATED)) {
236+
return false;
237+
}
238+
239+
$allowedShareIds = $this->session->get(self::DAV_AUTHENTICATED);
240+
if (!is_array($allowedShareIds)) {
241+
return false;
242+
}
243+
244+
return in_array($share->getId(), $allowedShareIds);
245+
}
224246
}

‎apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ public function testInvalidSharePasswordLinkValidSession(): void {
333333
)->willReturn(false);
334334

335335
$this->session->method('exists')->with('public_link_authenticated')->willReturn(true);
336-
$this->session->method('get')->with('public_link_authenticated')->willReturn('42');
336+
$this->session->method('get')->with('public_link_authenticated')->willReturn(['42']);
337337

338338
$result = $this->invokePrivate($this->auth, 'validateUserPass', ['username', 'password']);
339339

‎apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,15 @@ public function createFederatedShare($shareWith, $token, $password = '') {
8989
}
9090

9191
// make sure that user is authenticated in case of a password protected link
92+
$allowedShareIds = $this->session->get(PublicAuth::DAV_AUTHENTICATED);
93+
if (!is_array($allowedShareIds)) {
94+
$allowedShareIds = [];
95+
}
96+
97+
$authenticated = in_array($share->getId(), $allowedShareIds)
98+
|| $this->shareManager->checkPassword($share, $password);
99+
92100
$storedPassword = $share->getPassword();
93-
$authenticated = $this->session->get(PublicAuth::DAV_AUTHENTICATED) === $share->getId() ||
94-
$this->shareManager->checkPassword($share, $password);
95101
if (!empty($storedPassword) && !$authenticated) {
96102
$response = new JSONResponse(
97103
['message' => 'No permission to access the share'],

‎apps/files_sharing/lib/Controller/ShareController.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,12 @@ protected function authSucceeded() {
196196
}
197197

198198
// For share this was always set so it is still used in other apps
199-
$this->session->set(PublicAuth::DAV_AUTHENTICATED, $this->share->getId());
199+
$allowedShareIds = $this->session->get(PublicAuth::DAV_AUTHENTICATED);
200+
if (!is_array($allowedShareIds)) {
201+
$allowedShareIds = [];
202+
}
203+
204+
$this->session->set(PublicAuth::DAV_AUTHENTICATED, array_merge($allowedShareIds, [$this->share->getId()]));
200205
}
201206

202207
protected function authFailed() {

‎lib/public/AppFramework/AuthPublicShareController.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,7 @@ final public function authenticate(string $password = '', string $passwordReques
158158
$this->session->regenerateId(true, true);
159159
$response = $this->getRedirect();
160160

161-
$this->session->set('public_link_authenticated_token', $this->getToken());
162-
$this->session->set('public_link_authenticated_password_hash', $this->getPasswordHash());
161+
$this->storeTokenSession($this->getToken(), $this->getPasswordHash());
163162

164163
$this->authSucceeded();
165164

‎lib/public/AppFramework/PublicShareController.php‎

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,21 @@
2525
* @since 14.0.0
2626
*/
2727
abstract class PublicShareController extends Controller {
28-
/** @var ISession */
29-
protected $session;
28+
29+
public const DAV_AUTHENTICATED_FRONTEND = 'public_link_authenticated_frontend';
3030

3131
/** @var string */
3232
private $token;
3333

3434
/**
3535
* @since 14.0.0
3636
*/
37-
public function __construct(string $appName,
37+
public function __construct(
38+
string $appName,
3839
IRequest $request,
39-
ISession $session) {
40+
protected ISession $session,
41+
) {
4042
parent::__construct($appName, $request);
41-
42-
$this->session = $session;
4343
}
4444

4545
/**
@@ -98,8 +98,7 @@ public function isAuthenticated(): bool {
9898
}
9999

100100
// If we are authenticated properly
101-
if ($this->session->get('public_link_authenticated_token') === $this->getToken() &&
102-
$this->session->get('public_link_authenticated_password_hash') === $this->getPasswordHash()) {
101+
if ($this->validateTokenSession($this->getToken(), $this->getPasswordHash())) {
103102
return true;
104103
}
105104

@@ -116,4 +115,31 @@ public function isAuthenticated(): bool {
116115
*/
117116
public function shareNotFound() {
118117
}
118+
119+
/**
120+
* Validate the token and password hash stored in session
121+
*/
122+
protected function validateTokenSession(string $token, string $passwordHash): bool {
123+
$allowedTokensJSON = $this->session->get(self::DAV_AUTHENTICATED_FRONTEND) ?? '[]';
124+
$allowedTokens = json_decode($allowedTokensJSON, true);
125+
if (!is_array($allowedTokens)) {
126+
$allowedTokens = [];
127+
}
128+
129+
return ($allowedTokens[$token] ?? '') === $passwordHash;
130+
}
131+
132+
/**
133+
* Store the token and password hash in session
134+
*/
135+
protected function storeTokenSession(string $token, string $passwordHash = ''): void {
136+
$allowedTokensJSON = $this->session->get(self::DAV_AUTHENTICATED_FRONTEND) ?? '[]';
137+
$allowedTokens = json_decode($allowedTokensJSON, true);
138+
if (!is_array($allowedTokens)) {
139+
$allowedTokens = [];
140+
}
141+
142+
$allowedTokens[$token] = $passwordHash;
143+
$this->session->set(self::DAV_AUTHENTICATED_FRONTEND, json_encode($allowedTokens));
144+
}
119145
}

‎tests/lib/AppFramework/Controller/AuthPublicShareControllerTest.php‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,17 +109,15 @@ public function testAuthenticateValidPassword(): void {
109109
$this->session->method('get')
110110
->willReturnMap(['public_link_authenticate_redirect', ['foo' => 'bar']]);
111111

112-
$tokenSet = false;
113-
$hashSet = false;
112+
$tokenStored = false;
114113
$this->session
115114
->method('set')
116-
->willReturnCallback(function ($key, $value) use (&$tokenSet, &$hashSet) {
117-
if ($key === 'public_link_authenticated_token' && $value === 'token') {
118-
$tokenSet = true;
119-
return true;
120-
}
121-
if ($key === 'public_link_authenticated_password_hash' && $value === 'hash') {
122-
$hashSet = true;
115+
->willReturnCallback(function ($key, $value) use (&$tokenStored) {
116+
if ($key === AuthPublicShareController::DAV_AUTHENTICATED_FRONTEND) {
117+
$decoded = json_decode($value, true);
118+
if (isset($decoded['token']) && $decoded['token'] === 'hash') {
119+
$tokenStored = true;
120+
}
123121
return true;
124122
}
125123
return false;
@@ -131,7 +129,6 @@ public function testAuthenticateValidPassword(): void {
131129
$result = $this->controller->authenticate('password');
132130
$this->assertInstanceOf(RedirectResponse::class, $result);
133131
$this->assertSame('myLink!', $result->getRedirectURL());
134-
$this->assertTrue($tokenSet);
135-
$this->assertTrue($hashSet);
132+
$this->assertTrue($tokenStored);
136133
}
137134
}

‎tests/lib/AppFramework/Controller/PublicShareControllerTest.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,8 @@ public function testIsAuthenticatedNotPasswordProtected(bool $protected, string
7777
$controller = new TestController('app', $this->request, $this->session, $hash2, $protected);
7878

7979
$this->session->method('get')
80-
->willReturnMap([
81-
['public_link_authenticated_token', $token1],
82-
['public_link_authenticated_password_hash', $hash1],
83-
]);
80+
->with(PublicShareController::DAV_AUTHENTICATED_FRONTEND)
81+
->willReturn("{\"$token1\":\"$hash1\"}");
8482

8583
$controller->setToken($token2);
8684

0 commit comments

Comments
 (0)