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

2.0 - Simplified roles #9

Open
wants to merge 49 commits into
base: master
from
Open

2.0 - Simplified roles #9

wants to merge 49 commits into from

Conversation

@mnapoli
Copy link
Member

mnapoli commented May 22, 2014

  • master: Build Status Coverage Status Scrutinizer Code Quality
  • branch: Build Status Coverage Status Scrutinizer Code Quality

Role definitions

Updated example:

$roles = [
    'ArticleEditor' => [
        'resource' => Article::class,
        'actions' => new Actions([Actions::VIEW, Actions::EDIT]),
    ], // same as the next one
    'ArticleEditor' => [
        'resource' => Article::class,
        'authorizations' => function (ACL $acl, RoleEntry $role, Article $article) {
            $acl->allow(
                $role->getSecurityIdentity(),
                new Actions([Actions::VIEW, Actions::EDIT]),
                $article,
                $role
            );
        },
    ],
    'CategoryManager' => [
        'resource' => new ClassResource(Article::class),
        'actions' => new Actions([Actions::VIEW, Actions::EDIT]),
    ],
    'Administrator' => function (ACL $acl, RoleEntry $role) {
        $allArticles = new ClassResource(Article::class);
        $acl->allow($role->getSecurityIdentity(), Actions::all(), $allArticles, $role);
    },
    'BackendUser' => [
        'resource' => new VirtualResource('backend'),
        'actions' => new Actions([Actions::VIEW]),
    ],
    'BackendAdministrator' => function (ACL $acl, RoleEntry $role) {
        $allArticles = new VirtualResource('backend');
        $acl->allow($role->getSecurityIdentity(), Actions::all(), $allArticles, $role);
    }
];

Grant

Before:

$acl->grant($user, new ArticleEditorRole($user, $article));
$acl->grant($user, new Administrator($user));

After:

$acl->grant($user, 'ArticleEditor', $article);
$acl->grant($user, 'Administrator');

TODO

  • Simplified role definitions

  • #13 Unified handling of resources through ResourceInterface and ResourceId

  • Add isGranted($role)

  • Simplified ACL Doctrine setup ?

  • Update documentation

  • Upgrading guide

  • Document RoleEntryRepository::findByRoleAndResource($roleName, $resource)

  • Remove unGrant()

  • Rename SecurityIdentityInterface to Identity

  • PHP 5.5

  • More tests?

    Removed from 2.0:

  • Virtual resources (would impact too much how ResourceId works, lot of work, might need a new column in database)

  • Decouple from Doctrine to allow alternative backends: too much work for now

@benjaminbertin
Copy link
Member

benjaminbertin commented May 22, 2014

Why not have a simple method to allow a role on multiple resources?

$roles = [
    'ArticleAndCategoryEditor' => [
        [
            'resource' => new ClassResource(Article::class),
            'actions' => [Actions::VIEW, Actions::EDIT]
        ],
        [
            'resource' => new ClassResource(Category::class),
            'actions' => [Actions::VIEW, Actions::EDIT]
        ]
    ],
];
@mnapoli
Copy link
Member Author

mnapoli commented May 22, 2014

$roles = [
    'ArticleEditor' => [
        'resource' => Article::class,
        'actions' => [Actions::VIEW, Actions::EDIT],
    ],
];

is the same as

$roles = [
    'ArticleEditor' => [
        'resource' => Article::class,
        'authorizations' => function (ACL $acl, Role $role, Article $article) {
            $acl->allow(
                $user,
                new Actions([Actions::VIEW, Actions::EDIT),
                $article,
                $role
            );
        },
    ],
];
@benjaminbertin
Copy link
Member

benjaminbertin commented May 22, 2014

IsAllowed should accept a resource as a string, then we could create a virtual resource (like an application backend).

@benjaminbertin
Copy link
Member

benjaminbertin commented May 22, 2014

All the possible roles definition could be:

$roles = [
    'ArticleEditor' => [
        'resource' => Article::class,
        'actions' => [Actions::VIEW, Actions::EDIT],
    ], // same as the next one
    'ArticleEditor' => [
        'resource' => Article::class,
        'authorizations' => function (ACL $acl, Role $role, Article $article) {
            $acl->allow(
                $role->getSecurityIdentity(),
                new Actions([Actions::VIEW, Actions::EDIT]),
                $article,
                $role
            );
        },
    ],
    'CategoryManager' => [
        'resource' => ClassResource::get(Article::class),
        'actions' => [Actions::VIEW, Actions::EDIT]
    ],
    'Administrator' => function (ACL $acl, Role $role) {
        $allArticles = ClassResource::get(Article::class);
        $acl->allow($role->getSecurityIdentity(), Actions::all(), $allArticles, $role);
    },
    'BackendUser' => [
        'resource' => VirtualResource::get('backend'),
        'actions' => [Actions::VIEW]
    ],
    'BackendAdministrator' => function (ACL $acl, Role $role) {
        $allArticles = VirtualResource:get('backend');
        $acl->allow($role->getSecurityIdentity(), Actions::all(), $allArticles, $role);
    }
];
@mnapoli mnapoli added this to the 2.0 milestone May 22, 2014
@mnapoli mnapoli added the enhancement label Jun 19, 2014
@mnapoli mnapoli changed the title Simplified roles 2.0 Jun 30, 2014
@mnapoli mnapoli changed the title 2.0 2.0 - Simplified roles Jun 30, 2014
@Evertt
Copy link

Evertt commented Oct 19, 2014

Hey are you guys still working on this project? I was trying to implement this library using your guide on github.io, but after I implemented the Role-classes I suddenly couldn't use the doctrine console anymore to update the database schema (more info here).

I'd love to see this pull-request get completed, because it looks like a lot easier way to define roles. Or is this project dead and would you guys recommend me to try another library?

@mnapoli
Copy link
Member Author

mnapoli commented Oct 20, 2014

@Evertt unfortunately this branch will probably not get merged (anytime soon at least).

The current version should work fine, it is used in production (I said should because the problem you are facing looks tricky!). However there is no big refactoring or new features planned for all I know. Bugfixes will be merged, but probably will not be provided by us if you find one. I hope that answers your questions :) (in short if you want full support and maintenance, you might not want this)

And I have no idea what's happening in your console sorry :(

@mnapoli
Copy link
Member Author

mnapoli commented Oct 20, 2014

To expand a bit: the production will stay at v1.* so if there's a 2.0, it's hard to maintain it without running it ourselves. That's why this PR is stalled right now.

@mnapoli mnapoli mentioned this pull request Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.