Permissions handling

[gondzo]

With the new user roles we have a lot of places to update regarding permission checks so the code for permission checks will be more and more complex inside containers/components, ex

made up example
if ((util.isManager(user) && !util.isAccountManager(user)) || util.isCopilot(user) || util.hasAdminRole(user)){...}

How about centralizing permission checks to a common module (or configuration file)?
For example we could have a single method for permission checks like
util.checkPermission(PERMISSIONS.EDIT_PROJECT_PLAN, user, entity) with entity parameter being the one we're checking the permission for (ex project, plan, phase, timeline, thread, etc)
What do you think @vikasrohit @maxceem @RishiRajSahu ?

[maxceem]

@gondzo I absolutely agree that we have to centralize it. Also, several times came across this, when we redefine the same variables for roles in sevaral places:

The suggested approach looks good to me, if can describe all the relateion between user roles and user permissions.

[RishiRajSahu]

@gondzo yeah agree we could centralize it and let's use this centalize code for the Account manager role task only for now, as changing all the existing places for permissions check is risky. But going forward for newer checks we will be using this centralize method, even we could specify explicitly about using this in our future challenges specs.

[RishiRajSahu]

@maxceem can we include this as a part of CF ? we can define different sections of connect in different git issues or git checklist so that there are no bugs and well tested, as its a impacting change

[maxceem]

@RishiRajSahu
Do you think we should let the community to define names for the permissions by themselves from the code context, or should we create a list of all possible permissions first?

Also, I've noticed that here, we use checkPermission method which is combined with the old way of checking permission in the same line https://github.com/appirio-tech/connect-app/blob/feature/phase-tabs-notifications-red-cues/src/components/TeamManagement/TopcoderManagementDialog.js#L142
Do you know if it was intentional, or we are supposed to complitely move from using variables like currentUser.isAdmin and isMember and so on in favor of checkPermission method?

[RishiRajSahu]

@maxceem good point, I think the list of possible permissions should not be high only the places where they need to be put are lot, so I would like to rephrase my previous comment here, so not only different sections we need to define but also the different possible permissions

we are trying to completely remove using variables, it might be intentional as might requires defining new rules.

I think we ourselves should define the set of different possible permissions scenarios and leave all possible places to participants to apply those permissions.

[vikasrohit]

@maxceem @RishiRajSahu I would like to do this step by step. I mean, we should pick only one area of the application at time to replace the permission logic with the centralized system so that we can be sure of what are the potential area of failure after the release. So, could we identify different impact areas (e.g. team management is one area) first and add them as sub tasks of this issue?

[maxceem]

@vikasrohit yes, totally agree on this.

By the way, recently I got some idea. We have the same permission are being defined in several applications. And we always have to keep them in sync. For example.

Customer users cannot see private posts. So we have to handle this logic in three applications:

• message service
• connect app
• notification service

I don't suggest any particular solution at the moment. But probably in theory, we can define permissions somewhere in one place and use them across applications.

[vikasrohit]

Good idea @maxceem. However, I think we are not yet ready for moving this permission to a separate common place. Another reason is that we might get some changes in the way we authorize users after some upcoming changes e.g. organizations feature in Connect.

@RishiRajSahu @maxceem upto you guys, how you want to handle this for now. I think we can replace permissions check per bundle of features step by step for now.

[RishiRajSahu]

@vikasrohit yeah we can do it for connect only at first, in steps, with particular page/section of Connect chosen at a time to exploit permission model. Anyway once we have replaced all places with permissions model we can move it (in future when we are clear with all kind of authorisations required with respect to organisations feature) easily to central place/repo, also by then the changes we are doing for replacing will also be baked enough and foolproof.

@maxceem shall we start by creating a separate milestone/tag and under that create issues for each page/section of Connect ? and then how about running a CF for that ?

[maxceem]

@RishiRajSahu agree. I've created a project on Git for this: https://github.com/appirio-tech/connect-app/projects/13

I think we can start moving a section at a time as you suggested, so we could properly test small pieces rather than moving altogether. Will create the spec for some section.

and then how about running a CF for that ?

If we don't refactor all together, I think there is no need in a separate CF. We can include them in regular CF all handle via F2F.

[RishiRajSahu]

@RishiRajSahu so are we going to include some of the sections/items in the next CF ?

[maxceem]

@RishiRajSahu I'll try to include some portion.

[maxceem]

@RishiRajSahu I propose to slightly update checkPermission method before start migrating the rest of our code to this method.

1. Remove the third entity argument.
2. Rename the second argument from project to entities. The project should be a property of the entities argument. So the second entities argument may pass various objects we may need for checking permission, like project, phases and so on. And we don't have to add more arguments.
3. checkPermission method should get the current project object from the state instead of passing the project argument, like this:

   const project =  entities.project || _.get(store.getState(), 'projectState.project', null)
   

   So if we pass entities.project, then we use passed project object, otherwise, we get the project object from the state. This way mostly we don't have to pass the project object, as usually, we check permissions for the current project. If in some case we want to check permissions for another project, we may explicitly pass it in entities.project.
4. We should update the existent code where we use the method checkPermission to not pass project argument, or pass it as { project } instead, but only if there is any situation where we check permissions not for the current project.