[stable24] invalidate existing tokens when deleting an oauth client

[individual-it]

When an oauth client is deleted all the existing tokens should be invalidated

fixes #35068

I've created a PR to stable24 and not master because of #35045 that makes it harder to test the fix

[szaimen]

/backport to master

[szaimen]

/backport to stable25

[come-nc]

Things in OC namespace should not be used in applications.
But I do not have enough knowledge about this part of the code to understand why these interfaces are in OC and not OCP and if there is a better way of doing that…

[individual-it]

@come-nc I could not find anything in OCP that would do the job.
When deleting the oauth client the tokens get deleted from the oc_oauth2_access_tokens table but there are still present in the oc_authtoken table. I don't quite understand how the complete workflow works, but it looks to me that the the real authentication happens with data from oc_authtoken.

[individual-it]

I believe the last test that is failing is not relevant to the code changes

[julien-nc]

OC vs OCP

@nickvergessen @juliushaertl @blizzz As OC\Authentication\Token\IProvider is used by quite some apps

• https://github.com/nextcloud/notifications/blob/c61830aa40ae5430cc0e143cffbffbdba653384a/lib/Controller/PushController.php#L28
• https://github.com/nextcloud/spreed/blob/2d3c1b294127473cad08dcc42f2c1144cb897c4d/lib/MatterbridgeManager.php#L26
• https://github.com/nextcloud/user_oidc/blob/4ad91f540ed18c2f67aab685449a48a98bfc7a3a/lib/Controller/LoginController.php#L31
• https://github.com/nextcloud/user_retention/blob/83a60492f12937cd861b712081a318b73b53d53d/lib/BackgroundJob/ExpireUsers.php#L201-L211

Can/should we consider providing an interface in the OCP namespace?

Token management

In the oauth2 app, it seems tokens are duplicated between an oauth2 table and oc_authtoken.
Same in the notifications app with oc_notifications_pushhash.
So when we delete a token in the app-specific tables, it stays in oc_authtoken if we don't take care of it (with OC\Authentication\Token\IProvider).
For example in https://github.com/nextcloud/notifications/blob/c61830aa40ae5430cc0e143cffbffbdba653384a/lib/Controller/PushController.php#L264-L271
@nickvergessen Is this intentional or does it leave a token alive while we expect it to be gone?

[nickvergessen]

Same in the notifications app with oc_notifications_pushhash.
So when we delete a token in the app-specific tables, it stays in oc_authtoken if we don't take care of it (with OC\Authentication\Token\IProvider).

Notification is only additional. Not every authtoken has a pushhash entry (only the ones from devices that register for push afterwards). It is also self healing, stray entries from oc_notifications_pushhash are deleted on first push after oc_authtoken got deleted. So ignore that part. It's never used for authentication and basically only in another table to not bloat the oc_authtoken table with columns that are notification app specific.

Can/should we consider providing an interface in the OCP namespace?

I would welcome that. Also the current IToken interface does not have all columns, so apps need to even type hint to an actual implementation.

[come-nc]

There were 3 errors:

1) OCA\OAuth2\Tests\Controller\SettingsControllerTest::testAddClient
TypeError: Argument 7 passed to OCA\OAuth2\Controller\SettingsController::__construct() must implement interface OCP\Authentication\Token\IProvider, instance of Mock_IProvider_ce21455d given, called in /home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php on line 79

/home/runner/work/server/server/apps/oauth2/lib/Controller/SettingsController.php:63
/home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php:79

2) OCA\OAuth2\Tests\Controller\SettingsControllerTest::testDeleteClient
TypeError: Argument 7 passed to OCA\OAuth2\Controller\SettingsController::__construct() must implement interface OCP\Authentication\Token\IProvider, instance of Mock_IProvider_ce21455d given, called in /home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php on line 79

/home/runner/work/server/server/apps/oauth2/lib/Controller/SettingsController.php:63
/home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php:79

3) OCA\OAuth2\Tests\Controller\SettingsControllerTest::testInvalidRedirectUri
TypeError: Argument 7 passed to OCA\OAuth2\Controller\SettingsController::__construct() must implement interface OCP\Authentication\Token\IProvider, instance of Mock_IProvider_ce21455d given, called in /home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php on line 79

/home/runner/work/server/server/apps/oauth2/lib/Controller/SettingsController.php:63
/home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php:79

[individual-it]

@come-nc sorry I pushed that commit yesterday just before going home from work, I know the unit tests need to be fixed / adjusted. I will try to do that today

[come-nc]

The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh

(from drone CI, all the other tests passed without error)

[individual-it]

@come-nc a lot of files got changed, see 63fedca I hope that is correct

[come-nc]

@come-nc a lot of files got changed, see 63fedca I hope that is correct

I think it is correct, the CI is happy. It seems you ran into a composer new version, which often moves a few thing around.

[individual-it]

yes I believe the last failing test is not related to my changes

[szaimen]

This is missing one approval...

[blizzz]

moving to 24.0.9

[PVince81]

/rebase

[backportbot-nextcloud]

The backport to master failed. Please do this backport manually.

[backportbot-nextcloud]

The backport to stable25 failed. Please do this backport manually.

[individual-it]

forward port to master in #36033
forward port to stable25 in #36034