Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Jun 15, 2023
Merged

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

merged 10 commits into from
Jun 15, 2023

Conversation

backportbot-nextcloud[bot]
Copy link

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
@@ -92,6 +104,11 @@ public function addClient(string $name,

public function deleteClient(int $id): JSONResponse {
$client = $this->clientMapper->getByUid($id);

$this->userManager->callForAllUsers(function (IUser $user) use ($client) {
Copy link
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

Copy link
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?

Copy link
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.

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

Copy link
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

Copy link
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.

Copy link
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 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)

@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

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
expect invalidateTokensOfUser only be called for seen users
67bba35 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
Copy link
Contributor

@blizzz unit tests fixed, also for the other repos

@blizzz blizzz mentioned this pull request Jun 12, 2023
Merged
@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