EASOL / easol

EASOL - A New Way to Open Learning with Ed-Tech
http://easol.org
GNU Affero General Public License v3.0
1 stars 4 forks source link

Permissions and Access Rules #315

Closed edgarf closed 7 years ago

edgarf commented 8 years ago

@regiscamimura 1) Could you please tell us whether "ACCESS" section in flex reports is applying any restrictions across the system? We would also need to add 2 more roles there. And the restrictions should be applied

2) We need to apply server level validation for roles when user is applying changes. For example - we don't allow for the user to edit the Flex Report (the button is not visible), but there is a possibility to get there by using the URL and also we need to validate it before saving the data itself. All functions should have permission validation.

3) Similar to 2) we should not allow non System/Data Admins to open student profile from another school.

regiscamimura commented 8 years ago

@edgarf:

1) The "Access" is ONLY used when you got to dashboard configuration section and the dropdowns for reports are filtered by the proper access for each Role. BUT if you change the Access, and lets say you removed access to "Educator" on a given report, but it was already added in the dashboard configuration, it will STILL be available in the dashboard. In that case, educators will also have access to report view page.

2) I just pushed a change on branch 315 to restrict access on flex report functions. Rules now are:

The "Access" field within a report it's not being used to control permissions on view page or dashboard though, as mentioned above.

3) I see. We can put a specific solution in place, or a general, global library that will allow us to expand the permission control over the system and its entities.

So, said that, I'm assuming we must change the permission system so we use "Access" to control reports, and also restrict data edition, in particular, students, to the selected school -- or the assigned school when the user is no system or data admin.

We have a quick specific option for that, or general global feature to control entities that can be expanded with time. The quick option, to use the "access" on reports and school on students is 4h/6h. The general feature is 6h/8h.

Please share your thoughts, will wait for feedback.

edgarf commented 8 years ago

@regiscamimura thank you for such detailed explanation. We are thinking now which approach to take. I will also analyze what other places are there which can violate access/security of the system (let me know if you have any thoughts already).

Additionally, could you describe what would be done during the "quick" and "general" solution? What would be the implementation strategy and what further scalability can we get from the "general" solution?

Thanks

regiscamimura commented 8 years ago

@edgarf Sure, he's a description of the solutions:

Quick solution: I'll just go through the controllers and implement specific validation on permissions. In case of access, I'll put something like that:

if (!in_array($report->getAccess(), Easol_Authentication::userdata('RoleId')) {
    // user has no permission, either redirect or display some message
}

And for students, it would be something like:

if (in_array(['Educator', 'School Administrator'], Easol_Authentication::userdata('RoleType') && $student->getSchoolId() != Easol_Authentication::userdata('SchoolId')) {
    // user has no permission, either redirect or display message
}

So those are very specific changes that doesn't have much flexibility nor can be reused in other places. Can't be expanded neither.

General Solution

We create a library for permissions, and add functions to check the permission on an entity. There are still specific scenarios, but we expand the permission system by adding functions to that library.

We can also implement an improvement to the whole permission system to use some meta-description of what each role can do. Right now, the permission system is based on the role name, System Administrator, Data Administrator, School Administrator, Educator. If those roles change somehow, the current way to verify permissions would be broken. So we could make it different, and read some meta-data on a role, so the system would support changes in the roles and still behavioring well. It would be easier to change the permissions to a given group/role without needing to re-create or re-setup users. But that's just a heads up, something we can do in the future, and using this "general approach" would make it easier.

Anyway, the library would have a function named has_permission. It would look like this:

function has_permission($type='controller', $id=null) { $function = "haspermission$type"; return $this->$function($id); }

function has_permission_controller() { // this function would check if the user has access to the current controller }

function has_permission_student($id) { // this is a function that checks if the user has permission to see or edit a student. It will grab it's id, check the relationships and compare against the logged user relation to schools. So, we could call it like if ($this->Easol_Permissions->has_permission('student', $StudentId). The big advantage is if we start using this function all over the place, and then the rules for permission changes, we just need to change this function, not going through the code and implement new logic all over it. }

function has_permission_report($id) { // Similar to the above. We would call something like $this->Easol_permissions->has_permission('report', $ReportId). This function would compare the given report "Access" values with the current role. }

Does that makes sense?

edgarf commented 8 years ago

@regiscamimura the general solution is looking well. I'm also thinking about other "permission" kind of restrictions we would need to applied. Below I have prepared a list of items which are related with permission levels. Some of those would be covered with your solution and I assume for others we'd need to add something new?

edgarf commented 8 years ago

Hi @regiscamimura, let's proceed with your "general" (bigger) option. Also, please have in mind all of the points above.

regiscamimura commented 8 years ago

@edgarf I have it done and working as described. There are more improvements we can do there though, including "merging" the functions that sets the menu items with the permission system (i.e, automatically remove a menu item when the permission wouldn't allow the user), clean up the code of old permission settings, review any security breaches regarding the current session usage, etc. But I'm already over the estimate. Estimate was 8h, I'm at 14h. So please let me know if you want me to keep working on that item or if we push to a later stage.

The new system variable will be in another push.

edgarf commented 8 years ago

@regiscamimura Thank you. Let's pause on this task and continue on the other ones. Afterwards, we can review whether more amendments are needed.

edgarf commented 8 years ago

P.S. Please also add: 1.CurrentSchoolYear (not current date, but the configuration value from School Configuration) 2.CurrentTerm (the same like above). system variables. Thank you!

edgarf commented 8 years ago

@regiscamimura, I'm testing the updated access and still have found these issues

Thank you

regiscamimura commented 8 years ago

@edgarf

I did changes to fix permission so the itens you mentioned above are fixed.

The access is defined in a file at ./config/auth.php. On line 66, you'll find that:

$config['auth']['reports'] = [
        // The * means the rule for all views inside the Flex Reports area. In this case, note that "Educators" is not on the "*" list, so by **_default_** it won't have access to anything.
    '*'=>[
        'System Administrator' => true,
        'Data Administrator' => true,
        'School Administrator' => [
            'condition'=> ['report_has_access']
        ]
    ],
        // For the edit view, School Administrator is false, so it won't have access to that
    'edit'=>[
        'System Administrator' => true,
        'Data Administrator' => true,
        'School Administrator' => false
    ],
       // For just view the report, School Admin and Educator have access, but with one condition: the report must have being set with an access that matches the role of the logged in user. The function that performes that verification calls "report_has_access", a method of the class at ./libraries/Easol_Auth.php.
    'view'=>[
        'System Administrator' => true,
        'Data Administrator' => true,
        'School Administrator' => [
            'condition'=> ['report_has_access']
        ],
        'Educator' => [
            'condition'=> ['report_has_access']
        ]
    ],
];

Let me know if it needs further clarifications, ok?

Regarding setting the access in Flex Reports add/edit form, you actually can unselect items. That's a plain multiple dropdown box, so if you want to select multiple values, you can just hold the CTRL key and click the items you want. If you want to unselect items, you can click it again while holding the CTRL key and it will be unselected. Multiple dropdown boxes are not very usual, I agree, so we could also replace that with checkboxes. Want me to do it?

edgarf commented 8 years ago

@regiscamimura thank you I will validate it. No need to do checkboxes, thanks.