Skip to content

[Security] Remove role classes that allow migrating session payloads from v4 #44855

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

Closed
wants to merge 1 commit into from

Conversation

ajgarlag
Copy link
Contributor

@ajgarlag ajgarlag commented Dec 29, 2021

Q A
Branch? 6.0
Bug fix? no
New feature? no
Deprecations? no
Tickets Related to #44676
License MIT

These classes where reintroduced in Symfony 5.3 to allow migrating session payloads from v4 (See #44805).
They should be removed in Symfony 6

@nicolas-grekas
Copy link
Member

I don't think they should be removed.

@nicolas-grekas
Copy link
Member

(even if I said so on the original PR :) )

@wouterj
Copy link
Member

wouterj commented Dec 29, 2021

@nicolas-grekas why not?

What discussed this morning was keeping BC with skipping (or "not updating all sessions") an upgrade step in production for a minor (e.g. 5.3 to 6.0). This is about skipping a whole major. I don't think we should care about those. So I'm 👍 for removing these

@nicolas-grekas
Copy link
Member

I think that there is no reason to make migrating to v6 harder.
Eg ppl that are on 4.4 today should be able to move to 5.4 then 6.0 in close steps.
Close steps means not being able to upgrade existing session payloads.
I think breaking BC at the data level is way worse than breaking it at the code level and that we should strive to preserve compatibility with older payloads.

@nicolas-grekas
Copy link
Member

Comments like #44676 (comment) make me think that.

@derrabus
Copy link
Member

I agree. We can keep those classes a bit longer.

@wouterj
Copy link
Member

wouterj commented Dec 30, 2021

I'm fine either way. Do we need to define a policy when this will be removed, or will we always keep this class like it is?

@ajgarlag
Copy link
Contributor Author

I'm fine either way. Do we need to define a policy when this will be removed, or will we always keep this class like it is?

I think it should be defined. When I opened the original issue, I couldn't find any reference to know if the session payload unserialization between major versions is supported or not.

My option here would be to support the unserialization from the previous major version only, but anyway it should be documented and tested.

@nicolas-grekas
Copy link
Member

This is tested already since #44805
When should we remove that? It'd say with Symfony v7, but there is no hurry really as we won't gain much from doing that.

@ajgarlag
Copy link
Contributor Author

This is tested already since #44805

I think we need to add two tests

  1. Guarantee that v5 payload can be unserialized in Symfony 6.x (branch 6.0)
  2. Guarantee that v3 payload can be unserialized in Symfony 4.4 (branch 4.4)

When should we remove that? It'd say with Symfony v7, but there is no hurry really as we won't gain much from doing that.

If session payload unserialization is going to be supported for at least two major versions, this policy should be documented. In that case, I think two additional tests would be required:

  1. Guarantee that v3 payload can be unserialized in Symfony 5.x (branch 5.3)
  2. Guarantee that v2 payload can be unserialized in Symfony 4.4 (branch 4.4)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 30, 2021

We shouldn't create this policy IMHO. Instead, we should have discussions and decide what's best at the time we're having such discussions. Which is what we're doing here. Thanks for starting it @ajgarlag. On my side, I think we have a decision here.

@carsonbot carsonbot changed the title Remove role classes that allow migrating session payloads from v4 [Security] Remove role classes that allow migrating session payloads from v4 Feb 9, 2022
@nicolas-grekas
Copy link
Member

Closing as discussed, thanks for proposing.

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

Successfully merging this pull request may close these issues.

5 participants