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.

[maxceem]

@RishiRajSahu what to do you think about the suggestion above?

[RishiRajSahu]

@maxceem approach of making the method generic is good idea. Below is my input on that -
1.yeah we can remove that
2.I am good with it but how would we know which property to read from entities ?
3.yeah we can first check for entities for project or phases, but still it has to be determined during runtime which is to be fetched, as per me its related to the answer of point 2 above
4.yep we will have to update the existing code if we are refactoring the method.

what is the handler we are using here, as per code it seems its defined inside permissions object but didn't found it in the permissions config ?

[maxceem]

what is the handler we are using here, as per code it seems its defined inside permissions object but didn't found it in the permissions config ?

handler is an optional function which can be provided inside a permissions config, but looks like we don't use it in any config https://github.com/appirio-tech/connect-app/blob/cf17/src/config/permissions.js

[maxceem]

2.I am good with it but how would we know which property to read from entities ?

which property to read would be determined based on which role we are checking. For checking projectRoles from the premission config, we would use entities.project and check member roles.

If some config option from permission would require us checking us something for phases, it would use entities.phases. So there is no issue here, we only change the format of passing data from multiple arguments to one object with arguments. The rest logic can stay.

[vikasrohit]

fyi @RishiRajSahu

[RishiRajSahu]

@maxceem Our permission model contains both the roles(topcoder/project) with each section/page on our Connect App, for example phases/project-plan section has both accessibility constraints with project roles. Also as per your example -
For projectRoles we will be looking for project key in entities but what about phases we don't have any phase roles ? If I am missing something here ?

[maxceem]

but what about phases we don't have any phase roles ?

You are right @RishiRajSahu we don't have any premission which need phases at the moment.

So at the moment, I guess we will only have project inside entities argument.

The reason why I propose to use entities instead of project because there is one place where phases is passed as third argument https://github.com/appirio-tech/connect-app/blob/dev/src/projects/detail/containers/ProjectPlanContainer.jsx#L167 - I guess it's redundant and can be removed as it's not really used my this method in this case because EDIT_PROJECT_PLAN permission doesn't define handler which can use the current third argument entity=phases https://github.com/appirio-tech/connect-app/blob/cf17/src/helpers/permissions.js#L23.

So to sum up:

• at the moment we don't use phases and we don't use handler
• but just in case we would need any other objects for the checkPermission method I propose to keep the second argument general, so we can add other objects if need inside it

Example possible situation:

• we don't show to the customers draft phases, so we can add some permission rule like VIEW_PHASE and pass inside phase or phases to check if the current user can see them based on user roles and on the phase status. Maybe we don't need such particular permission, it's just for example.

[RishiRajSahu]

@maxceem yep I agree with just two arguments and having generic entities which contains required field with it for example project, project-plan etc. so the first argument would be (permissionsObject, entities) .
My worry was around, how in the code we will know which object(project, project-plan etc.) to read from entities because the first argument doesn't give any info about that ?

[maxceem]

Got your question @RishiRajSahu

In general, the code inside checkPermission should know it. As entities is an object, all params inside it would have names.

1. In permissionsObject we can only define some limited set properties, for now only projectRoles and topcoderRoles. To check permissions defined by these properties, we may need some objects. For example to check projectRoles we need project object, and code inside checkPermission knows it, see https://github.com/appirio-tech/connect-app/blob/cf17/src/helpers/permissions.js#L9
2. If we would use some custom handler functions inside permissionsObjects, then these custom handler functions should know which params they need from entites.

[RishiRajSahu]

Got it via 1. , thanks ! @maxceem

[maxceem]

@RishiRajSahu This is done via PR #4169

QA Guidelines

1. As many small changes are made across the whole app, it would be good to perform a smoke test of the whole app.
2. Some major test cases I've listed in this document https://docs.google.com/document/d/1ptXsnSLYJIdzVy7Z8TGirTyq3c5U6TFRcK3SVYtyT_0/edit?usp=sharing.
3. Additionally it would be good to test that the progressive registration profile works well for all 3 user roles:
   • customer
   • copilot
   • Topcoder member

[lakshmiathreya]

@RishiRajSahu @maxceem @gondzo :

1. Dashboard view is same for Customer and others:
   Admin:

Customer:

[lakshmiathreya]

For Customer users - Dashboard needs all Phases expanded according to the requirement.

[lakshmiathreya]

2. Connect Manager is able to view Direct and Salesforce links for All Projects now. Is this intended? In Prod - Connect Manager is not able to view these links.

Test01 - User is tcconmanager:-

Prod - User is lakshmiaconmgr:-

[maxceem]

2. Connect Manager is able to view Direct and Salesforce links for All Projects now. Is this intended? In Prod - Connect Manager is not able to view these links.

   @lakshmiathreya the easier way to check what is correct bevaiour is to compare DEV and TEST environments with the same user:

   • https://connect.topcoder-dev.com/projects/16530 - this how it worked before changes in this ticket
   • https://connect-test01.topcoder-dev.com/projects/16530 - this is how it works after the changes in this ticket

   I've tried to join the project with Connect Manager user and I can see these links before and after the changes:

   

[maxceem]

1. Dashboard view is same for Customer and others:

That was indeed an issue. Fixed now:

[lakshmiathreya]

@maxceem @acshields @RishiRajSahu pls confirm that Connect Manager is expected to see Direct and Salesforce links for all Projects.

[maxceem]

@lakshmiathreya as per me, they should see it.

But it's better if @acshields or @RishiRajSahu confirm this.

[lakshmiathreya]

@acshields - would like you to review this https://docs.google.com/spreadsheets/d/1KTlFFsBaGtEJw6wtpELqpYyqORjWmahE_mK34m8WxWM/edit#gid=0 before I mark this Pass!

[RishiRajSahu]

@lakshmiathreya looks good to me and connect manager is expected to see those links. Another update from sheet only admins and copilot manager can invite copilots.

[RishiRajSahu]

Closing this as verified on prod. cc @lakshmiathreya