Part 4: Refactor SSO enforcement for projects
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:
- !114111 (comment 1316056031)
- !114111 (comment 1317172042)
- !114111 (comment 1328907249)
- !114111 (comment 1333808828)
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.
-
I have evaluated the MR acceptance checklist for this MR.