Use specialized service to refresh authorizations when a member is added/removed/updated in a project

What does this MR do?

The service used in this MR, ie, AuthorizedProjectUpdate::ProjectRecalculatePerUserService was added in !66725 (merged).

I am copying the same content from its MR description for clarity.

It would also be worth checking the implementation of the new service in !66725 (merged) for a better understanding of how everything fits together.

Current setup

Presently, this is how project authourization records of a user is refreshed whenever it is added/updated/removed as a member from an entity. This entity can either be a Project or a Group, so like

class Member < ApplicationRecord
  after_commit :refresh_member_authorized_projects

  def refresh_member_authorized_projects
    return if destroyed_by_association.present?

    UserProjectAccessChangedService.new(user_id).execute
  end
end

class GroupMember < Member
  # inherits the callback from Member
end

class ProjectMember < Member
  # inherits the callback from Member
end

Here, UserProjectAccessChangedService would run a AuthorizedProjectsWorker job inline, which would recalculate the user's authorizations across all projects in the system.

And this is where we have a scope for improvement, specifically in case of a ProjectMember.

Ideas

The case of a ProjectMember is very unique. The refresh_member_authorized_projects callbacks in case of a project_member runs when a user is specifically added to/updated/removed from a specific project.

So, in this case, there is no need to recalculate all of this user's authorizations to all projects in the system. If the user is added to an Engineering project, we need to just recalculate the permissions of Engineering.

We already have AuthorizedProjectUpdate::ProjectRecalculateService to refresh the permissions of a particular project. Using this, we could accomplish the following change today.

class Member < ApplicationRecord
  after_commit :refresh_member_authorized_projects

  def refresh_member_authorized_projects
    return if destroyed_by_association.present?

    UserProjectAccessChangedService.new(user_id).execute
  end
end

class GroupMember < Member
  # inherits the callback from Member
end

class ProjectMember < Member
  # overrides the definition from Member
  # as we only need to worry about refreshing all of the permissions of this specific project

  def refresh_member_authorized_projects
    return if destroyed_by_association.present?

    AuthorizedProjectUpdate::ProjectRecalculateService.new(self.project).execute
  end
end

Improving the idea

While the above change is a significant improvement over what we have now, there is still further scope for improvement.

This is because, in this callback, we are also dealing with a specific user that is being added to/updated/removed from this particular project. If we can combine these 2 requirements together, we can further scope down the number of rows returned from the database while this refresh is being made. This is a performance improvement.

Which is what ProjectRecalculatePerUserService is going to do: It only cares about a specific user's authorizations to a specific project and makes additions or deletions to this as necessary.

The performance improvement being: It does not read any row in project_authorizations table belonging to other users in the same project.

So this change would become

class Member < ApplicationRecord
  after_commit :refresh_member_authorized_projects

  def refresh_member_authorized_projects
    return if destroyed_by_association.present?

    UserProjectAccessChangedService.new(user_id).execute
  end
end

class GroupMember < Member
  # inherits the callback from Member
end

class ProjectMember < Member
  # overrides the definition from Member
  # as we only need to worry about refreshing all of the permissions of this specific project
  # for the specific user that was added/updated/removed in this project

  def refresh_member_authorized_projects
    return if destroyed_by_association.present?
    return unless user

    AuthorizedProjectUpdate::ProjectRecalculatePerUserService.new(self.project, self.user).execute
  end
end

So this is what this MR essentially does.

How will this change help?

• Running AuthorizedProjectsWorker inline has a heavy footprint in response times. As AuthorizedProjectsWorker refreshes authorizations across all projects in the systems, it executes more queries, crunches more data and is slow in general.

• This problem has come up recently in #334627 (comment 612428354) and using the approach in this MR can help there as the new service has smaller footprint compared to AuthorizedProjectsWorker because it has been explicitly designed for this specific use-case alone. It is not a generalized solution like AuthorizedProjectsWorker.

• It is true that even the new solution runs the refresh inline, along with the request. This could be later changed to a sidekiq background job. (I did not want to introduce multiple big changes together, which is why I kept the behaviour same as the existing behaviour - ie, refreshes from this callback always run inline, as part of the web request)

• If we look at this Kibana data, we can see that most of the top contributors towards inline AuthorizedProjectsWorker jobs are ProjectMember create/update/destroy actions. While using the new service in place of this won't reduce the number of these occurrences, it certainly would improve the response times of these web requests.

Comparing execution times:

Note: I am using the worker AuthorizedProjectUpdate::ProjectRecalculateWorker for comparison with AuthorizedProjectsWorker because it is the closest relative to AuthorizedProjectUpdate::ProjectRecalculatePerUserService. I would expect the performance of ProjectRecalculatePerUserService to be even better (at least slightly)

No changelog added as this change is behind a feature flag.

Screenshots or Screencasts (strongly suggested)

How to setup and validate locally (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

• I have included changelog trailers, or none are needed. (Does this MR need a changelog?)
• I have added/updated documentation, or it's not needed. (Is documentation required?)
• I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?)
• I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?)
• I have self-reviewed this MR per code review guidelines.
• This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines)
• I have followed the style guides.
• This change is backwards compatible across updates, or this does not apply.

Availability and Testing

• I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.)
• I have tested this MR in all supported browsers, or it's not needed.
• I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

• 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