2018-06-25: 11.0.2 exception request for gitlab-org/gitlab-ee!6147

Exception request

  • Merge request to be considered for picking: gitlab-org/gitlab-ee!6147

Why it needs to be picked

We have a pending exception request in #275 (closed) for 11.0.2, which removed a cookie restriction from Group SAML. This would expose Group SAML to all users on GitLab.com. Group SAML is intended to be a Silver feature.

The MR in this request addresses a bug found during FA: we show the "SAML SSO" option in the Settings menu for all Groups, regardless of the GitLab.com plan. If the Group is on a Free or Bronze plan, clicking the link will present a 404 page.

What is the benefit?

Without this, many users who are interested in this feature (who are on Free and Bronze; the vast majority of users on GitLab.com) will hit 404s and feel like the feature is broken.

What is the urgency? What are alternatives?

We resolved concerns in #275 (closed), and if this is not merged with this issue, we'll expose many users to 404s who want to try this feature.

We could wait until 11.0.3, but these two should be picked together (or this should lead the cookie MR).

Potential negative impact of picking

If the license check was wrong the SAML SSO sidebar link might not show in the sidebar. This would be disappointing but not impact other areas. If it was wrong in the other direction it might have appeared for subgroups, which would be similar to the current problem of it appearing unnecessarily.

Explain what could go wrong with the release if the MR is picked: I can't think of anything other than the usual dangers of intermittent tests that could apply to any MR. I don't think that will be a problem here though.

Include a description of the worst case scenario in case something breaks: The SAML SSO link doesn't appear in the sidebar and we have to keep referring to it as beta.

Include an estimate of the potential number of users affected by any negative impact: If the sidebar link stops displaying this will only affect beta users because it was previously hidden behind a cookie. Since we mostly tested on a demo server previously this will be between 0 and 2 companies initially and any users who were planning to start using the feature. If it continues to display needlessly then any users clicking the new page to find out what it is will see a 404, but this is the problem this MR tries to solve and would probably still be fewer than at present

Explain the steps you took to minimize the risk: This change is covered by existing tests and has been manually verified.

Mitigation plan

If the SAML SSO sidebar link continues to appear unnecessarily and we for some reason we aren't happy to wait for a bug fix we can follow the mitigation steps from #275 (comment 83819758) and turn off Group SAML on GitLab.com from the omnibus settings. This is done by removing { "name" => "group_saml" } from gitlab_rails['omniauth_providers'] in a reverse of https://gitlab.com/gitlab-com/infrastructure/issues/4389

We can also revert the MR with:

diff --git a/ee/app/helpers/ee/saml_providers_helper.rb b/ee/app/helpers/ee/saml_providers_helper.rb
index e9b917946ff..8ca579398d5 100644
--- a/ee/app/helpers/ee/saml_providers_helper.rb
+++ b/ee/app/helpers/ee/saml_providers_helper.rb
@@ -1,15 +1,11 @@
 module EE
   module SamlProvidersHelper
-    def group_saml_configured?
+    def group_saml_enabled?
       ::Gitlab::Auth::GroupSaml::Config.enabled?
     end
 
     def show_saml_in_sidebar?(group)
-      return false unless group_saml_configured?
-      return false unless group.feature_available?(:group_saml)
-      return false if group.subgroup?
-
-      can?(current_user, :admin_group_saml, group)
+      group_saml_enabled? && !group.subgroup? && can?(current_user, :admin_group_saml, group)
     end
 
     def saml_link_for_provider(text, provider, *args)

Release manager sign-off

(Assign this issue to RMs managing this release. See https://about.gitlab.com/release-managers/)

The release manager needs to provide initial approval for this exception request.

  • Release manager

Mention others as necessary during discussion.

Sign-off for RC (delete this section if patch release)

Upon initial approval, the release manager will then ping and assign the following roles:

  • Engineering team leader
  • Engineering department leader (if different from team leader)
  • Quality leader

If you are the last person to provide initial approval, assign this issue to the VPE for his approval:

  • VPE

Sign-off for patch release (delete this section if RC)

Upon initial approval, the release manager will then ping and assign one more role for final approval:

  • Engineering team leader/Engineering department leader/Quality leader/VPE

After approval

Check that the following is accurate before closing this issue:

  • gitlab-org/gitlab-ee!6147 has the correct milestone and label set so that the release managers will pick it.
  • gitlab-org/gitlab-ee!6147 has a comment with a link to this issue:
    • Exception approved in ISSUE_LINK
Edited by James Edwards-Jones