Adding a function that grabs the ID from the request #4591

catharsisjelly · 2017-08-10T10:50:51Z

Subject

Added a RouteIdHandlerInterface component that will allow developers to change how the ID is taken from the route in the request. By default, it will take the ID from the ID parameter. But the intention is to be able to easily add a component that will decode any encoded IDs. For example using something like HashIds to secure routes from users.

Changelog

### Added
- RouteIdHandler that decides how to process the ID in the route on CRUDController

catharsisjelly · 2017-08-10T11:05:33Z

Controller/CRUDController.php

@@ -157,7 +157,7 @@ public function batchActionDelete(ProxyQueryInterface $query)
    public function deleteAction($id)


I wanted to suggest actually deleting the parameter in the action function as it is not used and a bit misleading

I'll open a separate issue for this so it is at least logged

jlamur · 2017-08-10T13:23:44Z

Hey @catharsisjelly, thank you for contributing to Sonata!

I'm targeting 3.x as it is the main branch I use, when I tried to switch it to 2.x the PR became huge and I got scared.

It's normal since 2.x is not maintained anymore. The purpose of this sentence in the PR template is to make sure you target master if your changes are not BC, and 3.x if they are. 2.x is never targeted.

About the PR itself, I think it would be better to migrate this function (CRUDController::getObjectIDFromRequest(...)) into the AbstractAdmin or even better IMO, you could create a new component (for example something like RouteIdHandler). Let other contributors decide what's best.

Anyway, I'm almost sure that simply creating a new function into the controller won't be accepted.

catharsisjelly · 2017-08-10T14:02:30Z

@jlamur No problem, thanks for taking a look and the feedback. I was looking for a quick solution to a situation that came up when using Sonata so hence the quick PR. I agree this could be farmed out to a service that does a very specific job possibly even resolving the object so I'll suggest it could be called ObjectResolver or ObjectIDResolver into the mix. I'll be happy to do this and then pull in the service via the container. Will wait for guidance from other Sonata peeps before changing though.

core23 · 2017-08-10T14:53:00Z

Controller/CRUDController.php

+     *
+     * @return array
+     */
+    protected function getObjectIDFromRequest(Request $request)


Why do you think this should be final? If I make this final then I can't override it which would defeat the reason I made it in the first place.

Sorry, didn't read the full PR body 😅

core23 · 2017-08-11T06:02:15Z

Controller/CRUDController.php

+     *
+     * @return array
+     */
+    protected function getObjectIDFromRequest(Request $request)


-getObjectIDFromRequest +getObjectIdFromRequest

OskarStark · 2017-08-18T11:53:17Z

any news here @catharsisjelly ?

catharsisjelly · 2017-08-18T12:05:14Z

@OskarStark Sorry I missed the last bit of feedback but have updated the function name, is there anything else that needs changing/adding?

jlamur · 2017-08-18T12:12:15Z

@OskarStark Do you have any opinion on what I wrote before?

OskarStark · 2017-08-18T12:21:28Z

I like the idea of the RouteIdHandler, it could be very powerful, what do you think @core23 @greg0ire ?

greg0ire · 2017-08-18T12:26:09Z

Not adding anything to the AbstractAdmin is best, a new component would indeed be great. You would solve your issue by implementing the interface of this component as you see fit.

catharsisjelly · 2017-08-18T12:44:24Z

@jlamur @OskarStark @greg0ire I'll move this out to a RouteIdHandler service over the weekend if I find the time, thanks for the feedback

catharsisjelly · 2017-08-18T15:28:43Z

I've added the handle, was not sure where to put it so any advice on where you might want it. Test has been written, let me know if there is a better place to put it or you want to implement this differently.

greg0ire · 2017-08-18T15:30:48Z

Route/RouteIdHandler.php

+    {
+        return $request->get($admin->getIdParameter());
+    }
+}


Please configure your IDE to always add a newline at end of file, this will make the red pictogram disappear.

Same for the test file.

Done and done, see next push

greg0ire · 2017-08-18T15:31:57Z

Route/RouteIdHandler.php

+use Sonata\AdminBundle\Admin\AdminInterface;
+use Symfony\Component\HttpFoundation\Request;
+
+class RouteIdHandler


Should be final and implement an interface

You want me to just make a RouteIdHandlerInterface or something more generic?

I'd like to see your implementation, with the hash decoding, it will help understand what the purpose of this is, and maybe a better name.

greg0ire · 2017-08-18T15:36:25Z

Can you show the override with the hash decoding so we better understand the issue?

greg0ire · 2017-08-18T15:37:55Z

Resources/config/core.xml

@@ -27,6 +27,7 @@
        <service id="sonata.admin.breadcrumbs_builder" class="Sonata\AdminBundle\Admin\BreadcrumbsBuilder">
            <argument>%sonata.admin.configuration.breadcrumbs%</argument>
        </service>
+        <service id="sonata.admin.route_id_handler" class="Sonata\AdminBundle\Route\RouteIdHandler"/>


