Skip to content

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

Merged
rullzer merged 1 commit intomasterfrom
enh/disable-clear-site-data-via-config
Dec 12, 2019
Merged

Disable Clear-Site-Data for Chrom* (and Opera, Brave, etc)#17784
rullzer merged 1 commit intomasterfrom
enh/disable-clear-site-data-via-config

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 2, 2019 •
edited
Loading

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
Closed
@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.

ChristophWurst
ChristophWurst approved these changes Nov 11, 2019
View reviewed changes
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

skjnldsv commented Nov 12, 2019

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
skjnldsv
skjnldsv reviewed Nov 30, 2019
View reviewed changes
core/Controller/LoginController.php Outdated
$this->session->close();
$response->addHeader('Clear-Site-Data', '"cache", "storage"');

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

@skjnldsv skjnldsv Nov 30, 2019

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

@kesselb kesselb Nov 30, 2019

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
Send Clear-Site-Data expect for Chrome
9378a6b 
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@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
Merged
Merged
@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
@kesselb kesselb mentioned this pull request Dec 27, 2019
Closed
@HLFH HLFH mentioned this pull request Oct 30, 2023
Closed
8 tasks
@joshtrichards joshtrichards mentioned this pull request Oct 30, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

No one assigned

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Milestone

Nextcloud 18.0.0

Development

Successfully merging this pull request may close these issues.

Slow log out.

4 participants

@kesselb @skjnldsv @rullzer @ChristophWurst