Repository navigation
feat(auth): limit scoped app passwords to the routes they open - #65340
pringelmann wants to merge 3 commits into
Conversation
pringelmann
added
security
feature: authentication
feature: settings
team: IDP
| $dispatcher->registerMiddleware($c->get(PublicShareMiddleware::class)); | ||
| $dispatcher->registerMiddleware($c->get(AdditionalScriptsMiddleware::class)); | ||
| // After AdditionalScriptsMiddleware, so pages are refused before their listeners run | ||
| $dispatcher->registerMiddleware($c->get(TokenScopeMiddleware::class)); |
There was a problem hiding this comment.
Own middleware instead of one more check in SecurityMiddleware, which is hand-built and already does a lot. Also registered last on purpose, the test in DIContainerTest checks the order.
| return; | ||
| } | ||
|
|
||
| if (!$this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class) |
There was a problem hiding this comment.
An admin's token passes SecurityMiddleware on admin routes. Scoped tokens shouldn't, even if they belong to an admin.
There was a problem hiding this comment.
But in the end won’t all tokens be scoped tokens? How does one do admin work with a token then?
There should be an admin scope, no?
There was a problem hiding this comment.
No, All access stays. Login flow tokens (desktop, mobile) and existing tokens have no scopes and keep full access, admin rights included, and "All access" stays an option in Settings. Scopes are opt-in for scripts and integrations that only need part of the account, so admin work keeps using an All access token like today.
A narrower admin scope (e.g. only user provisioning) could maybe come later if there's demand, but I'd keep it out of this round.
| * Page renders run every app's listeners and initial state, so none reach a token with scopes | ||
| */ | ||
| #[\Override] | ||
| public function afterController(Controller $controller, string $methodName, Response $response): Response { |
There was a problem hiding this comment.
We only know it's a page once the controller returns a TemplateResponse, so it has to check after controller runs. Page GETs don't change data, so letting the controller run is fine though. What we block is the render, which pulls in every app's listeners and initial state.
d4d67af to
a2b3655
Compare
pringelmann
changed the title
a2b3655 to
bd4dc21
Compare
|
Drafting for now, got feedback that necessitates major changes |
bd4dc21 to
0823522
Compare
|
Reworked after a design discussion with @artonge : the route list is gone, controllers now declare their scope with an attribute. Comments on |
0823522 to
cf1ad78
Compare
cf1ad78 to
c1b26e4
Compare
Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
c1b26e4 to
bdac8ee
Compare
This branch has not been deployed
4 participants
Summary
Second step for limited app passwords: a token with scopes can now only call the index.php and OCS routes its scopes open. Everything else gets a 403. Tokens without scopes aren't affected. DAV comes in the next PR.
How it works
#[TokenScope]attribute, on a method or the class. No attribute means refused, so new endpoints start closed#[TokenScope]without a scope is open to any scoped tokenfiles:write, their read-only controllersfiles:read. Shares, sharee search and direct links needfiles:shareKnown limit: an admin's calendar or contacts token can import into other users' calendars and address books via the
userparameter, and a files token can transfer ownership of files to another account.Tests
Unit tests for the route table, the middleware and its position in the stack. A test pins the scope of the share, direct link and preview endpoints, and checks that templates, direct editing and conversion stay unannotated. Integration tests cover an allowed call, a refused share, unified search and a page render with a scoped token.
Next step
DAV enforcement, then the access picker in Settings.
Checklist
3. to review, feature component)stable32)AI (if applicable)