Modular ACLS #91
Modular ACLS #91
Conversation
- Had the logic flipped for 'isAuthorized'
- Added hasAcl[s] functions to the XDUser class so that a particular user object can be interrogated as to whether or not it belongs to the provided Acl[s]. - Added hasAsset[s] functions to the XDUser class so that a particular user object can be interrogated as to whether or not it has access to the provided Asset[s].
- Think the odd behavior that lead to this change was due to rest testing w/ the same session id.
- Base Rest Authorization function now utilizes user acls as opposed to user roles. First implementation for PoC.
- Added the public acl as an actual acl as opposed to something that just lives in code.
- Added a default 'pub' acl to the public user.
- Exception had the wrong 2nd parameter, this was supposed to be the previous exception, not the code. Updated to be in line with the constructor arguments.
- Changed from authorization -> authentication because to be authorized you need to be authenticated first and at the point 'login' is called users are, by definition, not authenticated and therefor cannot be authorized.
- authenticate does not return a user object like authorization. So just retrieve the user object as normal.
- rejiggered the logic for what constitutes success in the new acl based authorization function.
- was just mucking up the logic.
- Just re-writing the logical conditions on what constitutes invalid requirements so as to be more intelligible.
- Updated the is[PublicUser, Developer, Manager] functions to utilize acls vs. roles ( since they're interchangeable ).
- Many calls to 'getRoles' utilized the values for their string / equivalency to the db column 'abbrev' ( which correlates to Acl->name ). Added a new '$names' argument with a default value of 'false' so that a caller can specify whether or not they want the object representation or just the keys ( 'name' values ).
- Updated the return value for getAcls($names=true) to use the array_keys function as opposed to iterating through each item and retrieving the name.
- Added some more detailed documentation for $_acls and $_assets.
- Added documentation types for all functions in the xd_utilities namespace that could be reasonably inferred. Also added @throws where appropriate.
- Swapped out $user->getRoles() for $user->getAcls(true). The result of $user->getRoles() was an array of $role->abbrev and is used to fill in a WHERE IN (role_values) clause. The result of $user->getAcls(true) is functionally the same, returning the equivalent value of the acl, ie. name.
- Just added acls to the returned User Details. Will remove roles when the front end has been modified to handle it.
- replaced the role based security w/ acl based security. It pains me to leave this in place but it will be overhauled when we finish moving the controllers to the new rest stack.
- the function was not used (hint: roles->getRoleCategories() was instead).
- Added the remaining DB models required to continue work on the ACL replacement. - Added table setup files for: filters acl_filters group_by_filters - Also added these new setup files to the acl_setup ACLTableManagement action
- user_acls.sql populates the initial values for this table w/ the contents of UserRoleParameters iff the values have not already been inserted. - report_template_acl.json just updates the table to be consistent with it's name ( which was fortuitous ).
- Added a script that will populate the ReportTemplateACL table with the initial values.
Merging remote work into another WC.
|
|
||
| $isAuthorized = function (Request $request, Application $app) { | ||
| $user = $this->getUserFromRequest($request); | ||
| $authorized = Authorization::authorized($user, array('mgr')); |
There was a problem hiding this comment.
The function authorized() always returns an array, which contains a status code. This should be checked.
Why don't you change the authorized() function to just directly throw the unauthorized exception, thus reducing the amount of boiler plate code that you have to write and maintain?
| $isAuthorized = function (Request $request, Application $app) { | ||
| $user = $this->getUserFromRequest($request); | ||
| $authorized = Authorization::authorized($user, array('mgr')); | ||
| if (!$authorized) { |
There was a problem hiding this comment.
The function authorized() always returns an array, which contains a status code. This should be checked.
Why don't you change the authorized() function to just directly throw the unauthorized exception, thus reducing the amount of boiler plate code that you have to write and maintain?
There was a problem hiding this comment.
Agreed ( and change incoming ) re: checking the status code.
The reason for the funky return format / not throwing an Exception inside authorized was to try and keep the code in BaseControllerProvider.php:276 - BaseControllerProvider.php:290 the same. But there's nothing to say that we can't move this logic into the 'authorized' function itself seeing as how we have access to the XDUser there.
classes/Realms.php
Outdated
| ); | ||
| } | ||
|
|
||
| public static function getRealmById($realmId) |
There was a problem hiding this comment.
What is this function used for? I can't find anywhere in the code that calls it. How has this been tested?
There was a problem hiding this comment.
Yeah, it looks like both getRealmByName and getRealmById are not currently used. Not sure what getRealmByName was about but I believe getRealmById was to support basic CRUD operations:
- GET /realms : list all currently installed realms
- GET /realms/{realm_id} : list information about realm identified by realm_id
- POST /realms : create a new realm
- DELETE /realms/{realm_id} : delete the realm identified by realm_id
- PATCH|PUT /realms/{realm_id} : update the realm identified by realm_id
etc. etc.
Seeing as how the route(s) that would utilize this function doesn't exist it pretty obviously doesn't need to be here. I'll go ahead and remove it.
classes/Statistics.php
Outdated
| use Exception; | ||
| use Statistic; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Don't forget that all new classes need to be in a namespace. Please make sure that all of the new classes that you have added are in a namespace. (I note that phpcs will find these errors).
| $old = $this->getBooleanParam($request, 'old', false, false); | ||
|
|
||
|
|
||
| $statistics = $old == false |
There was a problem hiding this comment.
This code looks like you are trying to support an 'old' mechanism as well as the new one. Why are you doing this?
There was a problem hiding this comment.
Not support so much as test that the new output matches the old. But it obviously shouldn't have made it into the PR.
There was a problem hiding this comment.
You should add unit and integration tests to check that the new output matches the old output.
There was a problem hiding this comment.
agreed, I'll make sure to have those added in the new smaller PR's
- Moved all of the new classes that lived in classes/ to classes/Models - Moved all of the new service classes that lived in classes/ to classes/Models/Services. - Updated all of the 'use' directives to point to the newly moved classes.
classes/Models/Hierarchy.php
Outdated
| @@ -0,0 +1,23 @@ | |||
| <?php namespace Models; | |||
|
|
|||
| use DBObject; | |||
There was a problem hiding this comment.
You should not "use DBObject" it is a class! DBObject needs to be in a namespace and then you can use the namespace that contains it.
Seeing as how we will not need these for a while / are not using them now. The asset infrastructure is being removed from this PR to clean things up.
- Moved the base 'DBObject' class into the Models namespace per review comment by @jpwhite4 - Removed 'use DBObject' statements as they are all now in the same namespace
- Moved the logic from 'BaseControllerProvider.php:273' into the 'authorized' function. - 'Authorization::authorized' now throws either an UnauthorizedHTTPException or AccessDeniedException based on whether or not the user is a public user when the authorization attempt fails. No Exception is to be interpreted as a success. The call sites for this function have been updated to reflect this change.
- Added the function back to 'Acls' ( Not sure when it was removed ) so that the AclsControllerProvider::getPermittedStatistics route works correctly. - Updated the SQL from it's original form so that it uses the current table layout.
| * ); | ||
| * @throws \Exception if the user provided is null | ||
| * if the requirements is not an array or it is an array but has no contents | ||
| * @throws \Exception if the user provided is null |
There was a problem hiding this comment.
User cant be null and it doesn't throw an exception for that. Please remove.
- Resolving a number of phpcs errors
- Updated per review comment by @plessbd
- Reverted as the UI changes that drove this change are not included in this PR.
- Added documentation to all of the functions with expected types where appropriate. - Changed the 'null === $variable' tests in 'listPermittedStatistics' to 'is_string($variable) === false' as 'is_string' will account for nulls and we really do expect these to be strings. - Small update to the sql query used in _listPermittedStatistics due to a column name change.
- Removed unneeded code
- Acl and AclType had invalid references to DBObject. Just updating them so that they're valid now.
- user_acl_parameters was replaced by user_acl_group_by_parameters as it was a more accurate representation of the data being tracked. It also has slight structural differences ( namely the fk to group_bys ).
- Added a use clause for Module as they no long reside within the same namespace - Added a use clause for Exception as there wasn't one previously.
note: I'll update this to not suck presently.
The intent of these changes is to provide an easy method for modules / module writers to interact with ACLS ( previously Roles ). In short, to take us from a hard coded set of roles / hierarchy to a more flexible architecture.
Description
The basic premise of how the system is used goes something like this: A module writer decides that they will require a new ACL with which to gate some/all of the functionality that they are providing. They write up a couple of json files to tell the system some basic information about their module and the acl they want to create. At install time these files are included / executed by an ETL action which incorporates them into the correct tables. Users can now be assigned the acl / they can check for it's existence in php code etc.
The overarching data flow is thus:
This flow, coupled with table foreign keys provides ample information when attempting to track down where a piece of information came from ( module ).
Motivation and Context
We have a very static set of roles / hierarchies at the moment.
Tests performed
All tests performed were manual tests utilizing both the site itself and Postman where appropriate to ensure that current data / behavior.
Types of changes
Checklist:
The intent of these changes is to provide an easy method for modules / module writers to interact with ACLS ( previously Roles ). In short, to take us from a hard coded set of roles / hierarchy to a more flexible architecture.