General permission methods

[maxceem]

During working on the Workstreams we had to check permissions based on allowRule and denyRule as was described in this comment #315 (comment). And it was implemented during the challenge in workManagementForTemplate permission middleware.

As we are going to have more and more places where we would have to restrict permissions based on various set of rules and to avoid issues like this one #332 I've moved the logic for checking permissions into general util methods:

• matchPermissionRule
• hasPermission
• hasPermissionForProject

See implementation: https://github.com/topcoder-platform/tc-project-service/blob/dev-next/src/util.js#L596-L748
For proof of concept I've refactored two methods for testing permissions using these general methods:

• copilotAndAbove
• workManagementForTemplate

The most general method is hasPermissionForProject: (permission, user, projectId):

• projectId is project id we should check permission for
• user is the user we should check permissions for, usually we would use the current user from the request object user = req.authUser
• permission

I suggest to be able to define the permission in two ways:

• Full when permission is defined with allowRule and denyRule:

  {
    allowRule: {
        projectRoles: [],
        topcoderRoles: []
    },
    denyRule: {
        projectRoles: [],
        topcoderRoles: []
    }
  }

• Simplified if we don't need denyRule we can deifne directly the inner part of allowRule without explicit property allowRule:

  {
     projectRoles: [],
     topcoderRoles: []
  }

  This simplified permission is equal to a full permission:

  {
     allowRule: {
       projectRoles: [],
       topcoderRoles: []
     }
  }

I guess this should cover most of our needs for permissions checking. And after some time we can refactor exitent code to use these methods instead of reimplementing this logic, for example many of these methods could be rewritten reusing the general methods.

Also, we can create constants with predefined reusable permissions, see example, same like we started to do in Connect app. Eventually, we should end up having the same or similar set of rules in Connect app and Project Service.

So far what I see can be done to improve them:

1. Create some way to check that the user is a member of the project with any member role. For now we would have to list all the user roles in projectRoles which is too wordy, also when adding a new role we would have to update all permission rules. So far I see two possible ways:
   1. Add new property like isProjectMember so the rule would look like:

       {
         isProjectMember: true,
         topcoderRoles: []
       }

      The big disadvantage I see, is that it may be misleading as it could feel that we have to add both propeties isProjectMember and projectRoles to check if user is a member and has a particular role.
   2. Another way - is somehow use projectRoles property to define all roles at once. Possible ways:
      1. projectRoles = [] - empty array, feels like areally bad idea
      2. projectRoles = true - boolean true is nice, the only small disadvantage I see, the word true doesn't exmplains what it means
      3. projectRoels = 'any' - string any, is good that it explains what does it mean, but it feels a little bit "hardcoded". We can introduce a new constant ANY = 'any', but then we would be required to always import this constant which feels not that comfortable.
      4. Maybe you have any other idea.
2. Optimize method hasPermissionForProject so in case there is not rule to check projectRoles we don't have to retrieve projectMembers from the DB, to make it faster in such case.
3. cover these methods with extensive unit tests
4. Validate all input params to make sure developer run these methods with the combinations which make sense and provide detailed errors otherwise, in particular but not limited:
   1. use Joi to validate possible shapes of permission to provide detailed feedback to the developer about possible method misusing.
   2. if there is any rule with projectRoles we should throw error if projectMembers or projectId is not defined. projectMembers may be still an empty array or even null.
5. Write wiki documentation on how to use these methods with examples.

@vikasrohit Let me know what you think. If you see any ways we can adjust these methods to cover more cases or to be more comfortable or reliable.

[vikasrohit]

Agreed with all the points and plan to improve permissions overall. I have suggestion for specifying the permission rule for user being a project member without being too wordy. We can have a constant PROJECT_MEMBER_ROLES and populate it with all possible member roles once and then use this constants in the permission configuration.

[maxceem]

I have suggestion for specifying the permission rule for user being a project member without being too wordy.

You mean instead of suggested ROLES_PROJECT_MEMBERS use PROJECT_MEMBER_ROLES ?

[vikasrohit]

No, I meant:
we have all project member roles listed here so we can create our permissions as follows:

{
   projectRoles: _.values(PROJECT_MEMBER_ROLE),
   topcoderRoles: []
 }

So, we don't need any new config field for saying all project members and we don't have to specify/update the project member roles every where.

[maxceem]

Got you. The reason for such option like projectRoles: true is that we can specify permissions not only in the code config file, but also in the DB records.

We already have several places where we specify permissions inside DB:

• ProjectSettings.writePermission
• ProjectSettings.readPermission
• WorkManagementPermission.permission

So if define inside DB value as:

{
   projectRoles: ['manager', 'observer', 'customer', 'copilot', 'account_manager'],
   topcoderRoles: []
 }

Later, if we add a new project role we would have to update all the records in the DB.

[vikasrohit]

Hmm, I think we should keep it simple for now and specify each role in projectRoles and topcoderRoles. We can optimize things later.