#3 Roles & Permissions

[JN-Jones]

3

I added the pivot tables and relations for them so far. Also applied the necessary changes for the permission handling etc. Checking basic permissions works already, need to work on forum permissions next and caching permissions to avoid querying every single permission. Also need to look at the guest role.

Note: migrations aren't tested and the seed doesn't work as it isn't yet updated.

Also one question: atm I'm saving the permission value as integer (-1 as never, 0 as no, 1 as yes) and added constants to the PermissionChecker class. Should we probably save them as enum?

[JN-Jones]

Forum permissions work now too. Need to check them everywhere next and need to look how to easily call the getUnviewableIds function from places where we don't have the forum model available. Also need to check how subforums are handled best.

Caching will be added as last thing as it's meanly modifying the function in the PermissionChecker class.

@euantorano @wpillar can you check what I've so far to avoid changing everything later again? I'll commit the fixed seeder/migrations in a minute to allow easier testing. PSR-2 may not be applied everywhere but I need to rerun code refactor later anyways.

[wpillar]

Ok, so based on chats with @JN-Jones I've done a little pseudo code to layout how this could work a bit differently:

interface PermissionableInterface
{
    public function getContentName();

    public function getContentId();

    public function getNegativeParentOverrides();

    public function getPositiveParentOverrides();
}

trait Permissionable
{
    public function getContentName()
    {
    }

    public function getContentId()
    {
    }

    public function getNegativeParentOverrides()
    {
    }

    public function getPositiveParentOverrides()
    {
    }
}

class Forum implements PermissionableInterface
{
    use Permissionable;
}

class ModelFactory
{
    protected static $namesToModels = [
        'forum' => 'MyBB\Core\Database\Models\Forum'
    ];

    public function build($name, $id)
    {
        $class = static::$namesToModels[$name];
        $model = $class::find($id);
        return $model;
    }
}

class PermissionChecker
{
    protected $user;
    protected $modelFactory;

    // inject the current user automajically
    public function  __construct(ModelFactory $modelFactory, User $user)
    {
        $this->user = $user;
        $this->modelFactory = $modelFactory;
    }

    public function hasPermission(PermissionableInterface $permissionable, $permission)
    {
        $model = $this->modelFactory->build($permissionable->getContentName(), $permissionable->getContentId());

        // do whatever to check $permission for the $user against the $model
    }
}

I don't know the permissions system as well so this is probably a very naive solution.

The basic idea is to abstract permission checking logic into PermissionChecker so we can inject what we need there without relying on app() or static function calls. Add a PermissionableInterface that sets up the contract for all the stuff in the Permissionable trait but still use the trait for convenience.

The ModelFactory is tasked with resolving models from an ID and a name. At the moment this is a very simple class but can be easily iterated to allow extensions to add name->model mappings through service providers. This could even be a separate class, ModelRegistry or something.

So we then have a cleaner model without added logic or dependencies, just satisfying the simple contract and a unit testable PermissionChecker which has the power to resolve models and use them to determine permissions.

Thoughts? Specifically, is there any requirement I've missed that wouldn't be possible with this? If so, I'd like to try and work it in.

[wpillar]

Given it some thought and ModelFactory should be RepositoryFactory.

class RepositoryFactory
{
    protected $app;

    public function __construct(Application $app)
    {
        $this->app = $app;
    }

    protected static $namesToRepositories = [
        'forum' => 'MyBB\Core\Database\Repositories\ForumRepositoryInterface'
    ];

    public function build($name)
    {
        $class = static::$namesToRepositories[$name];
        return $this->app->make($class);
    }
}

class PermissionChecker
{
    protected $user;
    protected $repositoryFactory;

    // inject the current user automajically
    public function  __construct(RepositoryFactory $repositoryFactory, User $user)
    {
        $this->user = $user;
        $this->repositoryFactory = $repositoryFactory;
    }

    public function hasPermission(PermissionableInterface $permissionable, $permission)
    {
        $repository = $this->repositoryFactory->build($permissionable->getContentName());
        $model = $repository->get($permissionable->getContentId());
        $all = $repository->getAll();

        // do whatever to check $permission for the $user against the $model
    }
}

Think that works better.

[JN-Jones]

I added the canViewForum permission checks where needed and couldn't find any issues with it so far.

@euantorano: @wpillar suggested to move the hasPermission function to the PermissionChecker class and remove the trait(s). However I still think having it in the models is more usable. (Note that the canView* permissions are special cases and therefore I'm calling getUnviewableIds instead where possible, but other permissions like canDeleteTopics etc are handled with the hasPermission method). What do you think?

Also I've kept the cache disabled atm to make testing easier, that should be changed when the backend for permissions has been written.

[euantorano]

I'm not sure what the best approach is, but I wouldn't mind injecting the PermissionChecker class. I don't like making models any heavier than they have to be, and would rather see the logic in a single location where it's easier to maintain. Plus, IDEs tend to struggle with Traits that are intended to be applied to only one type of class.

[JN-Jones]

The main problem with moving it to one external class is the inheritation logic: atm it's pretty easy with the InheritPermissionable trait. I'm not sure what the best approach would be when we move it somewhere else. We'd still need to have an interface or something else to allow easily check whether the current permission content has a parent one and to get the parent one. At the end the model (or any other class using permissions) would need to have some of the functions anyways if we want to keep it easily expandable.

[wpillar]

Would having two interfaces work? PermissionableInterface and InheritPermissionableInterface which contains the methods for dealing with the parent? The latter extending the former?

[JN-Jones]

That'd work but then we would do a lot of special checks for it instead of simply overriding the method as it is now. As said: if we really want to keep it expandable we'd still need most of the functions and only the hasPermission function would get moved. Some of the function could be removed (getContentName and getUnviewableIds) but functions like getContentId, getParentOverride, getParent etc would still need to be added to the interface & models. Or we hard code them and force some things (eg that the content id is always the key, so instead of calling getContentId we'd call getKey then. However that'd mean that non-models also need that function so we'd also need to add it to the interface)

[wpillar]

Completely agree that we should be moving hasPermission() and the other methods would have to remain implemented on the models and be on the interfaces, but that's a tiny amount of overhead really.

[wpillar]

getUnviewableIds() is probably better on a repository, but not a big deal for it to remain on the model I don't think, as long as it's not accessing outside of the model.

[euantorano]

More merge conflicts @JN-Jones :tongue:

[JN-Jones]

Please merge the other PR first, it'll produce another conflict and I don't want to fix them over and over again :P