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

Only output the logout JS when the cookie is present #165

Merged
merged 4 commits into from
Feb 14, 2023

Conversation

akirk
Copy link
Member

@akirk akirk commented Jan 25, 2023

Fixes #164

This adds a PHP check for the cookie so that the logout JS code is not printed for every user.

@akirk akirk requested a review from psrpinto January 25, 2023 14:46
@psrpinto
Copy link
Member

We originally implemented this in #153 but then reverted due to potential issues when there's caching:

I suspect this would cause logout issues when used on a website which has caching plugin, so might be safer to always load the logout script file and not based on cookie check. To expand on it, imagine a WP site with logout redirection to homepage and has a caching plugin so homepage load never really checks for cookie presence for loading logout script.

@akirk
Copy link
Member Author

akirk commented Jan 25, 2023

Hm, going that route adds an additional HTTP request to 99.999% of the page loads that doesn't do anything. I wonder if we should rather warn about it in the readme and allow to opt into this behavior using a constant.

@akirk
Copy link
Member Author

akirk commented Jan 25, 2023

Let's postpone the decision until 0.6.

@psrpinto
Copy link
Member

Since the caching scenario is the exception (most sites won't have it), I would agree with merging this PR and "allow to opt into this behavior using a constant" (or similar) in a later PR. What do you think @ashfame ?

@psrpinto psrpinto added this to the 0.6 milestone Jan 25, 2023
@psrpinto psrpinto removed this from the 0.6 milestone Jan 25, 2023
@ashfame
Copy link
Member

ashfame commented Jan 25, 2023

I would say majority of the WordPress websites have some or the other caching plugin installed, because of which I would want to err on the side of caution. If a separate HTTP request is a concern, perhaps we can consider having that code inline? I am afraid there is no way to ensure logout without having some code constantly check for the presence of logout cookie.

I don't fancy a toggle/setting for this kind of behavior as it would be hard to explain it to begin with.

@psrpinto
Copy link
Member

psrpinto commented Feb 2, 2023

Is it possible to find data about what percentage of WordPress sites have caching? That would allows us to make an informed decision.

@ashfame
Copy link
Member

ashfame commented Feb 2, 2023

We can roughly take a look at installations of caching plugins:

  • W3 total cache 1+ million
  • WP Super cache 2+ millions
  • WP Rocket ~3 million
  • Litespeed cache 3+ million
  • WP Fastest cache 1+ million

That list alone is 10+ millions install. There are other plugins out there as well. And then there are services like Sucuri which caches everything and several hosting providers have their own caching layers.

@ashfame
Copy link
Member

ashfame commented Feb 14, 2023

@akirk and I discussed: Even though the current PR covers the logout functionality aspect for OIDC logins in a conservative/safe way, there are two concerns:

  1. Incorrect to assume upon WordPress logout, non OIDC based logins in Chatrix should logout as well
  2. Forced to load the logout script even when its not required

To cover both cases, it would be nice if we can make use of OIDC backchannel logut in Synapse. That would enable us to completely remove logout functionality from Chatrix.

@ashfame ashfame self-assigned this Feb 14, 2023
@ashfame
Copy link
Member

ashfame commented Feb 14, 2023

@psrpinto and I agree that we can merge this since its improving the existing logout functionality so that this is included in the next release we are packaging right now. Later on, based on backchannel logout feasability, we can remove it.

@ashfame ashfame merged commit 5a1e33c into main Feb 14, 2023
@ashfame ashfame deleted the limit-logout-code-output branch February 14, 2023 10:29
@ashfame ashfame mentioned this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only output logout script when the logout cookie is set
3 participants