This should be renamed to sonata.admin.default_route_id_handler. sonata.admin.route_id_handler should be an alias you would use for your custom implementation.

This means that you would use it for every admin.

jlamur · 2017-08-18T15:55:55Z

A short cookbook entry about it would be great! Required or not @greg0ire?

jlamur · 2017-08-18T16:09:07Z

What I had in mind was RouteIdHandlerInterface::getIdFromRequest(Request $request) and RouteIdHandlerInterface::getIdFromSubject($subject). Also the service would have been added to the admin like for example the ModelManager, and could be overriden for each admin or globally.

And then the Admin would have used the RouteIdHandlerInterface service to generate urls.

catharsisjelly · 2017-08-18T16:12:07Z

@jlamur thank you that really helps clarify how it would be preferred to be done. I'm going home now but I'll make those changes as soon as I can.

greg0ire · 2017-08-18T16:14:27Z

@jlamur this makes the admin class grow though. The goal in the future is to split it into components, that it would still know about first, but that we would make independent after that. Let's think hard about how we can avoid editing AbstractAdmin.php at all.

greg0ire · 2017-08-18T16:15:43Z

Related : #2286 #4440

greg0ire · 2017-08-18T16:24:16Z

I think maybe the override by admin is not needed at first, what do you think?

greg0ire · 2017-08-18T16:33:35Z

Resources/doc/cookbook/recipe_hashid.rst

+Securing route id's with HashId
+===============================
+
+If you're working on an app where you need to hide the ID's from the routes then a good way to do ikt would be to use


"ikt" => "it"

"ID's" => "IDs"

greg0ire · 2017-08-18T16:35:03Z

Resources/doc/cookbook/recipe_hashid.rst

