Add Organization Admin Role

[mhrivnak]

We need a new role for the authorizer. The general idea is that a user in this role should be able to do anything a super admin can do, but only within their own organization.

The test for having this role is: does the user have the "Administrator" OrgRole within the organization in question?

Privileges this role should definitely have:

• create and edit users within the org
• bulk-upload users via CSV
• change status of users in the org
• enroll users in events
• edit enrollment status
• create and edit events, exams, FileDownload, FileUpload tasks.
• view venue schedules
• view resource schedules
• assign users to tasks
• create CurriculumEnrollments
• create credentials and achievements
• grand credentials

and perhaps some more things that we will identify as we go, but this is a good start.

[jc0n]

What do you think about separating the crud dictionary for the "super admin" role into a file of its own? It seems that a few roles could borrow from the central definition. For this particular role (organization admin) as you said it is basically the same so all of the crud will be the same the only difference is with the methods.

Shameless plug: Looking at these role definitions in initial_setup, it seems another project I have been working on (https://github.com/jc0n/coil) would suite this perfectly. In short, the project is a configuration language which supports object oriented key-value pairs with references, lists, etc. The key-value tree is exposed as a regular python dictionary. The original implementation is in pure python (http://code.google.com/p/coil/) but mine is in C with a python extension and performs around 250x faster for common cases. I don't anticipate that you will want to port all of the role definitions to a different format (though it is mostly the same), something to think about for future revisions or projects.

[jc0n]

assign users to tasks

Should this include any crud operations on Task? Or, just 'create' privileges for Assignment. In the former case, Task will need an organization attribute. I am guessing you mean the latter.

[mhrivnak]

My only concern is that we don't accidentally give the org admin the
ability to do something they shouldn't. We would need an authorizer
check method for every single object type to check its organization
affiliation against that of the actor, and not all objects have org
affiliations. In that case, the actor would potentially get
permission against the object.

So, I think it's safest to create a new ACL of only those object types
that we know have an organization relationship, and then we can verify
that each of those has an authorizer check method that will work for
it.

Does that make sense?

On Mon, Nov 28, 2011 at 10:53 PM, John O'Connor
reply@reply.github.com
wrote:

What do you think about separating the crud dictionary for the "super admin" role into a file of its own? It seems that a few roles could borrow from the central definition. For this particular role (organization admin) as you said it is basically the same so all of the crud will be the same the only difference is with the methods.

[mhrivnak]

You are correct that this does not require any write permissions on
Task. They will probably need to read the task, but not change it
just for the sake of making an assignment.

However, they will need create and update permissions on the
subclasses of Task, as noted in the issue with "create and edit
events, exams, FileDownload, FileUpload tasks."

On Mon, Nov 28, 2011 at 11:21 PM, John O'Connor
reply@reply.github.com
wrote:

Should this include any crud operations on Task? Or, just 'create' privileges for Assignment.

[jc0n]

When I say separate the admin role(s), I mean in such a way that other crud definitions (such as the one for the org admin) can borrow. Say in the case of the crud we know for the org admin where we do want full admin privileges for certain objects we can just reference the existing admin definition.

ie.

from super_admin_role import admin_crud
crud = { 
    'SomeObject': admin_crud['SomeObject'],
    'OtherObject': ...
}

[jc0n]

I also think if we are going to continue the current method of using CSVData objects see comments about handling file uploads in database (#37 (comment)) that CSVData model will need to have an organization attribute. Otherwise we will be allowing anyone with access to those objects to read them regardless of org.

[jc0n]

It seems strange to allow this role to create things but not be able to delete them. Such as creating a venue but then not being able to delete because we can't validate that it belongs to the org. Using owner wont help if there is more than one admin.

[mhrivnak]

I see. In regards to referencing the admin ACL, I think that's fine. It also occurs to me that something like this might also be helpful for admin ACLs where we really do want all attributes available:

crud = {
    'User' : {
        'c' : True,
        'r' : facade.managers.UserManager().getters.keys(),
        'u' : facade.managers.UserManager().setters.keys(),
        'd' : False
    }
}

On that note, there's really no need for 'getters' and 'setters' to be instance attributes of managers. They should really be class attributes.

Lots of opportunities for cleanup!

As for deleting things, there are lots of cases where we really want to just "deactive" an object instead of deleting it, so we can preserve its history. We haven't done a great job so far of making sure that all such objects have an "active" boolean or similar, but that will need to happen eventually. Fortunately, that sort of thing is fairly easy to add on.

[jc0n]

I really like that idea.

I notice that my_django_model should be a class attribute as well. And model_class might be a better name :P

[jc0n]

We should also use sets rather than lists for the 'r', 'u' attributes since they are mostly tested for membership. It also would allow things like

crud = {
    'User': {
        'c': True,
        'r': admin_crud['User']['r'] - frozenset(('attributes', 'we', 'want', 'to', 'hide')),
        'u': admin_crud['User']['u'] - frozenset(('attributes', 'we', 'want', 'to', 'protect')),
        'd': True
    }
}

or if django supported python 3000 (which it really should!)

crud = {
    'User': {
        'c': True,
        'r': admin_crud['User']['r'] - {'attributes', 'we', 'want', 'to', 'hide'},
        'u': admin_crud['User']['u'] - {'attributes', 'we', 'want', 'to', 'protect'},
        'd': True
    }
}

This would also benefit authorizer checks with requested_attributes, if those were sets as well we could do this:

# check that all requested attributes are authorized
if requested_attributes <= authorized_attributes

[jc0n]

Michael, can you please take a look at these two commits: 18718d75e2f5d0049d6d285acf43d762e48e2a29 and 8f39552b373631af267a0b565a2047cc5acdc5f4. I want to make sure I'm heading in the right direction with this. It should be nearly complete for all of the cases above. Unit tests forthcoming.

[jc0n]

81fc82bd4065d99bd643d7c4f999c9690270d178 is pretty important as well. It is the admin_crud abstraction discussed above.

[mhrivnak]

Can only glance through it at the moment until later tonight, but it looks reasonable. Make sure all the check methods are verifying the actee type, and eventually we'll want them sorted in the authorizer.py file by actee type as the other methods are.

[jc0n]

They are sorted by method name at present but I can change it.

I think all of the methods added for this role are very specific to this role. It may keep the authorizer.py easier to read if we put all of these (and any other) specific methods somewhere else. It looks like with slight modification to the Authorizer._acl_checks_pass we check for callable, or function types, and define the methods elsewhere and then reference the functions themselves instead of the string. ie

methods = [{'name': some_check_method}]

[jc0n]

Actually it looks a bit more involved with the way the methods are stored and loaded from the models.

[jc0n]

We could save a lot of test code for verifying roles by parameterizing the auth_token in the relevant test classes. This is a good idea for the sake of validating the use cases for this role and future roles but may take some time to get right.

[mhrivnak]

Can you elaborate on this?

On Tue, Dec 6, 2011 at 5:12 PM, John O'Connor
reply@reply.github.com
wrote:

We could save a lot of test code for verifying roles by parameterizing the auth_token in the relevant test classes. This is a good idea for the sake of validating the use cases for this role and future roles but may take some time to get right.

---

Reply to this email directly or view it on GitHub:
#39 (comment)

[jc0n]

In writing the tests I have come across a case which I'm not sure how to verify. It seems that the authorizer methods do not have the ability to determine which crud operation is being performed. In the case of the organization admin creating a user the method which validates the User actee checks that the actor and the user have the same organization which is not the case only when the user is created.

I will push my branch in a few minutes so you can see what I'm working with.

[jc0n]

If you take a look at c3430bea0f9554dfc0a0a71e58f0d3ef5c48d7b0, you'll see the direction I'm heading. Obviously there will be a lot more that is added to CommonUserManagerTests and this should be done for each of the significant managers. I also realized that a lot of the tests in pr_svc_tests can be merged and later borrow from the pr_tests. I'm a big fan of DRY

[mhrivnak]

There are two solutions to this which come to mind:

1. Allow all org admins to create org-less users and then allow them
   to create a UserOrgRole where the org is their org and the user does
   not have other org relationships.

2. Allow passing of an organization with role as part of the
   optional_parameters to UserManager.create(). Verify that if the user
   has a UserOrgRole at permission checking time, it must have either no
   org or the same org as the creator.

On Fri, Dec 9, 2011 at 3:36 PM, John O'Connor
reply@reply.github.com
wrote:

In writing the tests I have come across a case which I'm not sure how to verify. It seems that the authorizer methods do not have the ability to determine which crud operation is being performed. In the case of the organization admin creating a user the method which validates the User actee checks that the actor and the user have the same organization which is not the case only when the user is created.

I will push my branch in a few minutes so you can see what I'm working with.

---

Reply to this email directly or view it on GitHub:
#39 (comment)

[jc0n]

I can't find a reasonable way to authorize operations on an Exam for this role without a CurriculumTaskAssociation. It seems that you would not typically have an exam without the task association but this is not enforced anywhere (that I can tell) and ExamManager exposes a create method which makes me think at least this operation should be supported regardless of the curriculum task association. In this case, there is no way to associate an exam with an organization.

Suggestions:

(1) Add an organization attribute to Exam (or Task). I don't really think this is the way to go.

(2) Expose a variant of create in exam manager that requires a curriculum such that a CurriculumTaskAssociation can be automatically created. Then authorize just the creation of the task association. Then the task authorizer methods can just check for the association (with the correct org).

[jc0n]

For future reference: Issue #72 provides the fix for the above (associating Exams with an organization)

[jc0n]

Reopening this issue for the following use case:

The role shall be able to modify user not assigned to an org with 'pending' status; actor can assign that user to the actor's org and change user status to 'active'.

[mhrivnak]

Also need:

• Apply all permissions to not just the user's org, but all child orgs
• Read all Organization objects and update those that are the administered ones.
• Read all Group objects.

[mhrivnak]

Need the ability to read the "organization" attribute of tasks that are owned by orgs the actor doesn't control. Right now, I am seeing the "organization" attribute on the tasks owned by an org I do control, but the "organization" attribute is missing on other tasks. I see this specifically on ExamManager.achievement_detail_view

[mhrivnak]

Also need to be able to create a TaskFee for a task that is owned by an org the user controls. Currently getting permission denied as an org admin.

create[<AuthToken: 999b336673544cb0b5c35990b2e68dec>, u'97430233-7392-10E0-725B-9A82D64E37F7', u'Default', u'', u'20', 3, None] = {'status': 'error', 'error': [23, 'permission denied', {}]}

Also need to be able to read the task fees on any Task. This means read permission for any TaskFee, plus read permission for the task_fee attribute of each task type.

[mhrivnak]

Also need the ability to read the following fields on every Credential: ['name', 'description', 'required_achievements'].

When requesting those fields, I am currently only getting the 'id'.

[mhrivnak]

Also need the ability to create Curriculum owned by an organization the user controls. Currently getting permission denied. Also need ability to read all curriculums.

[mhrivnak]

Also need ability to create groups.