Skip to content

Convert users.managing_group_id to loose foreign key

Thong Kuah requested to merge no_user_fk_namespaces into master

What does this MR do and why?

Related issue: #409837 (closed)

  1. What happens currently ?

When a group is deleted, a foreign key cascade nullifies managing_group_id on users table

  1. What happens now ?

When a group is deleted, we queue in loose_foreign_keys a job to eventually nullify managing_group_id on users table

  1. Will things be OK while we are in the queue ?

Based on code present in ee/app/model/ee/user.rb, it should be OK.

  • user.managing_group will return nil but all calls are guarded by user.group_managed_account?. For example, see ee/app/views/profiles/notifications/_email_settings.html.haml.
    • One place needs to be updated in ee/app/workers/personal_access_tokens/instance/policy_worker.rb to handle this use case
  • For group, well, it's deleted :)

Application code where managing_group is used:

ee/app/helpers/ee/personal_access_tokens_helper.rb:      current_user.managing_group.max_personal_access_token_lifetime_from_now
ee/app/models/ee/group.rb:      has_many :managed_users, class_name: 'User', foreign_key: 'managing_group_id', inverse_of: :managing_group
ee/app/models/ee/identity.rb:      validate :validate_managing_group
ee/app/models/ee/identity.rb:    def validate_managing_group
ee/app/models/ee/identity.rb:      errors.add(:base, _('Group requires separate account')) if saml_provider.group != user.managing_group
ee/app/models/ee/personal_access_token.rb:      user.managing_group.max_personal_access_token_lifetime_from_now
ee/app/models/ee/user.rb:      belongs_to :managing_group, class_name: 'Group', optional: true, inverse_of: :managed_users
ee/app/models/ee/user.rb:        scope = where(managing_group_id: nil)
ee/app/models/ee/user.rb:        scope = scope.or(where.not(managing_group_id: group.id)) if group
ee/app/models/ee/user.rb:      scope :managed_by, ->(group) { where(managing_group: group) }
ee/app/models/ee/user.rb:      managing_group.present?
ee/app/models/ee/user.rb:      self.group_managed_account? && self.managing_group.owned_by?(user)
ee/app/services/ee/personal_access_tokens/revoke_service.rb:          token.user&.managing_group == group &&
ee/app/services/group_saml/group_managed_accounts/transfer_membership_service.rb:        current_user.managing_group = group
ee/app/services/group_saml/sign_up_service.rb:        new_user.managing_group = group if group.saml_provider&.enforced_group_managed_accounts?
ee/app/views/profiles/notifications/_email_settings.html.haml:- help_text = group_managed_account ? s_("Your account uses dedicated credentials for the \"%{group_name}\" group and can only be updated through SSO.").html_safe % { group_name: @user.managing_group.name } : nil
ee/lib/gitlab/auth/group_saml/gma_membership_enforcer.rb:          root_ancestor == user.managing_group

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

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 Thong Kuah

Merge request reports