Discussion: Access Management

[SKuipers]

In reviewing #1673 and #1666, we're at a position in the refactoring to make some architectural decisions regarding access management. Currently, almost all user access to a certain page or permission is determined through the functions isActionAccessible() and getHighestGroupedAction(). These functions use a legacy style access to the database and need deprecated, as seen in #1673 and #1666.

Considerations

• Going forward, we will ideally be transitioning to using routing rather than filenames for access.
• The isActionAccessible() function is used widely, and nested in both other functions as well as class methods.
• The getHighestGroupedAction function uses the precedent field from gibbonAction to determine the highest action within a grouped set.
• Any access-related class will need access to the database to determine user permissions.
• The SessionFactory currently makes a determination of the current module and action based on the $_GET['q'] and $_POST['address'] variables, but this isn't the ideal place for it, as it isn't session based and should only exist the lifetime of the script.

The solutions in #1673 and #1666 are a great start. @yookoala I like your approach to parsing out the actions in a testable way, and using an AccessManager class to internalize the calls to the gateway. However, in reviewing them together, I have some thoughts on the proposed code changes:

• Both isActionAccessible and getHighestGroupedAction are action-related, so ideally could be handled with the same class.
• In general, I aim to avoid the use of new to create objects, using either the container or factory classes for instantiation. This helps keep class constructors flexible and maintainable for the future.
• In this particular case, I'd prefer to not create objects inside function parameters, for the sole use of passing them into a function. I wonder if this could be handled internally.
• It'd be ideal to not add new global variables to the scope, as they are more mutable and easily overwritten.

I've put together some ideas on how we might approach these in a unified manner, while also hopefully laying some groundwork for future routing.

Architectural Suggestions

• Create a single Access class in Gibbon/Auth namespace (bundling Authentication and Authorization in this namespace).
  • Create public static Access::isAllowed and Access::getHighestLevel methods.
  • This is a slight change in the terminology, with the aim for a more fluent syntax.
• Initialize the Access class in the CoreServiceProvider with an Access::initialize method to pass in the ActionGateway, rather than in the constructor. This should help with testing, as the gateway or methods could be mocked as needed.
• Return false from Access methods if not initialized.
• Construct the Access class with the current action, so that Access::getCurrentAction and Access::getCurrentModule methods return the module and action of the current page, using $_GET['q'] and $_POST['address'] in the interim until we move onto routing refactoring.
• Deprecate getModuleName and getActionName, having parsed and determined them internally in the Access class.
• Internally handle whether an action is using a filename (eg users_manage_add.php) or a route path (eg /users/add). This could be done by using the Action class within the Access class.
• Internally cache when the same action is checked multiple times, to reduce database calls.

Using a class with static methods does have its drawbacks, however in this case as a core service, like Format, I think it may be worth considering. It would give a nice fluent Access::isAllowed syntax, and could be dropped-in to replace functions without any dependencies (just a use declaration).

Here's my thinking for the Access interface contract. Let me know what you think. Alternatively, getCurrentAction could return an Action object, which exposes the getModule and getActionName methods, which may be more clean.

/**
 * Access class contract for determining if users can perform an action based 
 * on the configured permissions for their role.
 *
 * @version  v25
 * @since    v25
 */
interface Access
{
    public static function initialize(Session $session, ActionGateway $actionGateway);

    public static function isInitialized() : bool;

    public static function getCurrentModule() : string;

    public static function getCurrentAction() : string;

    public static function isAllowed(string $module, string $route, ?string $action = null) : bool;

    public static function getHighestLevel(string $module, string $route) : string;
}

@yookoala Let me know what you think. I'm aiming for something that builds on what you've started in your PRs, and ideally takes us one step further in the overall refactoring in the system, while still being backwards-compatible enough for legacy routes.

[yookoala]

I separate #1673 and #1666 to keep both PR simpler. Also I do not understand the access model of Gibbon enough to also combine getHighestGroupedAction into the access manager. Ideally, everything related to access should be the responsibility of the access manager, not some Gateway class.

Consideration

This is an important change in the framework. And the design matters. I'd prefer to stick to the single responsibility principle so things are more flexible to change later.

On one hand, we need a way to describe a particular action in details (i.e. module, route, additional action name). On the other hand, we need an actor that have knowledge about the current session / user and determine if she / he can do the particular action.

Hence I designed Action and AccessManager to be 2 distinct class.

• Action can potentially be passed around. It can also be something bigger than route (i.e. if a route provide multiple capability for users) or even be independent to route.

• The access manager instance can be provided by the CoreServiceProvider on demand and loaded by Controller through DI in the future. It was made global to simplify the refactor process.

To combine the 2 into 1 single class would probably force to stick with a particular way to describe action (i.e. isAllowed() sticks to $module, $route and $action) and I think this is not ideal.

Other Development Concerns

There are 1244 files using isActionAccessible(). It's a pain sticking task to do all these files by hand. I use sed and grep to automatically modify most of these files. Then I ran some auto checks for syntax issues. Having isActionAccessible refactor to handle both a route and an Action is simply easier to do than a single step rewrite.

#1673 was supposed to be the step 1 of the refactor. Once the change is approved. My next step AccessManager would be to convert all isActionAccessible calls with comparable AccessManager method. Since all the URL path parameters of isActionAccessible are already converted to Action, things should be much easier.

My Idea

To accommodate both isActionAccessible and getHighestGroupedAction change, this is my idea.

isActionAccessible

if (isActionAccessible($guid, $connection2, '/modules/Data Updater/data_updates.php'))) {
    // ... some logics ...
}

into something like:

if ($accessManager->getAccessOf(Action::fromRoute('Data Updater', 'data_updates'))->allow()) {
    // ... some logics ...
}

getHighestGroupedAction

$highestAction = getHighestGroupedAction($guid, '/modules/Activities/activities_attendance.php', $connection2);
if ($highestAction == "Enter Activity Attendance" ||
    ($highestAction == "Enter Activity Attendance_leader" && ($activity['role'] == 'Organiser' || $activity['role'] == 'Assistant' || $activity['role'] == 'Coach'))) {
    // ... some logics ...
}

into something like:

$access = $accessManager->getAccessOf(Action::fromRoute('Activities', 'activities_attendance'));
if ($access->allow('Enter Activity Attendance') ||
    ($access->allow('Enter Activity Attendance_leader') && ($activity['role'] == 'Organiser' || $activity['role'] == 'Assistant' || $activity['role'] == 'Coach'))) {
    // ... some logics ...
}

[yookoala]

Renaming the Action class as Resource might be more sensible. It represents combination of module and route path (i.e. URLList in the gibbonAction) table. The "thingy" that an access "allow" would be the actual "actions".

The logic goes:

• AccessManager get the Access information of Resource.
• Access contains the action(s) that is allowed for the user / role.

The class signatures:

namespace Gibbon\Auth\Access;

class Resource {
  public static function fromRoute(string $module, string $routePath): Resource;
  public function getModule(): string;
  public function getRoutePath(): string;
}

class AccessManager {
  public function getAccessOf(Resource $resource): Access
}

class Access {
  public function allow(?string $action = null): bool;
  public function allowAny(string ...$actions): bool;
  public function allowAll(string ...$actions): bool;
}

[SKuipers]

Thanks again @yookoala for all your work moving the codebase forward, this feature has been added to the core in v29, based on your initial PR.