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

Discussion: Moving to policies for controller based authorization #3080

Merged
merged 22 commits into from
Dec 19, 2016

Conversation

dmeltzer
Copy link
Contributor

Hello,

This still has a long ways to go, just wanted to get the first bit into the wild so we could discuss whether we think this is a good idea. I think it will make everything cleaner as we add in controller gates, and it will allow for much more granularity of permissions in the future as as policy methods automatically have the Item and the user as parameters.

This PR looks huge until the delete PR is merged and this is rebased, but it's just the last commit that has the changes of importance here.

@dmeltzer dmeltzer force-pushed the work-on-policies branch 2 times, most recently from 8fce9d6 to b62ec35 Compare December 18, 2016 04:00
Add an artisan command for installing a settings setup on travis-ci
@dmeltzer
Copy link
Contributor Author

All permissions have been moved to policies that make sense to move to policies. Unit tests have been written to check that we are gating properly.

This PR also fixes/enables functional tests on travis-ci.

Also, I've gone ahead and ported a lot of hardcoded /admin/blah urls to use the relevant routes, and fixed issues I encountered along the way. This should be mergable I think, and close to usable.

@dmeltzer dmeltzer changed the title [NOMERGE] Discussion: Moving to policies for controller based authorization Discussion: Moving to policies for controller based authorization Dec 19, 2016
@@ -5,20 +5,20 @@
$I->am('logged in user');
$I->wantTo('ensure that the accessories listing page loads without errors');
$I->lookForwardTo('seeing it load without errors');
$I->amOnPage('/admin/accessories');
$I->amOnPage('/accessories');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably eventually switch these to routes, since that's what we're using mostly now in the app

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'm not 100% sure what we want to do with acceptance tests long term. I think functional + api tests will replace most of it.

@snipe snipe merged commit cd8c585 into snipe:laravel-53-upgrade Dec 19, 2016
@dmeltzer dmeltzer deleted the work-on-policies branch December 24, 2016 16:27
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