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

added names to routes #80

Merged
merged 1 commit into from
Apr 20, 2020
Merged

Conversation

iMokhles
Copy link
Contributor

added names to route, it's helpful for my own setup
i'm using permissions to determine if user has permission to access routes by names
here's my checkIfUserIsAdmin function
Capture d’écran 2020-04-14 à 17 08 55
and here's my permissions in my database
Capture d’écran 2020-04-14 à 17 09 43
it's really helpful to allow and disallow access for specific routes
idea from Laravel-Backpack/PermissionManager#203 (comment)

@welcome
Copy link

welcome bot commented Apr 14, 2020

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.
Please keep in mind that:

  • if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
  • even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
  • not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
  • we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!
-- Justin Case The Backpack Robot

@tabacitu
Copy link
Member

Totally agree - thanks for the PR @iMokhles , all routes should have names, for sure.

About your idea of turning route names into "permissions"... There's no other way to put it... I think it's freakin brilliant... 🤯 💥 So simple for both developer and admin, both to understand and to use... Then you'd only need:

  1. all routes to be under a middleware that checks for the permission (easy)
  2. an "import permissions" command, that adds all route names are as permissions in the db (not so easy)

And you'd still be able to add new permissions if you want, for things that are within a page or something (don't have a route). Excellent.

[Q1] The second one sounds a little tricky, to be able to have a command to import route names as permissions, from a php artisan route:list or something, but keep the old IDs (and relations) intact upon import. Did you already do that? If so, how? Does it also cover deleting old route names, that no longer exist?

[Q2] How does "Create User" form look, after importing the route names? Are there a lot of permissions then? Does it get very difficult to use?

Again, excellent idea. I freakin love it! I think it's the missing piece for a general roles&permissions system. One that would work for 80% of all applications. 🥳 🎈 I mean, if you need roles and permissions, that's probably what you actually need, to restrict people from doing stuff. And doing stuff is usually done with new routes. Brilliant!

@tabacitu tabacitu merged commit e77b447 into Laravel-Backpack:master Apr 20, 2020
@welcome
Copy link

welcome bot commented Apr 20, 2020

WHOOP-WHOOP! Congrats, your first PR on this repo has officialy been merged.
party
If you want to help out the community in other ways, you can:

  • give your opinion on other Github Issues & PRs;
  • chat with others in the Gitter Chatroom (usually for quick help: How do I do X);
  • answer Backpack questions on Stackoverflow; you get points, people get help; you can subscribe to the backpack-for-laravel tag by adding a new filter; that will send you emails when new questions come up with our tag;

Again. Thank you for the PR. You are a wonderful person. Keep 'em coming :-) Cheers!
-- Justin Case The Backpack Robot
P.S. Help in the Backpack community is rewarded with free Backpack commercial licenses. It's the least we can do. If you feel you've helped the community with PRs, help & other stuff, please shoot Tabacitu an email and ask him if you qualify for free licenses. You scratch my back, I scratch your back. Thank you!

@iMokhles
Copy link
Contributor Author

actually it's start point for my setup,
i'm adding permissions to database by a seeder and through this code

        $routes = app()->routes->getRoutes();

        foreach ($routes as $route) {
            if (!is_null($route->getName())) {
                \Backpack\PermissionManager\app\Models\Permission::create([
                    'name' => $route->getName(),
                    'guard_name' => 'backpack',
                    'created_at' => \Carbon\Carbon::now(),
                ]);
            }

        }

the only problem here is the getRoutes functions returns only the registered routes, that's means we need to seed the table each time we enable operation inside the CrudController because by default the CrudController enables only:

ListOperation
CreateOperation
UpdateOperation
DeleteOperation
ShowOperation

so other operation's routes won't be registered unless we enable it inside the CrudController.
and about deleting old routes names will be easy by dropping the table and re-seed the seeder class again.

i didn't add any routes permission for creating user form yet, i don't see any need for it ( right now ) but if it's important to use and check permission inside user's creation form i will try to figure out the best way to add it.

@tabacitu
Copy link
Member

tabacitu commented Apr 21, 2020

Hmmm... i’m not sure that’ll work well, with a seeder, i think it would be inconvenient when doing continuous deployment.

What if you:

  • finish the website, put it online
  • give everyone the permissions they need
  • need to add another page, or operation, or something that has a route; so you add the feature;
  • but when you re-deploy, you have to delete all entries inside permissions, and add them again (using the seeder); so you will have slightly different IDs; it is entirely possible that someone will get permission for something they’re not supposed to;

That’s why I prefer never to delete something that has an ID and relations. You risk compromising db integrity.

So I think a safer approach would be to do the seeding inside a Command (smth like php artisan permissions:seed). Then you could:

  • add this command to your “composer.json” inside scripts - to be run after every composer install or composer update (so upon auto-deployment it will get seeded)
  • make sure the command does NOT delete ALL routes, but only routes that no longer exist; and logs that deletion;
  • make sure the command only adds new entries for route names that aren’t already stored in the db;
  • maybe even create a button inside the Permissions CRUD, to trigger this command from the UI;

It’s a relatively small change, but I think it’s one that would improve the reliability of the system A LOT. At least that’s how I would do it, doesn’t mean it’s the only way to do it 😀

If you end up with something you like, please share it here too - would love to give it a go, maybe use it in some of my projects.

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

Successfully merging this pull request may close these issues.

2 participants