[3.0 Cleanup] Removal of the SecurityContext #12445

linaori · 2014-11-10T13:30:10Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	~
License	MIT
Doc PR	~

As follow up for PR #11690 in 2.6, I have removed the SecurityContext, SecurityContextInterface and security.context service. I have split the parts in separate commits to make it easier to review but I can squash them if desired.

~~I will submit the changes to the documentation when a branch is available to do so.~~ Docs are already updated.

linaori · 2014-11-10T14:30:37Z

It's correct that the 5.3 and 5.4 tests fail. I have done this on purpose with ::class and []. 5.5 seems to be failing due to a caching issue:

1) Symfony\Bundle\SecurityBundle\Tests\Functional\AuthenticationCommencingTest::testAuthenticationIsCommencingIfAccessDeniedExceptionIsWrapped
RuntimeException: OUTPUT: 
Catchable fatal error: Argument 1 passed to Symfony\Component\Security\Http\Firewall\ContextListener::__construct() must be an instance of Symfony\Component\Security\Core\SecurityContextInterface, instance of Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage given, called in /tmp/2.6.0-DEV/StandardFormLogin/cache/securitybundleteststandardformlogin/appSecuritybundleteststandardformloginDebugProjectContainer.php on line 1767 and defined in /home/travis/build/symfony/symfony/src/Symfony/Bundle/SecurityBundle/vendor/symfony/security/Symfony/Component/Security/Http/Firewall/ContextListener.php on line 42

javiereguiluz · 2014-11-11T15:30:55Z

@iltar thanks for preparing this PR and for your work on the security component of Symfony.

However, I'd like to ask you to reconsider the title of this PR. The use of the "Extermination" word feels extremely strong and disturbing to me. If you agree, you could replace it with Removal.

linaori · 2014-11-11T15:37:09Z

@javiereguiluz No problem! I've changed the title

javiereguiluz · 2014-11-11T15:40:17Z

@iltar thanks for your quick response!

stof · 2014-11-23T14:42:14Z

