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

Cannot force client to cache authenticated responses using Entity cache properties #6730

Open
nathansalter opened this issue Oct 16, 2024 · 2 comments

Comments

@nathansalter
Copy link

nathansalter commented Oct 16, 2024

API Platform version(s) affected: 2.7, 3.0

Description
The documentation is not particularly clear about this, but you cannot set the max-age property on authenticated responses. This isn't an issue with API Platform as such, it's a feature that's supplied by Symfony. Basically Symfony will override all cache headers sent to it for requests with a session, and set max-age=0, private, must-revalidate for the cache headers.

See this code snippet: https://github.com/symfony/http-kernel/blob/7.1/EventListener/AbstractSessionListener.php#L200

There are many valid cases for when you'd want Client Cache to be possible in an authenticated session, especially when working with SPAs and other Clients. You've got the s-maxage setting which is not overwritten, but that's only for people using a Cache such as Varnish.

How to reproduce
Simply add the following operation to an entity with security enabled:

        new GetCollection(
            security: 'is_granted(\'ROLE_USER\')',
            cacheHeaders: ['max_age' => 3600, 'shared_max_age' => 3600, 'public' => true, 'expires' => null],
        )

When making requests, the cache header will be:

max-age=0, must-revalidate, private, s-maxage=3600

Possible Solution
In ApiPlatform\HttpCache\EventListener\AddHeadersListener, simply set the AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER header if the max_age property is set for the operation. It seems like it should be safe to assume that in these cases the developer understands the implications of having cache set on the client.

Additional Context
I've written a shim in our codebase to add this header for certain entities, but it would be simpler to have this reflected directly from API Platform

Copy link

stale bot commented Dec 15, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 15, 2024
@soyuka soyuka added enhancement and removed stale labels Dec 16, 2024
@soyuka
Copy link
Member

soyuka commented Dec 16, 2024

I like your solution, wdyt @dunglas?

Would you be able to open a PR on main with your changes?

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

No branches or pull requests

2 participants