[Security] Security tweaks to SecurityExtension #1111

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@weaverryan
Member

weaverryan commented May 26, 2011

Hey guys-

Comments in the commit message. Due to their nature, the DI Extension classes tend to be hard to read. This includes several changes to SecurityExtension that were a result of me trying to go through the class, study, debug, etc.

Side note: I think the way that factories are processed (by calling SecurityFactoryInterface::create(), which returns an array of several things) can use some improvement. There are a few places where the return signature to functions is an array of several different items, which is hard to follow.

Thanks!

weaverryan added some commits May 26, 2011

[SecurityBundle] Heavy, but mostly meaningless cleanup of the Securit…
…yExtension class

This is mostly a result of trying to make the class more readable while studying it. Soem variable names were made more explicit, comments were added in many places, and a few other changes were made:

 * The security.context.listener.* services were renamed - the final portion was a counter before, it's now the firewall name, which makes more sense and is more consistent with the behavior of the rest of the class

 * User provider validation was added: if you specify a provider under your firewall or authentication listener that's not in the "providers" array, a LogicException is thrown
@stof

View changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
+ * Returns a service id that refers to a ContextListener with the given
+ * context key.
+ *
+ * @param \Symfony\Component\DependencyInjection\ContainerBuilder $container

This comment has been minimized.

@stof

stof May 26, 2011

Member

The CS are to use the short class name when there is a use statement in the file instead of the FQCN.

@stof

stof May 26, 2011

Member

The CS are to use the short class name when there is a use statement in the file instead of the FQCN.

This comment has been minimized.

@weaverryan

weaverryan May 26, 2011

Member

Thanks, I wondered about that - it definitely looks better that way, but my IDE isn't smart enough (yet) to figure that out.

I've just made this change.

@weaverryan

weaverryan May 26, 2011

Member

Thanks, I wondered about that - it definitely looks better that way, but my IDE isn't smart enough (yet) to figure that out.

I've just made this change.

This comment has been minimized.

@stof

stof May 26, 2011

Member

Which IDE ?

@stof

stof May 26, 2011

Member

Which IDE ?

This comment has been minimized.

@weaverryan

weaverryan May 26, 2011

Member

PHPStorm - it wants everything with FQN, and even doesn't recognize the correct class name of a method that uses an @return with just the class name (where the use is on top of the class).

@weaverryan

weaverryan May 26, 2011

Member

PHPStorm - it wants everything with FQN, and even doesn't recognize the correct class name of a method that uses an @return with just the class name (where the use is on top of the class).

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh May 26, 2011

Contributor

I would really like to merge the doc improvements, but I'm not so comfortable with the variable changes. The extension is far too low on unit tests for that.

Any chance you can extract the doc changes?

Contributor

schmittjoh commented May 26, 2011

I would really like to merge the doc improvements, but I'm not so comfortable with the variable changes. The extension is far too low on unit tests for that.

Any chance you can extract the doc changes?

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 26, 2011

Member

These extension classes are a pain to test since they're - effectively - just one giant public method with tons of logic.

