Skip to content

[stable26] invalidate existing tokens when deleting an oauth client#37230

Merged
blizzz merged 10 commits intostable26from
backport/36033/stable26
Jun 15, 2023
Merged

[stable26] invalidate existing tokens when deleting an oauth client#37230
blizzz merged 10 commits intostable26from
backport/36033/stable26

Conversation

@backportbot-nextcloud
Copy link

@backportbot-nextcloud backportbot-nextcloud bot commented Mar 15, 2023

backport of #36033

@backportbot-nextcloud backportbot-nextcloud bot added this to the Nextcloud 26 milestone Mar 15, 2023
@blizzz blizzz mentioned this pull request Mar 15, 2023
Merged
@come-nc come-nc changed the title [stable26] [master] invalidate existing tokens when deleting an oauth client [stable26] invalidate existing tokens when deleting an oauth client Mar 15, 2023
blizzz
blizzz requested changes Mar 15, 2023
View reviewed changes
apps/oauth2/lib/Controller/SettingsController.php Outdated
public function deleteClient(int $id): JSONResponse {
$client = $this->clientMapper->getByUid($id);

$this->userManager->callForAllUsers(function (IUser $user) use ($client) {
Copy link
Member

@blizzz blizzz Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be run for known users, not all users, and not in user facing requests as it may take ages

Copy link
Contributor

@come-nc come-nc Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same thing was merged in master and stable24 already :-/
Why would it take ages?

Copy link
Member

@blizzz blizzz Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it will ask all connected backends to all users. not an issue on small local instance, but a factor on big setups.

Copy link
Contributor

@individual-it individual-it Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz do you mean using callForSeenUsers() instead of callForAllUsers()?

Copy link
Member

@blizzz blizzz Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz do you mean using callForSeenUsers() instead of callForAllUsers()?

yes, and ideally it runs through background jobs only

Copy link
Contributor

@individual-it individual-it Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz I've changed it to callForSeenUsers() in e25b640

But I think I would not put it into background jobs, because as admin I would expect the connections to be deleted immediately after I delete the oauth client and not only after a cron job eventually runs.

Copy link
Member

@blizzz blizzz May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the instance size it may cycle over x thousands of users.

@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 26.0.1 Mar 16, 2023
@blizzz
Copy link
Member

blizzz commented Mar 16, 2023

moving to 26.0.1 (as final RC3 is due)

@individual-it individual-it force-pushed the backport/36033/stable26 branch from 55e0975 to e25b640 Compare March 17, 2023 08:18
@blizzz blizzz mentioned this pull request Mar 20, 2023
Merged
@come-nc come-nc requested a review from blizzz April 5, 2023 09:54
@come-nc
Copy link
Contributor

come-nc commented Apr 5, 2023

This will need to be backported to stable25, and the callForSeenUser change needs to be ported to master as well in a smaller PR

@skjnldsv skjnldsv mentioned this pull request Apr 13, 2023
Merged
8 tasks
@individual-it individual-it mentioned this pull request Apr 17, 2023
Merged
4 tasks
@individual-it
Copy link
Contributor

individual-it commented Apr 17, 2023

backport to stable25 already exists and I've added the last commit to it too: #37231
port to master of the last commit is in: #37761

@skjnldsv skjnldsv mentioned this pull request Apr 18, 2023
Merged
9 tasks
@blizzz blizzz mentioned this pull request May 16, 2023
Merged
@blizzz
Copy link
Member

blizzz commented May 17, 2023

CI is failing, at least PHPUnit / phpunit-oci (8.2) might be related

@individual-it individual-it mentioned this pull request May 17, 2023
Merged
1 task
@blizzz
Copy link
Member

blizzz commented May 17, 2023 •
edited
Loading

→ 26.0.3

@individual-it
invalidate existing tokens when deleting an oauth client
9f8f2d2 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
public interface to invalidate tokens of user
19bb66a 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
adjust SettingsController tests
aae3ff6 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
unit tests for Manager::invalidateTokensOfUser
774cdf5 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
added @SInCE tag
3e1e6f1 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
autoloaderchecker
57c7b73 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
move mocks into private variables
dcab216 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
invalidate oauth2 tokens only for seen users
cb005f6 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it individual-it force-pushed the backport/36033/stable26 branch from e25b640 to cb005f6 Compare May 19, 2023 06:17
@individual-it
expect invalidateTokensOfUser only be called for seen users
67bba35 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
Copy link
Contributor

individual-it commented May 19, 2023

@blizzz unit tests fixed, also for the other repos

@blizzz blizzz mentioned this pull request Jun 12, 2023
Merged
blizzz
blizzz reviewed Jun 13, 2023
View reviewed changes
blizzz
blizzz reviewed Jun 13, 2023
View reviewed changes
@blizzz
adjust @SInCE annotation
40dcc08 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz enabled auto-merge June 14, 2023 21:27
@blizzz blizzz disabled auto-merge June 15, 2023 09:54
@blizzz blizzz merged commit 00afe49 into stable26 Jun 15, 2023
@blizzz blizzz deleted the backport/36033/stable26 branch June 15, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@come-nc come-nc come-nc approved these changes

@blizzz blizzz blizzz approved these changes

@individual-it individual-it Awaiting requested review from individual-it

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Nextcloud 26.0.3

Development

Successfully merging this pull request may close these issues.

4 participants

@blizzz @come-nc @individual-it @skjnldsv