Skip to content

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

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

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

Conversation

@backportbot-nextcloud

@backportbot-nextcloudbackportbot-nextcloudBot commented Mar 15, 2023

Copy link
Copy Markdown

backport of #36033

@backportbot-nextcloudbackportbot-nextcloudBot added this to the Nextcloud 26 milestone Mar 15, 2023
@blizzzblizzz mentioned this pull request Mar 15, 2023
Merged
@come-nccome-nc changed the title [stable26] [master] invalidate existing tokens when deleting an oauth client[stable26] invalidate existing tokens when deleting an oauth clientMar 15, 2023
blizzz
blizzz requested changes Mar 15, 2023
View reviewed changes
Comment threadapps/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) {

@blizzzblizzzMar 15, 2023

Copy link
Copy Markdown
Member

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

@come-nccome-ncMar 15, 2023

Copy link
Copy Markdown
Contributor

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?

@blizzzblizzzMar 16, 2023

Copy link
Copy Markdown
Member

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.

@individual-itindividual-itMar 16, 2023

Copy link
Copy Markdown
Contributor

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()?

@blizzzblizzzMar 16, 2023

Copy link
Copy Markdown
Member

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

@individual-itindividual-itMar 17, 2023

Copy link
Copy Markdown
Contributor

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.

@blizzzblizzzMay 23, 2023

Copy link
Copy Markdown
Member

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 commented Mar 16, 2023

Copy link
Copy Markdown
Member

moving to 26.0.1 (as final RC3 is due)

@individual-it
individual-itforce-pushed the backport/36033/stable26 branch from 55e0975 to e25b640CompareMarch 17, 2023 08:18
@blizzzblizzz mentioned this pull request Mar 20, 2023
Merged
@come-nc
come-nc requested a review from blizzzApril 5, 2023 09:54
@come-nc

come-nc commented Apr 5, 2023

Copy link
Copy Markdown
Contributor

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

@skjnldsvskjnldsv mentioned this pull request Apr 13, 2023
Merged
8 tasks
@individual-itindividual-it mentioned this pull request Apr 17, 2023
Merged
4 tasks
@individual-it

individual-it commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

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

@skjnldsvskjnldsv mentioned this pull request Apr 18, 2023
Merged
9 tasks
@blizzzblizzz mentioned this pull request May 16, 2023
Merged
@blizzz

blizzz commented May 17, 2023

Copy link
Copy Markdown
Member

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

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

blizzz commented May 17, 2023•
edited
Loading

Copy link
Copy Markdown
Member

→ 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@SInCEtag
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-itforce-pushed the backport/36033/stable26 branch from e25b640 to cb005f6CompareMay 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

individual-it commented May 19, 2023

Copy link
Copy Markdown
Contributor

@blizzz unit tests fixed, also for the other repos

@blizzzblizzz mentioned this pull request Jun 12, 2023
Merged
blizzz
blizzz reviewed Jun 13, 2023
View reviewed changes
Comment threadlib/public/Authentication/Token/IProvider.php Outdated
blizzz
blizzz reviewed Jun 13, 2023
View reviewed changes
Comment threadlib/public/Authentication/Token/IProvider.php Outdated
@blizzz
adjust@SInCEannotation
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 stable26Jun 15, 2023
@blizzz
blizzz deleted the backport/36033/stable26 branch June 15, 2023 09:54
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@come-nccome-nccome-nc approved these changes
@blizzzblizzzblizzz approved these changes
@individual-itindividual-itAwaiting requested review from individual-it
@julien-ncjulien-ncAwaiting 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