Instead of backing out of these changes (which I hope add some clarity, but obviously I don't want to break anything) can we think of better way to organize things so that unit tests make sense? This entire "glue" layer of Symfony is complex and not tested.

The easiest thing I can think of is to make some of the private methods public. Obviously, they're not really meant to be public, but they would be testable and it would also help separate and define the individual "jobs" of each of these methods.

Thoughts?

Member

weaverryan commented May 26, 2011

These extension classes are a pain to test since they're - effectively - just one giant public method with tons of logic.

Instead of backing out of these changes (which I hope add some clarity, but obviously I don't want to break anything) can we think of better way to organize things so that unit tests make sense? This entire "glue" layer of Symfony is complex and not tested.

The easiest thing I can think of is to make some of the private methods public. Obviously, they're not really meant to be public, but they would be testable and it would also help separate and define the individual "jobs" of each of these methods.

Thoughts?

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh May 26, 2011

Contributor

Technically, we could use the reflection API to test the individual methods, but I'm not sure these tests would say much. How about adding some functional tests instead?

Contributor

schmittjoh commented May 26, 2011

Technically, we could use the reflection API to test the individual methods, but I'm not sure these tests would say much. How about adding some functional tests instead?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 26, 2011

Member

Look at the way Doctrine configurations are tested: it passes different configuration to test different cases and inspect the ContainerBuilder at the end. I think the same sort of tests could be done for SecurityBundle.

Member

stof commented May 26, 2011

Look at the way Doctrine configurations are tested: it passes different configuration to test different cases and inspect the ContainerBuilder at the end. I think the same sort of tests could be done for SecurityBundle.

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh May 26, 2011

Contributor

good luck then :p

On Thu, May 26, 2011 at 4:43 PM, stof <
reply@reply.github.com>wrote:

Look at the way Doctrine configurations are tested: it passes different
configuration to test different cases and inspect the ContainerBuilder at
the end. I think the same sort of tests could be done for SecurityBundle.

Reply to this email directly or view it on GitHub:
#1111 (comment)

Contributor

schmittjoh commented May 26, 2011

good luck then :p

On Thu, May 26, 2011 at 4:43 PM, stof <
reply@reply.github.com>wrote:

Look at the way Doctrine configurations are tested: it passes different
configuration to test different cases and inspect the ContainerBuilder at
the end. I think the same sort of tests could be done for SecurityBundle.

Reply to this email directly or view it on GitHub:
#1111 (comment)

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 26, 2011

Member

@stof: the Doctrine bundle tests do not help that much. They are very fragile and are not really that useful. I think that the only good way to go for testing the Security extension is to add some functional tests.

Member

fabpot commented May 26, 2011

@stof: the Doctrine bundle tests do not help that much. They are very fragile and are not really that useful. I think that the only good way to go for testing the Security extension is to add some functional tests.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 26, 2011

Member

@schmittjoh I never said it would be an easy task. But testing this class will be a lot of work whatever way you do it as it contains lots of logic.

Member

stof commented May 26, 2011

@schmittjoh I never said it would be an easy task. But testing this class will be a lot of work whatever way you do it as it contains lots of logic.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 6, 2011

Member

What do we do with this PR?

Member

fabpot commented Jun 6, 2011

What do we do with this PR?

@@ -58,7 +58,7 @@ class MainConfiguration implements ConfigurationInterface
// add a faux-entry for factories, so that no validation error is thrown
->fixXmlConfig('factory', 'factories')
->children()
- ->arrayNode('factories')->ignoreExtraKeys()->end()
+ ->arrayNode('factories')->ignoreExtraKeys()->addDefaultsIfNotSet()->end()

This comment has been minimized.

@stof

stof Jun 6, 2011

Member

This is useless a the factory entry is not used in the main config.

@stof

stof Jun 6, 2011

Member

This is useless a the factory entry is not used in the main config.

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Jun 6, 2011

Contributor

In its current form, we cannot merge it, but I see several options going forward:

a) extracting the doc changes
b) drastically improving the test coverage
c) not making any changes

I don't have time to work on the first two atm, but if someone has time, I think Ryan's doc changes are overall very good although most people probably won't look at the SecurityExtension.

Contributor

schmittjoh commented Jun 6, 2011

In its current form, we cannot merge it, but I see several options going forward:

a) extracting the doc changes
b) drastically improving the test coverage
c) not making any changes

I don't have time to work on the first two atm, but if someone has time, I think Ryan's doc changes are overall very good although most people probably won't look at the SecurityExtension.

@schmittjoh schmittjoh closed this Jun 6, 2011

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 6, 2011

Member

@weaverryan: Can you extra the doc changes?

Member

fabpot commented Jun 6, 2011

@weaverryan: Can you extra the doc changes?

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Jun 6, 2011

Member

Yes, I'll make another PR with just the docs and small changes.

Member

weaverryan commented Jun 6, 2011

Yes, I'll make another PR with just the docs and small changes.

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