src/Symfony/Bridge/Twig/Extension/SecurityExtension.php

 *
 * @author Fabien Potencier <fabien@symfony.com>
 */
 class SecurityExtension extends \Twig_Extension
 {
-    private $context;
+    private $authorization;


it should be authorizationChecker, not authorization (it is not an authorization, it is a checker)

stof · 2014-11-23T14:46:37Z

We should prepare the migration in a forward compatible way, to avoid people starting to work with 2.6 components to have to handle this BC break in 3.0. Constructors expecting a SecurityContextInterface should also accept a TokenStorageInterface or AuthorizationCheckerInterface (depending on the case), so that both the old and the new API are accepted.
then, 3.0 should only drop the support of the old one. See how we did it for the CSRF layer in Symfony 2.4

linaori · 2014-11-23T16:35:08Z

@stof to some degree I think it should be done. However, this will provide a lot of work for internal classes, that if overwritten by people, will only break things. Considering that just replacing it took me 8 hours, I don't mind keeping this PR open for a year for 3.0. I just don't think there will be any benefit doing this for 1 patch in 2.*.

fabpot · 2015-02-05T15:08:55Z

@iltar We are now ready to merge such PRs. Can you rebase on current master? Thanks.

linaori · 2015-02-05T15:12:02Z

@fabpot I will start working on it

fabpot · 2015-02-05T15:15:26Z

Thanks.

linaori · 2015-02-06T13:43:28Z

Seems like there are some unrelated test broken in the FrameworkBundle. As far as I can see, the tests and code I re-based is still working.

nicoschoenmaker · 2015-02-06T16:29:52Z

UPGRADE-3.0.md

@@ -511,6 +512,27 @@ UPGRADE FROM 2.x to 3.0
 ### Security

 * The `Resources/` directory was moved to `Core/Resources/`
+ * [BC BREAK] All usages of `SecurityContext` and `SecurityContextInterface` have been replaced by either the
+   `TokenStorageInterface` or `AuthorizationCheckerInterface`, this includes _all_ class.


Maybe change to "this includes all classes"?

What does this includes all class mean ?

fabpot · 2015-08-01T07:24:33Z

@iltar Can you rebase so that we can merge it? Thanks.

linaori · 2015-08-03T09:06:34Z

@fabpot The tests are flaky. Sometimes it runs out of memory, sometimes I get failing tests and sometimes they all succeed. I think it might have been cache related, but not sure.

fabpot · 2015-09-14T07:54:50Z

@iltar Sorry to ask you this again but can you rebase? Ping me when it's done and I will merge right away.

linaori · 2015-09-14T17:55:21Z

@fabpot rebase is done, I have no idea what phpunit did that made it unable to resolve a 2.8 dependency. I can't find the command that was executed that made fail so I cannot reproduce it myself, any way to find out which that was?

No idea why 3.0 is tested against a 2.8 dependency

Tobion · 2015-09-14T18:43:48Z

@nicolas-grekas should know about this (https://travis-ci.org/symfony/symfony/jobs/80268574#L2143)

sstok · 2015-09-15T06:48:36Z

UPGRADE-3.0.md

+ * [BC BREAK] All usages of `SecurityContext` and `SecurityContextInterface` have been replaced by either the
+   `TokenStorageInterface` or `AuthorizationCheckerInterface`, this includes _all_ class.
+ * [BC BREAK] Removed the `SecurityContext` class and `SecurityContextInterface`
+ * [BC BREAK] Constants from `SecurityContextInterface` are removed in favor of the constants in `Security`


Maybe provide a Fully qualified class-name for Security?

nicolas-grekas · 2015-09-18T11:20:44Z

@iltar changing the composer.json in 2.8 will fix the issue:

-        "symfony/http-kernel": "~2.2|~3.0.0"
+        "symfony/http-kernel": "~2.8"

this constraint should already come from framework-bundle since this is how it is there, but I guess composer has a bug here (mixing constraints from dev+not-dev+custom repository?)

stof · 2015-09-18T12:00:43Z

src/Symfony/Bridge/Twig/CHANGELOG.md

@@ -1,6 +1,10 @@
 CHANGELOG
 =========

+3.0.0
+-----
+ * [BC BREAK] `SecurityExtension::__construct()` now uses `AuthorizationCheckerInterface` instead of `SecurityContextInterface`


2.8 already allows both though

stof · 2015-09-18T12:09:12Z

src/Symfony/Component/Security/Core/Tests/Validator/Constraints/UserPasswordValidatorTest.php


        return $mock;
    }

    protected function createPasswordEncoder($isPasswordValid = true)
    {
-        return $this->getMock('Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface');
+        return $this->getMock(PasswordEncoderInterface::class);


this should be reverted. It makes merging branches harder (2.8 will keep using the string for the next 3 year of maintenance) and it does not add any real value to counterpart the maintenance pain

stof · 2015-09-18T12:10:30Z

I think it would be much easier to recreate the PR from scratch: 90% of the changes done in this PR have to be reverted.

linaori · 2015-09-18T14:23:37Z

@stof I agree, it's becoming a mess because the PR is pretty old. I will get to this the start of next week.

This PR was merged into the 3.0-dev branch. Discussion ---------- [3.0] Finish of the SecurityContext (follow-up pr) | Q | A | ------------- | --- | Fixed tickets | #12445 | License | MIT | Doc PR | ~ Because the original PR was getting too big for pretty much nothing (everything was already done in smaller commits over time), I have opened just this PR to confirm the changes by adding it to the CHANGELOG.md files. Commits ------- 351ebfc Updated CHANGELOG

linaori changed the title ~~[3.0 Cleanup] Extermination of the SecurityContext~~ [3.0 Cleanup] Removal of the SecurityContext Nov 11, 2014

stof reviewed Nov 23, 2014
View reviewed changes

fabpot added the Security label Dec 7, 2014

nicoschoenmaker reviewed Feb 6, 2015
View reviewed changes

sstok reviewed Sep 15, 2015
View reviewed changes

stof reviewed Sep 18, 2015
View reviewed changes

linaori closed this Sep 21, 2015

linaori mentioned this pull request Sep 21, 2015

[3.0] Finish of the SecurityContext (follow-up pr) #15855

Merged

Uh oh!

[3.0 Cleanup] Removal of the SecurityContext #12445

[3.0 Cleanup] Removal of the SecurityContext #12445

Uh oh!

Conversation

linaori commented Nov 10, 2014

Uh oh!

linaori commented Nov 10, 2014

Uh oh!

javiereguiluz commented Nov 11, 2014

Uh oh!

linaori commented Nov 11, 2014

Uh oh!

javiereguiluz commented Nov 11, 2014

Uh oh!

stof Nov 23, 2014

Choose a reason for hiding this comment

Uh oh!

stof commented Nov 23, 2014

Uh oh!

linaori commented Nov 23, 2014

Uh oh!

fabpot commented Feb 5, 2015

Uh oh!

linaori commented Feb 5, 2015

Uh oh!

fabpot commented Feb 5, 2015

Uh oh!

linaori commented Feb 6, 2015

Uh oh!

nicoschoenmaker Feb 6, 2015

Choose a reason for hiding this comment

Uh oh!

stof Sep 18, 2015

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 1, 2015

Uh oh!

linaori commented Aug 3, 2015

Uh oh!

fabpot commented Sep 14, 2015

Uh oh!

linaori commented Sep 14, 2015

Uh oh!

Tobion commented Sep 14, 2015

Uh oh!

sstok Sep 15, 2015

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Sep 18, 2015

Uh oh!

stof Sep 18, 2015

Choose a reason for hiding this comment

Uh oh!

stof Sep 18, 2015

Choose a reason for hiding this comment

Uh oh!

stof commented Sep 18, 2015

Uh oh!

linaori commented Sep 18, 2015

Uh oh!

Uh oh!