Fix effective access level of group members (!56677) · Merge requests · GitLab.org / GitLab

Jonas Wälter requested to merge siemens/gitlab:fix/group_members_max_access_level into master Mar 15, 2021

What does this MR do?

If a user is a member of both a group and its subgroup but with different access levels, the property access_level is not properly inherited from the parent group. Instead, the wrong effective access_level is shown in the subgroup (UI and API).

This bug is known described in the API documentation:

Due to an issue, the users effective access_level may actually be higher than returned value when listing group members.

https://docs.gitlab.com/ee/api/members.html#list-all-members-of-a-group-or-project-including-inherited-members

This bug will be fixed by this MR.

🛠 with ❤ at Siemens

/cc @bufferoverflow

Screenshots

Steps to reproduce (#249523 (closed)):

create a group parent

create a group sub in the parent namespace

in the group sub add a member with an access_level of 20 (Reporter)

in the group parent add same member with an access_level of 30 (Developer)

Note that 3. and 4. steps should not be swapped because this would show a validation error.

➡ Member should have the effective access level 30 (Developer) in group sub.

before MR	after MR

API

GET /api/v4/groups/:id_of_subgroup/members/all

Response: before MR

[
    ...
    {
        "id": 55,
        "name": "Altha Huel",
        "access_level": 20,
        ...
    }
]

Response: after MR

[
    ...
    {
        "id": 55,
        "name": "Altha Huel",
        "access_level": 30,
        ...
    }
]

Database Review

GroupMembersFinder.new(sub_group).execute:

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN ( WITH RECURSIVE "base_and_ancestors" AS (
(
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = 123)
                UNION (
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces",
                        "base_and_ancestors"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = "base_and_ancestors"."parent_id"))
                SELECT
                    "id"
                FROM
                    "base_and_ancestors" AS "namespaces")
                AND (members.access_level > 5)
            ORDER BY
                user_id,
                invite_email,
                access_level DESC,
                expires_at DESC,
                created_at ASC) members

GroupMembersFinder.new(sub_group).execute(include_relations: [:direct]):

Raw SQL

SELECT
    "members".*
FROM
    "members"
WHERE
    "members"."type" = 'GroupMember'
    AND "members"."source_id" = 123
    AND "members"."source_type" = 'Namespace'
    AND "members"."requested_at" IS NULL
    AND "members"."access_level" != 5

GroupMembersFinder.new(sub_group).execute(include_relations: [:direct, :inherited]):

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN ( WITH RECURSIVE "base_and_ancestors" AS (
(
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = 123)
                UNION (
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces",
                        "base_and_ancestors"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = "base_and_ancestors"."parent_id"))
                SELECT
                    "id"
                FROM
                    "base_and_ancestors" AS "namespaces")
                AND (members.access_level > 5)
            ORDER BY
                user_id,
                invite_email,
                access_level DESC,
                expires_at DESC,
                created_at ASC) members

GroupMembersFinder.new(sub_group).execute(include_relations: [:direct, :descendants]):

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN ( WITH RECURSIVE "base_and_descendants" AS (
(
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = 123)
                UNION (
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces",
                        "base_and_descendants"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."parent_id" = "base_and_descendants"."id"))
                SELECT
                    "id"
                FROM
                    "base_and_descendants" AS "namespaces")
                AND (members.access_level > 5)
            ORDER BY
                user_id,
                invite_email,
                access_level DESC,
                expires_at DESC,
                created_at ASC) members

GroupMembersFinder.new(sub_group).execute(include_relations: [:direct, :descendants, :inherited]):

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN ( WITH RECURSIVE "base_and_ancestors" AS (
(
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = 123)
                UNION (
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces",
                        "base_and_ancestors"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = "base_and_ancestors"."parent_id")),
                "base_and_descendants" AS (
(
                        SELECT
                            "namespaces".*
                        FROM
                            "namespaces"
                        WHERE
                            "namespaces"."type" = 'Group'
                            AND "namespaces"."id" = 123)
                    UNION (
                        SELECT
                            "namespaces".*
                        FROM
                            "namespaces",
                            "base_and_descendants"
                        WHERE
                            "namespaces"."type" = 'Group'
                            AND "namespaces"."parent_id" = "base_and_descendants"."id"))
                    SELECT
                        "namespaces"."id"
                    FROM ((
                            SELECT
                                "namespaces".*
                            FROM
                                "base_and_ancestors" AS "namespaces"
                            WHERE
                                "namespaces"."type" = 'Group')
                        UNION (
                            SELECT
                                "namespaces".*
                            FROM
                                "base_and_descendants" AS "namespaces"
                            WHERE
                                "namespaces"."type" = 'Group')) namespaces
                    WHERE
                        "namespaces"."type" = 'Group')
                    AND (members.access_level > 5)
                ORDER BY
                    user_id,
                    invite_email,
                    access_level DESC,
                    expires_at DESC,
                    created_at ASC) members

