EasyCorp / EasyAdminBundle

EasyAdmin is a fast, beautiful and modern admin generator for Symfony applications.
MIT License
4.09k stars 1.03k forks source link

Default urls are just terrible #5010

Closed php4fan closed 2 years ago

php4fan commented 2 years ago

Backend urls should be nice and clean out of the box. Let's leave aside the signature, which I consider a useless feature but I'm not against it in and of itself (I'm never against a feature that is useless to me, it can always be useful in scenarios that I'm not concerned with). Let's consider urls with the signature disabled.

A url like this:

https://localhost:8000/admin?crudAction=index&crudControllerFqcn=App%5CController%5CAdmin%5CUserCrudController

should be something like this instead:

https://localhost:8000/admin/user/index

Of course the details are subject to taste, but it should be as simple, readable, clean and short as possible.

And having the full name of the controller class in the url?? Come on.

Actually the existence of the Url Signature feature, the fact that it is enabled by default, and the way you document it, gives me the impression that you consider ugly urls to be even desirable:

// by default, all backend URLs include a signature hash. If a user changes any
// query parameter (to "hack" the backend) 

"To hack the backend"? Writing a url by hand into the address bar is not hacking anything, it is a legitimate use of the browser. (And writing a link to a backend url from a source completely external to the application is also legitimate). Guessing a url is not hacking and should not be discouraged. A url easy to guess is a well-designed url.

If writing a url by hand is dangerous, then something is very badly designed at some other level. For example, if by writing a url such as https://whatever.com/whatever/delete into the address bar can lead to the deletion of something with no confirmation prompt, it means (among other things) that some action that should only accept POST requests is accepting a GET request (that's just a trivial example). Making the URL difficult to come up with (or even impossible to guess via a cryptographically strong signature) is no substitute for fixing that.

Now again, if one wants an extra layer of protection against potential misuses of whatever, the url signature is a nice feature to have. But especially since that is available, there is no reason for making the URLs ugly, and more importantly, redundant.

This:

https://localhost:8000/admin?crudAction=index&crudControllerFqcn=App%5CController%5CAdmin%5CUserCrudController&menuIndex=1&submenuIndex=-1

The menuIndex and submenuindex thingies seem to be responsible for highlighting the current item in the menu. If I remove them from the url, I reach the same page but unsurprisingly, the menu item is not highlighted. Worse, if I take another url and modify or add the menuIndex and/or submenuIndex parameters in the URL, I can get an arbitrary page with an unrelated arbitrary menu item highlighted. That's not the correct way to do it. The menu should be able to recognize that the requested url matches an item in the menu with no redundant parameters, and highlight it.

Oh well, and now I've found this: https://github.com/EasyCorp/EasyAdminBundle/issues/4268

🤦

I said "default urls" in the title because I haven't even yet investigated whether I can customize them. But they should at the very least be not flawed (if not short and pretty) out of the box by default.

javiereguiluz commented 2 years ago

@php4fan this is a key design decision and we're not going to change them. As we mention in the docs:

urls

I totally understand if this is a deal-breaker for you. Luckily there are many other open source projects that you could try to see if they manage URLs differently. E.g. for Symfony apps you have SonataAdminBundle and Api Platform Admin. Cheers!

php4fan commented 2 years ago

this is a key design decision

By the piece of documentation you cite, I seem to understand the "key design decision" you are referring to is "using a single route to handle all urls". That's a legitimate one, but that does NOT have the inevitable consequence of having all the flaws that your urls have.

First of all, I'm not sure using a single route means everything must be in query parameters as opposed as in the path. That is, I don't think that in symfony, for an url to be /admin/thisEntity/view/12 and another to be /admin/thatEntity/index, they have to be two different routes necessarily.

But let's ignore that and assume that you have made two key design decisions:

That is aesthetic. I don't like it, but it's aesthetic.

Having the url contain information that it shouldn't contain is not. Examples of that are the menu indexes and the referrer issue. Both are examples of objectively flawed design that is not justified by any tradeoff with anything.

javiereguiluz commented 2 years ago

@php4fan I don't know any reliable way of getting the referrer URL in the backend (you can't use the HTTP referrer header for example because many clients block it). So, we'd lose the nice "back to where you were" feature if we remove this URL param.

About the menu ... because of the single-route feature, any other way to implement this look more complicate to me. But I'm willing to listen to proposals to simplify this (simplify for end users and for us, maintainers). If we find a better solution, let's use it. Thanks!

lightglitch commented 2 years ago

@javiereguiluz I understand your decision about this point but are willing to make some changes to several classes to allow us to implement the "pretty urls" without having to fork all the code? By allowing to extend some classes and add some extra interfaces that would allow to implement this using the easyadmin core as a base?

Thanks!

javiereguiluz commented 2 years ago

@lightglitch I'm sorry to disappoint you, but we're not willing to make those changes that you asked. However, if it's an important feature for you, maybe you can add a listener/subscriber in your application to configure some common redirects in your backend. E.g. if adding clients is important and common in your backend, add a https://example.com/admin/add-client URL shortcut, catch it in the listener and redirect to the proper internal URL generated with the AdminUrlGenerator service. Same for other common URLs that you may use in your backend. Cheers!

lightglitch commented 2 years ago

@javiereguiluz Sorry to ear that. My goal was to implement my own listener and redirect the logic internally but if I can't use my version of "AdminUrlGenerator" (without copy all factories and actions logic) to generate my urls then just doesn't make sense to do a half bake feature. Another point was to reuse "AdminContextFactory" in my listener but I need to change how it fetches the parameters and right now that is also impossible. I think having flexibility wouldn't hurt the project but the final decision is yours and I have to find other options.