Skip to content

Commit 2eaad34

Browse files
provokateurinprovokateurin
authored
Merge pull request #61870 from nextcloud/kano-ocm-token-confinement
fix(auth): confine OCM access token to the masked share endpoint
2 parents bc162c7 + db96073 commit 2eaad34

8 files changed

Lines changed: 98 additions & 23 deletions

File tree

‎apps/dav/appinfo/v1/caldav.php‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
use OCA\DAV\CalDAV\Validation\CalDavValidatePlugin;
1919
use OCA\DAV\Connector\LegacyDAVACL;
2020
use OCA\DAV\Connector\Sabre\Auth;
21-
use OCA\DAV\Connector\Sabre\BearerAuth;
2221
use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin;
2322
use OCA\DAV\Connector\Sabre\MaintenancePlugin;
2423
use OCA\DAV\Connector\Sabre\Principal;
@@ -48,12 +47,6 @@
4847
Server::get(IThrottler::class),
4948
'principals/'
5049
);
51-
$bearerAuthBackend = new BearerAuth(
52-
Server::get(IUserSession::class),
53-
Server::get(ISession::class),
54-
Server::get(IRequest::class),
55-
Server::get(IConfig::class),
56-
);
5750
$principalBackend = new Principal(
5851
Server::get(IUserManager::class),
5952
Server::get(IGroupManager::class),
@@ -115,9 +108,7 @@
115108

116109
// Add plugins
117110
$server->addPlugin(new MaintenancePlugin(Server::get(IConfig::class), $davL10n));
118-
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);
119-
$authPlugin->addBackend($bearerAuthBackend);
120-
$server->addPlugin($authPlugin);
111+
$server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend));
121112
$server->addPlugin(new \Sabre\CalDAV\Plugin());
122113

123114
$server->addPlugin(new LegacyDAVACL());

‎apps/dav/appinfo/v1/carddav.php‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
use OCA\DAV\CardDAV\Validation\CardDavValidatePlugin;
1818
use OCA\DAV\Connector\LegacyDAVACL;
1919
use OCA\DAV\Connector\Sabre\Auth;
20-
use OCA\DAV\Connector\Sabre\BearerAuth;
2120
use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin;
2221
use OCA\DAV\Connector\Sabre\MaintenancePlugin;
2322
use OCA\DAV\Connector\Sabre\Principal;
@@ -45,12 +44,6 @@
4544
Server::get(IThrottler::class),
4645
'principals/'
4746
);
48-
$bearerAuthBackend = new BearerAuth(
49-
Server::get(IUserSession::class),
50-
Server::get(ISession::class),
51-
Server::get(IRequest::class),
52-
Server::get(IConfig::class),
53-
);
5447
$principalBackend = new Principal(
5548
Server::get(IUserManager::class),
5649
Server::get(IGroupManager::class),
@@ -97,9 +90,7 @@
9790
$server->setBaseUri($baseuri);
9891
// Add plugins
9992
$server->addPlugin(new MaintenancePlugin(Server::get(IConfig::class), Server::get(IL10nFactory::class)->get('dav')));
100-
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);
101-
$authPlugin->addBackend($bearerAuthBackend);
102-
$server->addPlugin($authPlugin);
93+
$server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend));
10394
$server->addPlugin(new Plugin());
10495

10596
$server->addPlugin(new LegacyDAVACL());

‎apps/dav/appinfo/v1/publicwebdav.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
Server::get(ISession::class),
5757
Server::get(IRequest::class),
5858
Server::get(IConfig::class),
59+
allowOcmAccessToken: true,
5960
);
6061
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);
6162
$authPlugin->addBackend($bearerAuthBackend);

