Skip to content

Update enterprise badge, filtering, and disabling 2FA to new definition

Bogdan Denkovych requested to merge bdenkovych-updage-enterprise-users-badge-filtering-and-disabling-2fa into master

What does this MR do and why?

Related to #429637 (closed)

Currently, disabling two-factor authentication for enterprise users feature works based on the old enterprise user definition("Users created by SAML SSO or SCIM provisioning are enterprise users"). As a part of "Automatic claims of enterprise users" rollout, we introduced the new definition that is based on domain verification, see the documentation https://docs.gitlab.com/ee/user/enterprise_user/#automatic-claims-of-enterprise-users.

This MR makes disabling two-factor authentication for enterprise users feature work based on the new definition. The UI for the feature is tightly coupled with the Enterprise badge and filtering group members by Enterprise option as described in https://docs.gitlab.com/ee/user/enterprise_user/#disable-two-factor-authentication. We need to update the definition for those features simultaneously to prevent inconsistencies between the system behavior and UI.

While working on the implementation changes, I noticed a few bugs and issues that should be resolved(the MR resolves all them):

  1. Currently, group owners could filter members by Enterprise option, if SAML authentication for this group is enabled. While it could make sense with the old enterprise user definition, it is not correct for the new definition. The new definition is based on the group domain verification. There could be paid groups that do not use SAML at all but have their domain verified for automatic claims purposes. Group owners should be able to filter members by 'Enterprise' option if the domain verification feature is available for the group.
  2. Currently, the badge is shown and the Enterprise filtering work if an enterprise user is a member of top-level group. I see that currently, it is possible to filter by Enterprise option on the subgroup level, too. It could lead to confusion for group owners since some enterprise users might be direct members of the subgroups they are filtering in but not members of the top-level group. I suggest allowing filter members by Enteprrise option on top-level groups only. It will make clear that currently filtering members by Enterprise requires them to be members of the top-level group. We plan to build a separate page that will show a group's enterprise users regardless of whether they are members.
  3. Currently, group owners could disable 2FA for their enterprise users even after the group's subscription is canceled or expired. It is a bug since the docs say:

Enterprise users have user accounts that are administered by an organization that has purchased a GitLab subscription.

If a group’s purchased subscription expires or is canceled, the group is not able to manage their enterprise users.

Disabling two-factor authentication for enterprise users should be only allowed for a group if the group's current subscription is GitLab Premium+ (supports domain_verification feature).

Related references

Initial MR that implements the enterprise badge: !63474 (merged).

Initial MR that implements the enterprise option to filter group members: !81804 (merged).

Initial MRs that implement disabling 2FA for enterprise users: !99129 (merged), !107401 (merged), !106837 (merged).

Screenshots or screen recordings

https://www.youtube.com/watch?v=0L8t_vxTqX0

How to set up and validate locally

  1. Make sure the GitLab instance simulates or a SaaS instance since Enterprise Users is a SaaS feature
  2. Create a user and set up 2FA for the user.
  3. Set up a group and configure "Automatic claims of enterprise users". Enable enterprise_users_automatic_claim FF, since this is currently under a FF in the codebase. Guide: https://docs.gitlab.com/ee/administration/feature_flags.html#enable-or-disable-the-feature. For testing purposes on the local environment you can claim the user manually from the Rails console:
User.find_by_username('USERNAME').user_detail.update(enterprise_group_id: GROUP_ID)
  1. Disable two-factor authentication for the user by your group owner account, see https://docs.gitlab.com/ee/user/enterprise_user/#disable-two-factor-authentication
  2. Confirm the 2FA is reset for the user.

DB

Migration

bin/rails db:migrate 
bogdanvlviv@lenovo:~/gitlab-development-kit/gitlab$ bin/rails db:migrate RAILS_ENV=test
main: == [advisory_lock_connection] object_id: 174520, pg_backend_pid: 62775
main: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: migrating =====
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0144s
main: -- index_exists?(:user_details, [:enterprise_group_id, :user_id], {:name=>"index_user_details_on_enterprise_group_id_and_user_id", :algorithm=>:concurrently})
main:    -> 0.0046s
main: -- Index not created because it already exists (this may be due to an aborted migration or similar): table_name: user_details, column_name: [:enterprise_group_id, :user_id]
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0007s
main: -- indexes(:user_details)
main:    -> 0.0031s
main: -- execute("SET statement_timeout TO 0")
main:    -> 0.0005s
main: -- remove_index(:user_details, {:algorithm=>:concurrently, :name=>"index_user_details_on_enterprise_group_id"})
main:    -> 0.0015s
main: -- execute("RESET statement_timeout")
main:    -> 0.0005s
main: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: migrated (0.0437s)

