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.

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

Disable Clear-Site-Data for Chrom* (and Opera, Brave, etc) #17784

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 2, 2019

Close #9179 (again)

I think Clear-Site-Data is a good feature but it will slow down the logout for chromium based browsers. For some reason clearing the site data (see #9179 (comment) and #9179 (comment)) takes a while. There is nothing we can do.

People will always complain to their administrators about the slow logout. We should give them a way to opt out. I've been thinking about sending the header only for firefox but this makes things even more complicated.

Since this patch has no real impact I would like to ask for a backport to 17.

@kesselb kesselb added enhancement 3. to review Waiting for reviews labels Nov 2, 2019
@kesselb kesselb added this to the Nextcloud 18 milestone Nov 2, 2019
@kesselb kesselb mentioned this pull request Nov 2, 2019
@skjnldsv
Copy link
Member

skjnldsv commented Nov 3, 2019

I'm really not sure :/
What is really the cause of slow logout on firefox? Why is ff taking so long? Is there an opened bug report for it?

@kesselb
Copy link
Contributor Author

kesselb commented Nov 3, 2019

What is really the cause of slow logout on firefox? Why is ff taking so long?

Chrome (and everything else based on chromium) ;) It's working fine with Firefox.

Is there an opened bug report for it?

https://bugs.chromium.org/p/chromium/issues/detail?id=762417 They merged a fix for Chromium 77 (Stable Release Tue, Sep 10, 2019). Added this information to #9179 (comment). Let's wait for feedback ;)

@kesselb
Copy link
Contributor Author

kesselb commented Nov 3, 2019

I think we should turn it off by default for now. There more than 70 reports over at #9179. We can't fix it but people will always think it's related to Nextcloud.

According to https://bugs.chromium.org/p/chromium/issues/detail?id=762417#c37 GitHub does not send the header anymore for chrome.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

I agree that there is no other way to fix this right now

👍 😢

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 12, 2019
@rullzer
Copy link
Member

rullzer commented Nov 12, 2019

I don't like it.
Lets either kill it all together. Or do it by user agent.

Having it as a config option doesn't help. Because this is not something a lot of users can change.

@skjnldsv skjnldsv added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Nov 12, 2019
@skjnldsv
Copy link
Member

Or do it by user agent.

Did not thought of this! Nice idea! :)

@kesselb kesselb force-pushed the enh/disable-clear-site-data-via-config branch 3 times, most recently from e037e06 to 49fac48 Compare November 12, 2019 21:00
@kesselb
Copy link
Contributor Author

kesselb commented Nov 12, 2019

Or do it by user agent.

Fine by me but I'm not a fan :)

@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 13, 2019
@kesselb kesselb changed the title Add config option to disable Clear-Site-Data Disable Clear-Site-Data for Chrome* (and Opera, Brave, etc) Nov 13, 2019
@kesselb kesselb changed the title Disable Clear-Site-Data for Chrome* (and Opera, Brave, etc) Disable Clear-Site-Data for Chrom* (and Opera, Brave, etc) Nov 13, 2019
@@ -128,7 +128,11 @@ public function logout() {

$this->session->set('clearingExecutionContexts', '1');
$this->session->close();
$response->addHeader('Clear-Site-Data', '"cache", "storage"');

if (!$this->request->isUserAgent(['#Chrome/#'])) {
Copy link
Member

Choose a reason for hiding this comment

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

we have a list of valid user agent somewhere in a php class iirc :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. They are a bit more specific than '#Chrome/#' 😆

@kesselb kesselb force-pushed the enh/disable-clear-site-data-via-config branch from 49fac48 to 8c01bb9 Compare November 30, 2019 13:49
@kesselb kesselb force-pushed the enh/disable-clear-site-data-via-config branch from 8c01bb9 to 9378a6b Compare November 30, 2019 14:17
This was referenced Dec 11, 2019
@rullzer rullzer merged commit 87104ce into master Dec 12, 2019
@rullzer rullzer deleted the enh/disable-clear-site-data-via-config branch December 12, 2019 20:59
@HLFH HLFH mentioned this pull request Oct 30, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow log out.
4 participants