‎apps/dav/appinfo/v2/publicremote.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@
7474
$session,
7575
$request,
7676
Server::get(IConfig::class),
77+
allowOcmAccessToken: true,
7778
);
7879
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);
7980
$authPlugin->addBackend($bearerAuthBackend);

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ public function __construct(
2828
private IConfig $config,
2929
private string $principalPrefix = 'principals/users/',
3030
private string $token = '',
31+
private bool $allowOcmAccessToken = false,
3132
) {
3233
// setup realm
3334
$defaults = new Defaults();
@@ -57,7 +58,7 @@ public function validateBearerToken($bearerToken) {
5758
\OC_User::setIncognitoMode(false);
5859

5960
if (!$this->userSession->isLoggedIn()) {
60-
$this->userSession->tryTokenLogin($this->request);
61+
$this->userSession->tryTokenLogin($this->request, $this->allowOcmAccessToken);
6162
}
6263
if ($this->userSession->isLoggedIn()) {
6364
return $this->setupUserFs($this->userSession->getUser()->getUID());

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,40 @@ public function testValidateBearerToken(): void {
7070
$this->assertSame('principals/users/admin', $this->bearerAuth->validateBearerToken('Token'));
7171
}
7272

73+
public function testValidateBearerTokenDefaultsOcmFlagToFalse(): void {
74+
$this->userSession
75+
->method('isLoggedIn')
76+
->willReturnOnConsecutiveCalls(false, false);
77+
$this->userSession
78+
->expects($this->once())
79+
->method('tryTokenLogin')
80+
->with($this->request, false);
81+
82+
$this->assertFalse($this->bearerAuth->validateBearerToken('Token'));
83+
}
84+
85+
public function testValidateBearerTokenPassesOcmFlagWhenAllowed(): void {
86+
$bearerAuth = new BearerAuth(
87+
$this->userSession,
88+
$this->session,
89+
$this->request,
90+
$this->config,
91+
allowOcmAccessToken: true,
92+
);
93+
$this->userSession
94+
->method('isLoggedIn')
95+
->willReturnOnConsecutiveCalls(false, true);
96+
$this->userSession
97+
->expects($this->once())
98+
->method('tryTokenLogin')
99+
->with($this->request, true);
100+
$user = $this->createMock(IUser::class);
101+
$user->method('getUID')->willReturn('admin');
102+
$this->userSession->method('getUser')->willReturn($user);
103+
104+
$this->assertSame('principals/users/admin', $bearerAuth->validateBearerToken('Token'));
105+
}
106+
73107
public function testChallenge(): void {
74108
/** @var RequestInterface&MockObject $request */
75109
$request = $this->createMock(RequestInterface::class);

‎lib/private/User/Session.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -816,10 +816,13 @@ private function validateTokenLoginName(?string $loginName, IToken $token): bool
816816
* Tries to login the user with auth token header
817817
*
818818
* @param IRequest $request
819+
* @param bool $allowOcmAccessToken Whether an OCM access token may log in
820+
* from a Bearer header. Only the masked
821+
* public share endpoints may set this.
819822
* @todo check remember me cookie
820823
* @return boolean
821824
*/
822-
public function tryTokenLogin(IRequest $request) {
825+
public function tryTokenLogin(IRequest $request, bool $allowOcmAccessToken = false) {
823826
$authHeader = $request->getHeader('Authorization');
824827
$tokenFromCookie = false;
825828
if (str_starts_with($authHeader, 'Bearer ')) {
@@ -847,7 +850,7 @@ public function tryTokenLogin(IRequest $request) {
847850
if ($dbToken instanceof PublicKeyToken
848851
&& $dbToken->getType() === IToken::TEMPORARY_TOKEN
849852
&& !$tokenFromCookie
850-
&& $dbToken->getName() !== IToken::OCM_ACCESS_TOKEN_NAME) {
853+
&& !($allowOcmAccessToken && $dbToken->getName() === IToken::OCM_ACCESS_TOKEN_NAME)) {
851854
// Session token but from Bearer header, not allowed
852855
return false;
853856
}

‎tests/lib/User/SessionTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,59 @@ public function testTryTokenLoginNotAnAppPassword(): void {
662662
self::assertTrue($loginResult);
663663
}
664664

665+
public function testTryTokenLoginOcmAccessTokenRejectedFromBearerByDefault(): void {
666+
$request = $this->createMock(IRequest::class);
667+
$request->method('getHeader')->with('Authorization')->willReturn('Bearer ocm-access-token');
668+
$dbToken = new PublicKeyToken();
669+
$dbToken->setId(42);
670+
$dbToken->setUid('alice');
671+
$dbToken->setLoginName('alice');
672+
$dbToken->setLastCheck(0);
673+
$dbToken->setType(IToken::TEMPORARY_TOKEN);
674+
$dbToken->setName(IToken::OCM_ACCESS_TOKEN_NAME);
675+
$this->tokenProvider->expects(self::once())
676+
->method('getToken')
677+
->with('ocm-access-token')
678+
->willReturn($dbToken);
679+
// The guard must reject before any login is attempted.
680+
$this->manager->expects(self::never())
681+
->method('get');
682+
683+
$loginResult = $this->userSession->tryTokenLogin($request);
684+
685+
self::assertFalse($loginResult);
686+
}
687+
688+
public function testTryTokenLoginOcmAccessTokenAllowedFromBearerWhenPermitted(): void {
689+
$request = $this->createMock(IRequest::class);
690+
$request->method('getHeader')->with('Authorization')->willReturn('Bearer ocm-access-token');
691+
$dbToken = new PublicKeyToken();
692+
$dbToken->setId(42);
693+
$dbToken->setUid('alice');
694+
$dbToken->setLoginName('alice');
695+
$dbToken->setLastCheck(0);
696+
$dbToken->setType(IToken::TEMPORARY_TOKEN);
697+
$dbToken->setName(IToken::OCM_ACCESS_TOKEN_NAME);
698+
$this->tokenProvider->method('getToken')
699+
->with('ocm-access-token')
700+
->willReturn($dbToken);
701+
$this->session->method('set')
702+
->willReturnCallback(function ($key, $value): void {
703+
if ($key === 'app_password') {
704+
throw new ExpectationFailedException('app_password should not be set in session');
705+
}
706+
});
707+
$user = $this->createMock(IUser::class);
708+
$user->method('isEnabled')->willReturn(true);
709+
$this->manager->method('get')
710+
->with('alice')
711+
->willReturn($user);
712+
713+
$loginResult = $this->userSession->tryTokenLogin($request, true);
714+
715+
self::assertTrue($loginResult);
716+
}
717+
665718
public function testRememberLoginValidToken(): void {
666719
$session = $this->createMock(Memory::class);
667720
$managerMethods = get_class_methods(Manager::class);

0 commit comments

Comments
 (0)