main: == [advisory_lock_connection] object_id: 174520, pg_backend_pid: 62775
ci: == [advisory_lock_connection] object_id: 174720, pg_backend_pid: 62777
ci: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: migrating =====
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0007s
ci: -- index_exists?(:user_details, [:enterprise_group_id, :user_id], {:name=>"index_user_details_on_enterprise_group_id_and_user_id", :algorithm=>:concurrently})
ci:    -> 0.0047s
ci: -- Index not created because it already exists (this may be due to an aborted migration or similar): table_name: user_details, column_name: [:enterprise_group_id, :user_id]
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0006s
ci: -- indexes(:user_details)
ci:    -> 0.0032s
ci: -- execute("SET statement_timeout TO 0")
ci:    -> 0.0006s
ci: -- remove_index(:user_details, {:algorithm=>:concurrently, :name=>"index_user_details_on_enterprise_group_id"})
ci:    -> 0.0014s
ci: -- execute("RESET statement_timeout")
ci:    -> 0.0005s
ci: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: migrated (0.0301s)

ci: == [advisory_lock_connection] object_id: 174720, pg_backend_pid: 62777
bin/rails db:rollback 
bogdanvlviv@lenovo:~/gitlab-development-kit/gitlab$ bin/rails db:rollback:main RAILS_ENV=test
main: == [advisory_lock_connection] object_id: 174180, pg_backend_pid: 63236
main: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: reverting =====
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0147s
main: -- indexes(:user_details)
main:    -> 0.0046s
main: -- execute("SET statement_timeout TO 0")
main:    -> 0.0005s
main: -- remove_index(:user_details, {:algorithm=>:concurrently, :name=>"index_user_details_on_enterprise_group_id_and_user_id"})
main:    -> 0.0107s
main: -- execute("RESET statement_timeout")
main:    -> 0.0006s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- view_exists?(:postgres_partitions)
main:    -> 0.0006s
main: -- index_exists?(:user_details, :enterprise_group_id, {:name=>"index_user_details_on_enterprise_group_id", :algorithm=>:concurrently})
main:    -> 0.0030s
main: -- add_index(:user_details, :enterprise_group_id, {:name=>"index_user_details_on_enterprise_group_id", :algorithm=>:concurrently})
main:    -> 0.0012s
main: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: reverted (0.0556s)

main: == [advisory_lock_connection] object_id: 174180, pg_backend_pid: 63236
bogdanvlviv@lenovo:~/gitlab-development-kit/gitlab$ bin/rails db:rollback:ci RAILS_ENV=test
ci: == [advisory_lock_connection] object_id: 174020, pg_backend_pid: 63574
ci: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: reverting =====
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0145s
ci: -- indexes(:user_details)
ci:    -> 0.0042s
ci: -- execute("SET statement_timeout TO 0")
ci:    -> 0.0006s
ci: -- remove_index(:user_details, {:algorithm=>:concurrently, :name=>"index_user_details_on_enterprise_group_id_and_user_id"})
ci:    -> 0.0015s
ci: -- execute("RESET statement_timeout")
ci:    -> 0.0004s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- view_exists?(:postgres_partitions)
ci:    -> 0.0005s
ci: -- index_exists?(:user_details, :enterprise_group_id, {:name=>"index_user_details_on_enterprise_group_id", :algorithm=>:concurrently})
ci:    -> 0.0031s
ci: -- add_index(:user_details, :enterprise_group_id, {:name=>"index_user_details_on_enterprise_group_id", :algorithm=>:concurrently})
ci:    -> 0.0016s
ci: == 20231030205756 IndexUserDetailsOnEnterpriseGroupIdAndUserId: reverted (0.0533s)

ci: == [advisory_lock_connection] object_id: 174020, pg_backend_pid: 63574

Query Plans

ee/app/finders/ee/group_members_finder.rb change

