Ensure that we have regression test coverage for unintended User's Access to Private Groups

The security incident from https://gitlab.com/gitlab-org/gitlab/issues/202520 affected a marquee account on GitLab.com.

Let's make sure to have adequate test coverage for this.

The issue is that the user in question does not appear in the member list via the UI or the API and evidence that they have access to these groups can only be seen on their profile page. Even checking their access via the admin area shows that they aren't listed as having access. If the user is impersonated by an admin though the three subgroups appear as groups they have access to.

@at.ramya please kindly provide input here.

/cc @edjdev

added Engineering Management ~9860889 labels

assigned to @meks

changed due date to February 17, 2020

changed the description

The regression test would need to be appropriately covered at lower level as, from what I understand, reproducing this issue requires a user belonging to (owner of?) a project with the same id as "the shared with" group. This coverage is coming along with the fix: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/202/diffs#633dcbcfddc22c34093868d4204fca7d31d5e4a2_173_150

However, as mentioned in the original test plan, we should add an end to end test for the happy path for share groups with groups feature but that should be tracked under ~"Quality:test-gap" and would be unrelated to this issue. I have created an issue for it here: gitlab-org/gitlab#205259 (moved)

Thanks for providing the details, Sanad!

Hi @sliaquat ,

With @cmaxim we are working on writing security tests.

At the moment we are doing end-to-end tests, but we have been told that due to the important amount of tests required to test the access permutations and inheritance (~300 tests), end-to-end tests may not be adequate for that. Those tests would need to be run whenever a change is made in groups and their permissions to ensure we are not breaking how it is supposed to be working. Any suggestions?

Thanks!

@vdesousa I believe a good place for such tests would be: https://gitlab.com/gitlab-org/gitlab/blob/master/spec/lib/gitlab/project_authorizations_spec.rb

Also, I think @ifarkas inputs and suggestion would be helpful here (as someone who had fixed this issue).

@sliaquat, yeah, spec/lib/gitlab/project_authorizations_spec.rb might be a good start, although I am not sure about every permutations. We should test various code paths there and I guess most of the 300 cases are redundant from that perspective. Also, that class is for testing project access. For group access, most of them are in spec/models/group_spec.rb and spec/policies/group_policy_spec.rb.

cc @jeremymatos, as you are also working on something similar.

We have a couple of engineers that are working to build out a test suite that will provide as much coverage as we can define for the group sharing feature. I know they have reached out to some QA engineers already for assistance.

Thanks @estrike!

Much appreciate your work on this.

Should this issue he assigned to the engineers working on this? gitlab-org/gitlab#205259 (moved)

Also, as mentioned here: #6555 (comment 287087943) we could use the suggested end-to-end tests mentioned in the test plan issue.

@estrike do we still have Security team working on this? Please let us know if need more input.

Thanks!

I believe @vdesousa reached out earlier in https://gitlab.slack.com/archives/C3JJET4Q6/p1581515931447700 I assume this is the same effort we are driving here.

@estrike Should the due dates be adjusted? Could you please advise on what the right date could be?

@vdesousa @cmaxim Could you please update the milestone and due date here since you are actively working on this per this slack message from yesterday ?

@vdesousa @cmaxim I've set the milestone to %12.9 . Please confirm whether it looks fine.

assigned to @at.ramya

changed due date to February 26, 2020

marked this issue as related to gitlab-org/gitlab#205259 (moved)

removed due date

changed milestone to %12.9

@vdesousa @estrike please kindly update us on the status of these new tests. We are tracking this as a high level item from Quality Department.

mentioned in merge request gitlab-org/gitlab!26066 (closed)

Latest update is in gitlab-org/gitlab!26066 (closed) the app sec team is working on ~300 new tests for user access permutations.

Let's help the team to be more efficient by dissecting these tests into unit and integration test layer. These are very top heavy in the pyramid currently.

changed milestone to %13.0

changed due date to April 17, 2020

changed due date to May 29, 2020

I've pinged in the related MR for any possible help needed for the Security team to close this out - gitlab-org/gitlab!26066 (closed)

changed the description

changed due date to August 31, 2020

I linked this issue to https://gitlab.com/gitlab-com/gl-security/secops/operations/-/issues/699 now that the approach from the security department has changed.

@sliaquat has graciously picked up work in the end-to-end test with gitlab-org/quality/testcases#934 (closed)

Pushing this due date so that Quality and Security teams have headroom to complete this.

changed due date to October 30, 2020

changed due date to January 31, 2021

changed due date to April 30, 2021

unassigned @meks

@at.ramya I am pushing this to end of Q2, I hope that once we fill the SET for groupimport that can free up Sanad to fully take on a deep dive in the ~"group::access" and it's permission test gaps.

We need to work with AppSec as well. The recent incident on CI tokens was a downstream implication from a security fix to very high GMAU groups in the verify stage (broken tokens) due to a change from the ~access::group. Much headroom for improvement opportunities here for the team #10546 (closed).

cc @laurence.bierner @jo_shih

Related end-to-end tests added and in WIP so far:

• Add end-to-end test for "Share groups with groups"
• Add minimal access user test for UI, API an CLI
• Add specs for user access termination after membership termination (WIP)

Hey team this seems complete, I will close this out based on the MRs already merged.

closed