Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$150] Fix copilotAndAbove permission to check that users is a member of the project #332

Closed
maxceem opened this issue Jul 5, 2019 · 27 comments

Comments

@maxceem
Copy link
Contributor

maxceem commented Jul 5, 2019

From @vikasrohit

API for adding phases to a project endpoint does not validate if the caller is a member of the project or not. So, a copilot of one project can add phases to any other project in Connect. I am not sure if this is the case with other phase or other entities options as well.

I see that permission copilotAndAbove doesn't check if the user is a member of a project or no, only user role of Topcoder, see https://github.com/topcoder-platform/tc-project-service/blob/challenge%2F30095006/src/permissions/copilotAndAbove.js#L11

This rule is used for all the CRUD endpoints for phases and products, see https://github.com/topcoder-platform/tc-project-service/blob/challenge%2F30095006/src/permissions/index.js#L43-L48

@vikasrohit

  1. Should managers only be able to update projects which are they member of or no?
@vikasrohit
Copy link

Connect Manager s should also be restricted to add phases to the projects they are part of. They can always join the project at their will, but this restriction would prevent any accidental modification of the plan.

@maxceem
Copy link
Contributor Author

maxceem commented Jul 5, 2019

@vikasrohit last question, what about admins? Should they also be members of the project to add phases?

@vikasrohit
Copy link

No, Connect Admin and administrators can do any change to any project.

@maxceem
Copy link
Contributor Author

maxceem commented Jul 5, 2019

To sum up:

  1. We should fix the permission checking method copilotAndAbove so:
    1. User with Topcoder admins roles should be able to perform operations. See example of checking Topcoder roles.
    2. Project members with copilot and manager Project roles should be also able to perform the operations. See example of checking Project roles.
  2. We should add unit tests for each endpoint which is used by copilotAndAbove permission method and test the next cases:
    1. Users with admin Topcoder roles can do actions even they are not project members
    2. Users with Project copilot roles can do actions
    3. Users with Topcoder Connect Copilot roles cannot do actions when they are NOT members
    4. Users with Project manager roles can do actions
    5. Users with Topcoder Connect Manager roles cannot do actions when they are NOT members
    6. Users with Project account_manager cannot do actions

@PrakashDurlabhji
Copy link
Contributor

@maxceem will it be open soon?

@maxceem
Copy link
Contributor Author

maxceem commented Jul 5, 2019

@vikasrohit to confirm, should account mangers be able to add phases and so on when they are members of the project?

@maxceem
Copy link
Contributor Author

maxceem commented Jul 5, 2019

@PrakashDurlabhji yes, it will be opened as soon as all the details are clarified. Note, as per this Bug Bash rules we do not allow to reserve issues. As soon as this issue is open for pick up, the first who comments will take it.

@PrakashDurlabhji
Copy link
Contributor

@maxceem sure i understand, whoever comments first when it is opened, issue goes to them

@vikasrohit
Copy link

should account mangers be able to add phases and so on when they are members of the project?

No, they are restricted to manage the projecgt plan.

@maxceem maxceem changed the title Fix copilotAndAbove permission to check that users is a member of the project [$150] Fix copilotAndAbove permission to check that users is a member of the project Jul 5, 2019
@maxceem
Copy link
Contributor Author

maxceem commented Jul 5, 2019

This issue is now open for pick up.

Issue summary is provided at the comment #332 (comment).

@romitgit
Copy link

romitgit commented Jul 5, 2019

Please assign to me

@PrakashDurlabhji
Copy link
Contributor

give to me thanks

@maxceem
Copy link
Contributor Author

maxceem commented Jul 5, 2019

Assigned to @romitgit as he was the first one to comment on this.

@romitgit
Copy link

romitgit commented Jul 6, 2019

@maxceem please unassign as I'm not able to finish this.

@maxceem
Copy link
Contributor Author

maxceem commented Jul 6, 2019

Thanks for update @romitgit. This issue is now open for pick up.

@mfikria
Copy link
Contributor

mfikria commented Jul 6, 2019

@maxceem is this issue open for public? If yes I want to work on this

@maxceem
Copy link
Contributor Author

maxceem commented Jul 6, 2019

Yes, @mfikria, it's a part of Bug Bash https://www.topcoder.com/challenges/30095031. Assigned you.

@mfikria
Copy link
Contributor

mfikria commented Jul 6, 2019

@maxceem I've made a PR in #336
Note that since the provided token for test in https://github.com/topcoder-platform/tc-project-service/blob/dev/src/tests/util.js limited. I simplify the test case for each endpoints into 3 cases:

  • Users with admin roles can do actions even they are not project members
  • Users with copilot roles cannot do actions when they are NOT members
  • Users with Project manager roles can do actions if they are members

@maxceem
Copy link
Contributor Author

maxceem commented Jul 6, 2019

Thanks, @mfikria.

I simplify the test case for each endpoints into 3 cases:

Can we also test 2 cases that a user with Connect Manager or Connect Copilot Topcoder roles cannot do actions to projects they are not members of?

I'm not sure what is the limitations in https://github.com/topcoder-platform/tc-project-service/blob/dev/src/tests/util.js to add such tests.

@mfikria
Copy link
Contributor

mfikria commented Jul 6, 2019

@maxceem do we need a jwt token with "no expiration" for test?

@maxceem
Copy link
Contributor Author

maxceem commented Jul 7, 2019

@mfikria
Copy link
Contributor

mfikria commented Jul 7, 2019

@maxceem actually i used those tokens in the unit tests

@maxceem
Copy link
Contributor Author

maxceem commented Jul 7, 2019

Yep so now we have cases:

  • Users with copilot roles cannot do actions when they are NOT members
  • Users with Project manager roles can do actions if they are members

And would like to also have opposite cases:

  • Users with copilot roles CAN do actions when they are members
  • Users with Project manager roles CANNOT do actions if they are NOT members

@mfikria
Copy link
Contributor

mfikria commented Jul 7, 2019

@maxceem the case "Users with copilot roles CAN do actions when they are members" is not needed since the existing successful unit test using copilot token

@maxceem
Copy link
Contributor Author

maxceem commented Jul 7, 2019

@maxceem the case "Users with copilot roles CAN do actions when they are members" is not needed since the existing successful unit test using copilot token

Ok, then only Users with Project manager roles CANNOT do actions if they are NOT members if we don't have such test yet.

@mfikria
Copy link
Contributor

mfikria commented Jul 7, 2019

@maxceem the PR has been updated.

maxceem added a commit that referenced this issue Jul 9, 2019
Fix copilotAndAbove permission to check that users is a member of the project #332
@maxceem maxceem added accepted and removed feedback labels Jul 10, 2019
This was referenced Jul 16, 2019
@maxceem maxceem added the paid label Jul 18, 2019
@maxceem
Copy link
Contributor Author

maxceem commented Jul 31, 2019

@vikasrohit this issue has been fixed and deployed with the release 2.4.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants