Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default urls are just terrible #5010

Closed
php4fan opened this issue Jan 31, 2022 · 6 comments
Closed

Default urls are just terrible #5010

php4fan opened this issue Jan 31, 2022 · 6 comments

Comments

@php4fan
Copy link
Contributor

php4fan commented Jan 31, 2022

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:
#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
Copy link
Collaborator

@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
Copy link
Contributor Author

php4fan commented Jan 31, 2022

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:

  • to have one route for all EA urls
  • to use query parameters, i.e. ?action=index&entity=User.... as opposed to /user/index.

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
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

@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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants