rosstuck commented 7 years ago

Road to 1.0

[x] Integration tests for the security middleware
[x] Review SecurityMiddlewareTest assertions (and label the data fixtures to see if we're hitting all the main points)
[x] Port DoctrineMiddlewarePass away from mocks (see https://github.com/thephpleague/tactician-bundle/blob/master/tests/DependencyInjection/Compiler/DoctrineMiddlewarePassTest.php for an example)
[x] Port CommandHandlerPassTest away from mocks
[x] Port ContainerBasedHandlerLocator away from mocks
[x] Fix incorrect docblocks (a number of them are missing type annotations, have old params, etc)
[x] Review Documentation

1.1

[x] Reduce complexity of main compiler pass
[x] Add Autowiring features (see #44)
[x] Add SecurityMiddlewarePassTest
[x] Add ValidatorMiddlewarePassTest
[x] Add tactician
[x] Add tactician:debug command to see command->handler service routing (see #73 )
[x] Review docs & changelog

1.2

[ ] Split validator middleware into separate package?

2.0

[ ] Support Tactician2...someday.

Misc

[ ] Start reorganizing README into the wiki or multiple markdown files

chalasr commented 7 years ago

tasks number 4 and 5 of Road to 1.0 addressed in #48

tyx commented 7 years ago

I give a try to the integration test of the SecurityMiddleware but with the following lines

$authenticationManager = static::$kernel->getContainer()->get('security.authentication.manager');
$authenticationManager->authenticate(new UsernamePasswordToken('admin', 'kitten', 'main', ['ROLE_ADMIN']));

I get Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException: The token storage contains no authentication token. One possible reason may be that there is no firewall configured for this URL.

I don't get how to handle that. I should admit I have a very small experience with this component. If anyone know how to simply authenticate a user with a single password without Request

rosstuck commented 7 years ago

Hmm, I haven't had this problem myself, but this might help?

http://symfony.com/doc/current/testing/http_authentication.html

The WebTestCase base class used above is just a thin addition to the same KernelTestCase base classes we use, some I imagine the same solutions apply.

RonRademaker commented 7 years ago

Addressed number 2 in #49

chalasr commented 7 years ago

Fix incorrect docblocks (a number of them are missing type annotations, have old params, etc)

Fixed in #51

chalasr commented 7 years ago

Add SecurityMiddlewarePassTest Add ValidatorMiddlewarePassTest

fixed in #52 , good to have

chalasr commented 7 years ago

increasing full test coverage of the security middleware

@RonRademaker Would you mind looking at this? Otherwise I can pick it, but I would first have to look at this middleware.

RonRademaker commented 7 years ago

I'll have some time to look at that in the upcoming days 👍

chalasr commented 7 years ago

Am I mistaken or do we only need to review the docs for a v1 to be out?

rosstuck commented 7 years ago

@chalasr I'm poring over the new security tests as we speak but it's starting to look that way.... 🌞

rosstuck commented 7 years ago

Unfortunately, I found some issues with the security unit tests, documented in #54. I have to go offline for a family event, I'm blocking in some time to go forward with review on Saturday though.

chalasr commented 7 years ago

@rosstuck I'll have a look at these meanwhile, maybe submit a PR targetting your cleanup branch

rosstuck commented 7 years ago

I'm on vacation right now but since we're not making much progress on getting the security issues figured out, I propose we disable/remove them for the time being so we can ship a release that will help the issues 3.3 users are experiencing. I feel bad doing this but I think it's the best option right now. If there's no complaints or alternate suggestions, I'll start on this next week

chalasr commented 7 years ago

I'm fine with postponing security related features.

RonRademaker commented 7 years ago

I'd like to see the security middleware in the next release (as I'm already using it in a project that is currently in development, but will be in production in a few months). I'm not exactly clear what the security issues are ATM. Are these clear so I can invest some time to address them?

rosstuck commented 7 years ago

I'd like to see it there as well because you have really been put through the wringer over it. The issues are entirely the test suite, I'm very hesitant to ship a security related piece without full integration tests.

On Jul 19, 2017 9:42 AM, "Ron Rademaker" notifications@github.com wrote:

I'd like to see the security middleware in the next release (as I'm already using it in a project that is currently in development, but will be in production in a few months). I'm not exactly clear what the security issues are ATM. Are these clear so I can invest some time to address them?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thephpleague/tactician-bundle/issues/47#issuecomment-316300172, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9To7uXF9VQOA9nxjEOq8nDkb1X7qJks5sPbNegaJpZM4NuFaX .

rosstuck commented 7 years ago

Moving again on #54, will do a bit more tomorrow as well (look at Scrutinizier issues, etc)

rosstuck commented 7 years ago

Okay, I've finished tests, updated the docs and walked through all the README examples in a fresh Symfony install. So far it looks really good!

The only thing that smacked me in the face was when I tried to run a command as an anon user. Symfony doesn't automatically give a anon ROLE_USER role so there's no way to configure some commands right now to be runnable by anon users (say, sign me up for the newsletter) and some to be authenticated users (bill this user) without splitting them into two separate buses or overwriting the token in token storage. We might want to address this or at least give a way of mitigating it in the docs.

Still, I don't see that as a blocker, so I'm thinking I'll tag an RC for 1.0 today and we can get that out there for some install and feedback! Sound good? :)

RonRademaker commented 7 years ago

:+1:

chalasr commented 7 years ago

👍

RonRademaker commented 7 years ago

The only thing that smacked me in the face was when I tried to run a command as an anon user. Symfony doesn't automatically give a anon ROLE_USER role so there's no way to configure some commands right now to be runnable by anon users (say, sign me up for the newsletter) and some to be authenticated users (bill this user) without splitting them into two separate buses or overwriting the token in token storage. We might want to address this or at least give a way of mitigating it in the docs.

I've also ran into this issue and worked around it using my own authorization checker with a dedicated CLI role, something like:

use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;

class WorkaroundAuthorizationChecker implements AuthorizationCheckerInterface
{
    /**
     * Container to retrieve the actual authorization checker from.
     *
     * @var Container
     */
    private $container;

    /**
     * AuthorizationCheckerFacade constructor.
     *
     * @param Container $container
     */
    public function __construct(Container $container)
    {
        $this->container = $container;
    }

    /**
     * Proxy to the authorization checker in the container.
     *
     * @param mixed $attributes
     * @param null $object
     *
     * @return bool
     */
    public function isGranted($attributes, $object = null): bool
    {
        $tokenStorage = $this->container->get('security.token_storage');

        if (null === ($token = $tokenStorage->getToken()) && php_sapi_name() ==='cli') {
            // workaround because cli wouldn't have a token
            $tokenStorage->setToken(
                new AnonymousToken('cli', 'anon', [new Role('ROLE_CLI'), new Role('IS_AUTHENTICATED_ANONYMOUSLY')])
            );
        } 

        return $this->container->get('security.authorization_checker')->isGranted($attributes, $object);
    }
}

This is the auth checker I inject in the security middleware.

rosstuck commented 7 years ago

It's been a couple weeks and I haven't heard of any major issues except the auth ones above, so I'll tag 1.0 today.

tyx commented 7 years ago

Indeed ! we are using the RC1 since the release and everything is fine.

rosstuck commented 7 years ago

Released, congrats and thanks again to everyone for all your hard work! 😄

Going forward, I think #44 and #60 are high on my priority list. Also, if y'all are interested, someone's also opened a PR for this bundle in the Symfony recipes repo https://github.com/symfony/recipes-contrib/pull/71#discussion_r135396790

rosstuck commented 6 years ago

I've just merged #67 into master. This adds the ability to have automatic wiring/mapping strategies. If y'all can give this a shot in your applications and open bugs for anything broken, I would greatly appreciate it!

See https://github.com/thephpleague/tactician-bundle#configuring-command-handlers to get started

Also, if you try using this in combination with the Symfony autowiring strategies, please post a sample of your config. I'd love to see what this looks like for most folks and get some samples into the documentation.

Thanks a ton!

rosstuck commented 6 years ago

Oh and I've updated the roadmap to push the new tactician:debug command to the next release since it's not a must-have IMHO and I don't have the energy to pick it up right now. If anyone is interested in doing so, please post on #71.

dmecke commented 6 years ago

Also, if you try using this in combination with the Symfony autowiring strategies, please post a sample of your config. I'd love to see what this looks like for most folks and get some samples into the documentation.

I have about 100 commands in my app, so I have a seperate command_handler.yml for the handler service definitions. Here is a before and after:

Before

services:
    onlinetennis.command_handler.cancel_showmatch_request:
        class: AppBundle\CommandHandler\CancelShowmatchRequestHandler
        arguments: ['@onlinetennis.repository.player', '@onlinetennis.repository.showmatch_request']
        tags:
            - { name: tactician.handler, command: Domain\Command\CancelShowmatchRequest }
    // 500 more lines

After

services:
    AppBundle\CommandHandler\:
        resource: "../../CommandHandler"
        autowire: true
        autoconfigure: true
        public: true
        tags:
            - { name: tactician.handler, typehints: true }

dmecke commented 6 years ago

@rosstuck Are you planning to tag the v1.1 in the near future? If you want to give it some more time, would it be possible to tag a v1.1-RC1 as you did with v1.0?

rosstuck commented 6 years ago

Hey @dmecke, I was hoping #73 would land first (since that would be good to have for folks struggling with the new system) but if that's gonna take a bit longer, then I can tag an RC-ish thing tomorrow.

rosstuck commented 6 years ago

Tagged v1.1-RC1

rosstuck commented 6 years ago

73 has landed, so there's now a debug command for seeing which commands are mapped to which services! Will review docs and update changelog, etc in next couple days then cut 1.1.0 if there's no blockers.

In the meantime, if anyone is interested in giving the debug command a spin on their full app, let me know if it works out for you. 👍

ogizanagi commented 6 years ago

Just tried on an app with ~70 handlers and 3 buses, works fine 👍 (nice work @kejwmen) I'm just wondering if debug:tactician would've been acceptable instead of tactician:debug as for other debug commands, even if it's not a symfony core one.

rosstuck commented 6 years ago

Good idea! Changed the command name for consistency. I've also tweaked a couple docs and cut the release, v1.1.0 is out. If anyone finds any bugs, please open an issue.

Otherwise, thanks for all your hardwork and testing, everyone :)

rosstuck commented 6 years ago

Tagged 1.1.1 (fixes an error using the default bus when absolutely no config is defined, improved version constraints, better docs, no BC breaks). Thanks for picking up the hard work on this one folks :)

lcobucci commented 6 years ago

Split validator middleware into separate package?

@rosstuck we're using the validator middleware with tactician in a Zend Expressive application and we just have some SF packages installed because of this bundle. With that said, could we help with splitting that middleware to another package? It should be pretty straightforward but what are your thoughts on this?

ogizanagi commented 6 years ago

@lcobucci : there is a pending PR regarding the validator middleware extraction -> #74

thephpleague / tactician-bundle

Roadmap #47

Road to 1.0

1.1

1.2

2.0

Misc

Before

After

73 has landed, so there's now a debug command for seeing which commands are mapped to which services! Will review docs and update changelog, etc in next couple days then cut 1.1.0 if there's no blockers.