GroupMembersFinder.new(sub_group).execute(include_relations: [:inherited]):

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN ( WITH RECURSIVE "base_and_ancestors" AS (
(
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = 122)
                UNION (
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces",
                        "base_and_ancestors"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."id" = "base_and_ancestors"."parent_id"))
                SELECT
                    "id"
                FROM
                    "base_and_ancestors" AS "namespaces")
                AND (members.access_level > 5)
            ORDER BY
                user_id,
                invite_email,
                access_level DESC,
                expires_at DESC,
                created_at ASC) members

GroupMembersFinder.new(sub_group).execute(include_relations: [:descendants]):

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN ( WITH RECURSIVE "base_and_descendants" AS (
(
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."parent_id" = 123)
                UNION (
                    SELECT
                        "namespaces".*
                    FROM
                        "namespaces",
                        "base_and_descendants"
                    WHERE
                        "namespaces"."type" = 'Group'
                        AND "namespaces"."parent_id" = "base_and_descendants"."id"))
                SELECT
                    "id"
                FROM
                    "base_and_descendants" AS "namespaces")
                AND (members.access_level > 5)
            ORDER BY
                user_id,
                invite_email,
                access_level DESC,
                expires_at DESC,
                created_at ASC) members

GroupMembersFinder.new(sub_group).execute(include_relations: [:descendants, :inherited]):

Raw SQL

SELECT
    "members".*
FROM ( SELECT DISTINCT ON (user_id, invite_email)
        *
    FROM
        "members"
    WHERE
        "members"."type" = 'GroupMember'
        AND "members"."source_type" = 'Namespace'
        AND "members"."requested_at" IS NULL
        AND "members"."source_id" IN (
            SELECT
                "namespaces"."id"
            FROM (( WITH RECURSIVE "base_and_ancestors" AS (
(
                            SELECT
                                "namespaces".*
                            FROM
                                "namespaces"
                            WHERE
                                "namespaces"."type" = 'Group'
                                AND "namespaces"."id" = 122)
                        UNION (
                            SELECT
                                "namespaces".*
                            FROM
                                "namespaces",
                                "base_and_ancestors"
                            WHERE
                                "namespaces"."type" = 'Group'
                                AND "namespaces"."id" = "base_and_ancestors"."parent_id"))
                        SELECT
                            "namespaces".*
                        FROM
                            "base_and_ancestors" AS "namespaces")
                    UNION ( WITH RECURSIVE "base_and_descendants" AS (
(
                                SELECT
                                    "namespaces".*
                                FROM
                                    "namespaces"
                                WHERE
                                    "namespaces"."type" = 'Group'
                                    AND "namespaces"."parent_id" = 123)
                            UNION (
                                SELECT
                                    "namespaces".*
                                FROM
                                    "namespaces",
                                    "base_and_descendants"
                                WHERE
                                    "namespaces"."type" = 'Group'
                                    AND "namespaces"."parent_id" = "base_and_descendants"."id"))
                            SELECT
                                "namespaces".*
                            FROM
                                "base_and_descendants" AS "namespaces")) namespaces
                    WHERE
                        "namespaces"."type" = 'Group')
                    AND (members.access_level > 5)
                ORDER BY
                    user_id,
                    invite_email,
                    access_level DESC,
                    expires_at DESC,
                    created_at ASC) members

Does this MR meet the acceptance criteria?

Conformity

📋 Does this MR need a changelog?
- I have included a changelog entry.
Documentation (if required)
Code review guidelines
Merge request performance guidelines
Style guides
Database guides
[-] Separation of EE specific content

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
[-] Tested in all supported browsers
[-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

[-] Label as security and @ mention @gitlab-com/gl-security/appsec
[-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
[-] Security reports checked/validated by a reviewer from the AppSec team

Edited Mar 16, 2021 by Jonas Wälter

Fix effective access level of group members

What does this MR do?

Screenshots

API

Database Review

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Merge request reports