Query plan before this MR: 
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 ((SELECT "namespaces"."id", "namespaces"."name", "namespaces"."path", "namespaces"."owner_id", "namespaces"."created_at", "namespaces"."updated_at", "namespaces"."type", "namespaces"."description", "namespaces"."avatar", "namespaces"."membership_lock", "namespaces"."share_with_group_lock", "namespaces"."visibility_level", "namespaces"."request_access_enabled", "namespaces"."ldap_sync_status", "namespaces"."ldap_sync_error", "namespaces"."ldap_sync_last_update_at", "namespaces"."ldap_sync_last_successful_update_at", "namespaces"."ldap_sync_last_sync_at", "namespaces"."description_html", "namespaces"."lfs_enabled", "namespaces"."parent_id", "namespaces"."shared_runners_minutes_limit", "namespaces"."repository_size_limit", "namespaces"."require_two_factor_authentication", "namespaces"."two_factor_grace_period", "namespaces"."cached_markdown_version", "namespaces"."project_creation_level", "namespaces"."runners_token", "namespaces"."file_template_project_id", "namespaces"."saml_discovery_token", "namespaces"."runners_token_encrypted", "namespaces"."custom_project_templates_group_id", "namespaces"."auto_devops_enabled", "namespaces"."extra_shared_runners_minutes_limit", "namespaces"."last_ci_minutes_notification_at", "namespaces"."last_ci_minutes_usage_notification_level", "namespaces"."subgroup_creation_level", "namespaces"."emails_disabled", "namespaces"."max_pages_size", "namespaces"."max_artifacts_size", "namespaces"."mentions_disabled", "namespaces"."default_branch_protection", "namespaces"."max_personal_access_token_lifetime", "namespaces"."push_rule_id", "namespaces"."shared_runners_enabled", "namespaces"."allow_descendants_override_disabled_shared_runners", "namespaces"."traversal_ids", "namespaces"."organization_id" FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 4249178)) 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 WHERE "members"."type" = 'GroupMember' AND (EXISTS (SELECT 1 FROM "user_details" WHERE "user_details"."provisioned_by_group_id" = "members"."source_id" AND "user_details"."user_id" = "members"."user_id"));

https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/23507/commands/75524

Query plan after this MR: 
DROP INDEX index_user_details_on_enterprise_group_id;
CREATE INDEX index_user_details_on_enterprise_group_id_and_user_id ON user_details USING btree (enterprise_group_id, user_id);
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 ((SELECT "namespaces"."id", "namespaces"."name", "namespaces"."path", "namespaces"."owner_id", "namespaces"."created_at", "namespaces"."updated_at", "namespaces"."type", "namespaces"."description", "namespaces"."avatar", "namespaces"."membership_lock", "namespaces"."share_with_group_lock", "namespaces"."visibility_level", "namespaces"."request_access_enabled", "namespaces"."ldap_sync_status", "namespaces"."ldap_sync_error", "namespaces"."ldap_sync_last_update_at", "namespaces"."ldap_sync_last_successful_update_at", "namespaces"."ldap_sync_last_sync_at", "namespaces"."description_html", "namespaces"."lfs_enabled", "namespaces"."parent_id", "namespaces"."shared_runners_minutes_limit", "namespaces"."repository_size_limit", "namespaces"."require_two_factor_authentication", "namespaces"."two_factor_grace_period", "namespaces"."cached_markdown_version", "namespaces"."project_creation_level", "namespaces"."runners_token", "namespaces"."file_template_project_id", "namespaces"."saml_discovery_token", "namespaces"."runners_token_encrypted", "namespaces"."custom_project_templates_group_id", "namespaces"."auto_devops_enabled", "namespaces"."extra_shared_runners_minutes_limit", "namespaces"."last_ci_minutes_notification_at", "namespaces"."last_ci_minutes_usage_notification_level", "namespaces"."subgroup_creation_level", "namespaces"."emails_disabled", "namespaces"."max_pages_size", "namespaces"."max_artifacts_size", "namespaces"."mentions_disabled", "namespaces"."default_branch_protection", "namespaces"."max_personal_access_token_lifetime", "namespaces"."push_rule_id", "namespaces"."shared_runners_enabled", "namespaces"."allow_descendants_override_disabled_shared_runners", "namespaces"."traversal_ids", "namespaces"."organization_id" FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 4249178)) 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 WHERE "members"."type" = 'GroupMember' AND (EXISTS (SELECT 1 FROM "user_details" WHERE "user_details"."enterprise_group_id" = "members"."source_id" AND "user_details"."user_id" = "members"."user_id"));

https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/23507/commands/75537

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Bogdan Denkovych

Merge request reports