+        return $hashId->decodeHex($request->get($admin->getIdParameter()));
+    }
+}
+```


Please comply with the sf docs standards:

Each line should break approximately after the first word that crosses the 72nd character (so most lines end up being 72-78 characters);

Ah, I've never written RST docs before, I will check the standard and work on the cookbook doc first so it will help understand the change that is needed.

greg0ire · 2017-09-25T13:30:02Z

The less you override, the better

jlamur · 2017-09-25T13:31:55Z

@greg0ire You argued to not do it because it would make the admin class grow.

greg0ire · 2017-09-25T13:43:05Z

@jlamur is there no way to have the controller figure out things by giving it an admin and a route id handler? It could ask the latter to provide an encoded id, then call AbstractAdmin::generateUrl itself? But not calling generateObjectUrl would probably be a BC-break since that method is overriden, which means we should do that iff the route id handler is not the default one... ugly.

greg0ire · 2017-09-25T13:48:21Z

@greg0ire did you mean "outside the admin"?

No, I really did mean outside the controller. Anything not dealing directly with HTTP should be outside the controller.

catharsisjelly · 2017-09-25T13:56:48Z

My idea was this, to change generateObjectUrl to always use the handler so something like

public function generateObjectUrl($name, $object, array $parameters = [], $absolute = UrlGeneratorInterface::ABSOLUTE_PATH)
{
    $parameters['id'] = $this->container->get('sonata.admin.route_id_handler')->encode($this->getUrlsafeIdentifier($object));

    return $this->generateUrl($name, $parameters, $absolute);
}

By the looks of it I can get the container in AbstractAdmin by $this->getConfigurationPool()->getContainer()

That's a little convoluted perhaps but before I go change it all I wanted to just check we all have the same idea

greg0ire · 2017-09-25T14:05:25Z

@catharsisjelly I think it would be better to not touch the admin class, and avoid coupling it even more with the sf container, which looks like a mistake to me.

IMO we should merge this as is, and ultimately move all the route generation outside the admin class, in a service implementing just https://github.com/sonata-project/SonataAdminBundle/blob/3.x/Admin/UrlGeneratorInterface.php . But this is too much to ask you IMO

SonataCI · 2017-10-08T07:20:09Z

Could you please rebase your PR and fix merge conflicts?

SonataCI · 2017-10-08T12:52:05Z

Could you please rebase your PR and fix merge conflicts?

catharsisjelly · 2017-10-08T13:18:40Z

I seem to of royally foobar'ed the branch, I have another copy of it in the office. I'll force push that and rebase again on Monday.. DOH!

greg0ire · 2017-10-08T16:44:26Z

Otherwise, use git reflog and reset to a pre fuck-up state then force push

Changed functions that get the ID from Request in CRUDController to use this new service so that it can be replaced should the developer want to encode the Ids in their application for security measures

catharsisjelly · 2017-10-09T09:41:18Z

Branch no longer foobared.. Hurrah!

greg0ire · 2017-10-09T09:43:49Z

@core23 please review again

greg0ire · 2017-10-09T15:19:23Z

Congrats @catharsisjelly !

wingsergey · 2017-10-12T18:23:16Z

After composer update with this release I got [Symfony\Component\DependencyInjection\Exception\RuntimeException] Unable to resolve service "AppBundle\Admin\XxxAdmin": method "setRouteIdHandler()" does not exist.
On v3.23.0 all is ok.

Please investigate

jlamur · 2017-10-12T18:34:22Z

@wingsergey Please open a new issue with the full stack trace and bundle versions (or branch in this case, because this PR is not in any release).

catharsisjelly · 2017-10-12T19:17:13Z

@wingsergey I'm not seeing this, when you open the ticket can you reference this PR so I can take a look

…ject#4591)" This reverts commit 59ade03.

This reverts commit 59ade03.

SonataCI added the review required label Aug 10, 2017

catharsisjelly changed the base branch from 3.x to 2.x August 10, 2017 10:56

catharsisjelly changed the base branch from 2.x to 3.x August 10, 2017 10:59

catharsisjelly commented Aug 10, 2017

View reviewed changes

core23 requested changes Aug 10, 2017

View reviewed changes

core23 reviewed Aug 11, 2017

View reviewed changes

OskarStark added the pending author label Aug 18, 2017

SonataCI removed the pending author label Aug 18, 2017

OskarStark added the minor label Aug 18, 2017

greg0ire requested changes Aug 18, 2017

View reviewed changes

greg0ire reviewed Aug 18, 2017

View reviewed changes

greg0ire added the Needs: Documentation label Aug 18, 2017

greg0ire reviewed Aug 18, 2017

View reviewed changes

SonataCI added the pending author label Oct 8, 2017

catharsisjelly dismissed stale reviews from soullivaneuh and greg0ire via 5e2a716 October 8, 2017 12:45

SonataCI removed the pending author label Oct 8, 2017

SonataCI added pending author and removed pending author labels Oct 8, 2017

Adding RouteIdHandlerInterface and RouteIdHandler

d4301c0

Changed functions that get the ID from Request in CRUDController to use this new service so that it can be replaced should the developer want to encode the Ids in their application for security measures

greg0ire approved these changes Oct 9, 2017

View reviewed changes

core23 approved these changes Oct 9, 2017

View reviewed changes

greg0ire merged commit 59ade03 into sonata-project:3.x Oct 9, 2017

core23 added the hacktoberfest label Oct 9, 2017

jlamur added a commit to jlamur/SonataAdminBundle that referenced this pull request Oct 21, 2017

Revert "Adding RouteIdHandlerInterface and RouteIdHandler (sonata-pro…

4956871

…ject#4591)" This reverts commit 59ade03.

greg0ire pushed a commit that referenced this pull request Oct 22, 2017

Revert "Adding RouteIdHandlerInterface and RouteIdHandler (#4591)"

ad52e7c

This reverts commit 59ade03.

jlamur mentioned this pull request Oct 30, 2017

Allow to generate url in url field type with another parameter than identifier #4733

Closed

		@@ -157,7 +157,7 @@ public function batchActionDelete(ProxyQueryInterface $query)
		public function deleteAction($id)

Adding a function that grabs the ID from the request #4591

Adding a function that grabs the ID from the request #4591

Conversation

catharsisjelly commented Aug 10, 2017 • edited by soullivaneuh

Subject

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlamur commented Aug 10, 2017

catharsisjelly commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark commented Aug 18, 2017

catharsisjelly commented Aug 18, 2017

jlamur commented Aug 18, 2017

OskarStark commented Aug 18, 2017

greg0ire commented Aug 18, 2017

catharsisjelly commented Aug 18, 2017

catharsisjelly commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Aug 18, 2017 • edited

Choose a reason for hiding this comment

greg0ire commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlamur commented Aug 18, 2017

jlamur commented Aug 18, 2017 • edited

catharsisjelly commented Aug 18, 2017

greg0ire commented Aug 18, 2017

greg0ire commented Aug 18, 2017

greg0ire commented Aug 18, 2017

greg0ire Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Sep 25, 2017

jlamur commented Sep 25, 2017

greg0ire commented Sep 25, 2017

greg0ire commented Sep 25, 2017

catharsisjelly commented Sep 25, 2017 • edited

greg0ire commented Sep 25, 2017 • edited

SonataCI commented Oct 8, 2017

SonataCI commented Oct 8, 2017

catharsisjelly commented Oct 8, 2017

greg0ire commented Oct 8, 2017

catharsisjelly commented Oct 9, 2017

greg0ire commented Oct 9, 2017

greg0ire commented Oct 9, 2017

wingsergey commented Oct 12, 2017

jlamur commented Oct 12, 2017

catharsisjelly commented Oct 12, 2017

catharsisjelly commented Aug 10, 2017 •

edited by soullivaneuh

greg0ire Aug 18, 2017 •

edited

jlamur commented Aug 18, 2017 •

edited

greg0ire Aug 18, 2017 •

edited

catharsisjelly commented Sep 25, 2017 •

edited

greg0ire commented Sep 25, 2017 •

edited