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

Do not cache login/logout #659

Open
wants to merge 1 commit into
base: 1.13
Choose a base branch
from
Open

Do not cache login/logout #659

wants to merge 1 commit into from

Conversation

glye
Copy link
Member

@glye glye commented May 25, 2021

Proposed by community "to avoid several cache issues related to the out of the box login code". Is this sensible? CI is happy.

@peterkeung
Copy link

peterkeung commented May 25, 2021

Mugo discovered we were essentially adding this on every project, sometimes in different places in the stack (such as the Varnish config). So it would be great to standardize this in the CMS configuration as per the PR.

@glye glye requested a review from a team May 25, 2021 15:01
@Steveb-p
Copy link
Contributor

Makes sense, although I think this is only relevant to GET requests, as POST should not be cached anyway?

I summon thee @mnocon.

@ViniTou
Copy link
Contributor

ViniTou commented May 26, 2021

Two things:

"to avoid several cache issues"

What are those issues, maybe we just have problem in other places?

Login form should and afaik is generated thru non-cached esi call (at least for 3.3+ frontend commerce), what we are caching is everything around it.

I would say this will be quite big change if we are targeting 1.13 with it. How does it apply to 3.X line?

@thiagocamposviana
Copy link

thiagocamposviana commented May 26, 2021

Problem is that the POST request is sent to /login_check (and not /login), which redirects to /login ( this is a GET request ), which can cache information like the user login that failed, then the next user is going to see it if in the template we use some of the template variables, like {{ last_username }}.

/logout is always called without a POST request.

Like @peterkeung said, it can cause issues in almost every project.

@glye
Copy link
Member Author

glye commented May 27, 2021

I would say this will be quite big change if we are targeting 1.13 with it. How does it apply to 3.X line?

It applies fine, this config hasn't changed between 1.13/2.5/3.2, it has just moved.
https://github.com/ezsystems/ezplatform/blob/1.13/app/config/config.yml#L114
https://github.com/ezsystems/ezplatform/blob/2.5/app/config/config.yml#L127
https://github.com/ezsystems/ezplatform/blob/3.2/config/packages/ezplatform_http_cache.yaml#L2

@ViniTou What is the big change you refer to? What's the danger, e.g. performance trouble for existing sites when the default changes?

@thiagocamposviana Almost every project? So what's the difference between those affected and those not? Could it be about custom login templates or custom login handlers, for instance? Trying to narrow down what the problem is.

@ViniTou
Copy link
Contributor

ViniTou commented May 27, 2021

e.g. performance trouble for existing sites

yes. Maybe big is to big (;) ) word, but in front login form in commerce, we are caching whole page (with login form excluded) as for example it includes basket that can be cached per user.
Maybe we should apply similar treatment to admin login page as well, as I think it could be cached as whole atm.

Like I am not convinced to exclude whole path from cache if there are problems with just parts of it. But I am still trying to find myself in cache areas, so maybe someone else have better input on that (@adamwojs, @kmadejski )

@thiagocamposviana
Copy link

@thiagocamposviana Almost every project? So what's the difference between those affected and those not? Could it be about custom login templates or custom login handlers, for instance? Trying to narrow down what the problem is.

Projects with custom login/logout routes and also projects where the login template doesn't make use of variables that could be cached.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants