-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(provisioning-api): Add endpoint to invalidate user tokens #52253
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
feat(provisioning-api): Add endpoint to invalidate user tokens #52253
Conversation
#[NoAdminRequired] | ||
public function invalidateUserTokens(string $userId): DataResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big no no if anyone can do this to any user!
Ok I see you still have a user check in place, but from the description it really sounds like you only want admins to do this. |
lib/private/Authentication/Events/AUserTokensInvalidationEvent.php
Outdated
Show resolved
Hide resolved
public function invalidateAllUserTokens(string $uid): bool { | ||
$this->logger->info("Invalidating all tokens for user: $uid"); | ||
$this->eventDispatcher->dispatch(TokensInvalidationStarted::class, new TokensInvalidationStarted($uid)); | ||
|
||
$tokens = $this->tokenProvider->getTokenByUser($uid); | ||
foreach ($tokens as $token) { | ||
$this->tokenProvider->invalidateTokenById($uid, $token->getId()); | ||
} | ||
|
||
$this->eventDispatcher->dispatch(TokensInvalidationFinished::class, new TokensInvalidationFinished($uid)); | ||
return true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add that method to PublicKeyTokenProvider
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PublicKeyTokenProvider
has no imported IEventDispatcher
.
We would like to be able to log events via the admin_audit app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to add it to the constructor arguments.
if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { | ||
throw new OCSException('', 101); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inspired by \OCA\Provisioning_API\Controller\UsersController::wipeUserDevices
throw new OCSException('', 101); |
$subAdminManager = $this->groupManager->getSubAdmin(); | ||
$isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()); | ||
$isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); | ||
if (!$isAdmin && !($isDelegatedAdmin && !$this->groupManager->isInGroup($targetUser->getUID(), 'admin')) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) { | ||
throw new OCSException('', OCSController::RESPOND_NOT_FOUND); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we have the same kind of code in other places, but it’s inefficient, because it will try to see if the user is a subadmin even if he’s an admin, which may be expensive if groups are through a backend with subgroups and stuff.
So, please try to write this in a way where it only computes needed information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of this PR, I won't mind the copy past. If we want a better way of doing it, let's do it in another PR.
use OC\Authentication\Events\TokensInvalidationFinished; | ||
use OC\Authentication\Events\TokensInvalidationStarted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these events listened to by anyone?
If the intent is that applications may listen to them, they should be part of OCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to consume it via admin_audit app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it needs to be part of public API in OCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@printminion-co please move those events to the lib/public/Authentication/Events
directory. And set their namespace to OCP\Authentication\Events;
79a9700
to
826559d
Compare
0e9fab4
to
69d095a
Compare
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
69d095a
to
51de786
Compare
Introduce a core API endpoint for external user management systems to invalidate user tokens. This action removes all tokens of the target user, effectively logging them out from all sessions (browser, apps) without wiping data on their devices. ### Usage example: ```bash curl -s -X DELETE "$NEXTCLOUD_URL/ocs/v2.php/cloud/users/${testUserId}/sessions?format=json" \ -u "$ADMIN_USERNAME:$ADMIN_PASSWORD" \ -H "OCS-APIRequest: true" ``` Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
51de786
to
19015d8
Compare
Our team has decided to deprioritize the approach outlined in that pull request. If Nextcloud is interested in pursuing this feature, please feel free to maintain the PR. Otherwise, I will proceed to close it in the near future. |
Summary
Introduce a core API endpoint for external user management systems to invalidate user tokens.
This action removes all tokens of the target user, effectively logging them out from all sessions (browser, apps) without wiping data on their devices.
Test
You can use following script
test_token_invalidation_api.sh
Unitests
tests/phpunit-autotest-user-invalidate.xml
```xml ./lib/User/SessionTest.php ./lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php ./Core/Controller/ClientFlowLoginV2ControllerTest.php ./Core/Service/LoginFlowV2ServiceUnitTest.php ./Core/Controller/AppPasswordControllerTest.php ./Core/Controller/ClientFlowLoginControllerTest.php ./Core/Controller/WipeControllerTest.php ./Core/Controller/UserControllerTest.php ./lib/Authentication/Token/RemoteWipeTest.php ./lib/Authentication/Token/InvalidatorTest.php ../core/* ../lib/private/* ../**/ ../3rdparty/**/* ../apps/**/* ../apps-custom/**/* ../apps-external/** ../apps-custom/** ../build ../IONOS/**/* ../lib/composer ../vendor/**/* ../tests ```tests/phpunit-autotest-user-invalidate.xml
```xml ../apps/provisioning_api/tests ../lib/private/Files/Storage/DAV.php ../apps/provisioning_api ../apps/provisioning_api/l10n ../apps/provisioning_api/3rdparty ../apps/provisioning_api/tests ```Checklist