Skip to content

Part 4: Refactor SSO enforcement for projects

Bogdan Denkovych requested to merge bdenkovych-issue-405021-part-4 into master

What does this MR do and why?

Related to #405021 (closed), !102104 (merged), #378928 (closed)

In !117389 (merged) and !117582 (merged) we added hundreds of specs related to SSO enforcement to change the related code confidently.

This MR refactors SSO enforcement policy definition for projects from

with_scope :subject
condition(:needs_new_sso_session) do
  ::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject.group, user: @user, for_project: true)
end

# For public projects, SSO enforcement only applies to group members
rule { public_project & needs_new_sso_session & group_member }.policy do
  prevent :public_user_access
  prevent :public_access
end

rule { needs_new_sso_session }.policy do
  prevent :guest_access
  prevent :reporter_access
  prevent :developer_access
  prevent :maintainer_access
  prevent :owner_access
end

to

with_scope :subject
condition(:needs_new_sso_session) do
  ::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject.group, user: @user, for_project: true) && (subject.private? || group_member?)
end

rule { needs_new_sso_session }.policy do
  prevent :read_project
end

This should make SSO enforcement policy definition for projects to be more maintainable and understandable. Looking at SSO enforcement table we can say that:

The SSO enforcement check should only be applied to [private resources or group members]. To rephrase this in passive form, we can say that the SSO enforcement check is not applied to [non-members and public resources].

The refactored code reflects this - it reads very close to the table since you can see the similarity between the code and the table.

This also removes the need to use public_user_access and public_access for SSO enforcement. IMO, their use should be deleted from the codebase; they are confusing and look like a workaround.

Another advantage of this refactoring is that it will make SSO enforcement policy definition for projects look the same as for SSO enforcement policy definition for groups, related to #386920 (closed). As you can see from the previous attempt to amend SSO enforcement for group in !114111 (merged), following the existing policy definition has led to confusion and lots of tech issues:

Following this refactoring for SSO enforcement policy definition for groups, it will eliminate all these issues. See !118596 (merged)

Screenshots or screen recordings

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

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 Bogdan Denkovych

Merge request reports