Require group owner to have linked SAML before enabling Group Managed Accounts

What

Prevent owner from enabling Group Managed Accounts if they haven't first linked SAML to their account.

Why

Fixes #39183 (closed) and #38021 (closed)

Allows us to proceed with rolling out Group Managed Accounts

SSO Enforcement would otherwise force them to go through the SAML linking process to access the group, which with Group Managed Accounts would result in a new user being created. Since this new user would be created as a guest the original owner would have no further control over the group and would be locked out.

Why not

In a way this is a temporary fix until we can convert users into group managed accounts.

Another workaround might be to detect if the user is signed in as an owner on the old account and set the new account up as an owner. This might be more smooth, but is more complicated and would need careful thought from a security perspective to avoid the wrong user taking over as owner.

Screenshots

Acceptance criteria

Conformity

• Changelog entry
• Documentation (if required)

Availability and Testing

• Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
• Tested in all supported browsers

changed milestone to %12.6

added backend devopsmanage groupauthentication and authorization [DEPRECATED] saml typebug labels

changed the description

added 869 commits

• e273533b...f0e751e8 - 868 commits from branch master
• f8613795 - Enabling group-managed-accounts needs linked SAML

Compare with previous version

	1 Warning
	The title of this merge request is longer than 72 characters and would violate our commit message rules when using the Squash on Merge feature. Please consider adjusting the title, or rebase the commits manually and don’t use Squash on Merge.

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

Category	Reviewer	Maintainer
backend	Reuben Pereira (@rpereira2)	Sean McGivern (@smcgivern)
test Quality for spec/features/*	Désirée Chevalier (@dchevalier2)	No maintainer available
QA	Zeff Morgan (@zeffmorgan)	Dan Davison (@ddavison)

Generated by Danger

added 1 commit

• d75f05dc - Enabling group-managed-accounts needs linked SAML

Compare with previous version

@oswaldo Could you first-pass review?

assigned to @oswaldo

mentioned in issue #34653 (closed)

Thanks @jamedjo, couple minor suggestions and a question, thanks!

unassigned @oswaldo

changed milestone to %12.7

added missed:12.6 label

added 1 commit

• f1de3ef0 - Enabling group-managed-accounts needs linked SAML

Compare with previous version

added 1 commit

• c418a2cf - Enabling group-managed-accounts needs linked SAML

Compare with previous version

I've split the shared examples for CreateService/UpdateService up into two sections, allowing us to exclude the tests for enabling Group Managed Accounts form the CreateService. In the future we might move it back if this behaviour returns to allowing owners to enable without linking.

@engwan Could you maintainer review?

assigned to @engwan

resolved all threads

Thanks @jamedjo, this looks good to me. Just one optional suggestion.

Could you also rebase since this is already behind by 900 commits? Thanks

unassigned @engwan

added 1069 commits

• c418a2cf...249e94f7 - 1068 commits from branch master
• 65148e7b - Enabling group-managed-accounts needs linked SAML

Compare with previous version

added 1 commit

• 63453dde - Enabling group-managed-accounts needs linked SAML

Compare with previous version

resolved all threads

assigned to @engwan

Thanks @jamedjo, looks good to me

@sliaquat can you review the feature spec change here? Thanks

I can merge this after the test review

assigned to @sliaquat and unassigned @engwan

approved this merge request

Also, do you think we should add a note to https://docs.gitlab.com/ee/user/group/saml_sso/#group-managed-accounts mentioning this requirement?

I guess it's not really necessary as the user would realize this from the error message after trying to enable the feature.

unassigned @sliaquat

added 757 commits

• 63453dde...69278cd8 - 756 commits from branch master
• 00423ea5 - Enabling group-managed-accounts needs linked SAML

Compare with previous version

added 1 commit

• 6b3f3bea - Enabling group-managed-accounts needs linked SAML

Compare with previous version

@sliaquat I've updated the QA tests to sign root in with SAML before enabling Group Managed Accounts, and then to sign back out of the IdP; could you review?

assigned to @sliaquat

added 152 commits

• 6b3f3bea...7b131b67 - 151 commits from branch master
• 04ac754c - Enabling group-managed-accounts needs linked SAML

Compare with previous version

assigned to @dchevalier2

unassigned @sliaquat

approved this merge request

unassigned @dchevalier2

resolved all threads

Thanks @dchevalier2!

@jamedjo can you rebase one more time then I'll merge?

added 634 commits

• 04ac754c...df21f58e - 633 commits from branch master
• 2b9997d5 - Enabling group-managed-accounts needs linked SAML

Compare with previous version

enabled an automatic merge when the pipeline for 2b9997d5 succeeds

I set MWPS

The gitlab-qa downstream pipeline passed. .

merged

mentioned in commit 6268ee84

Hi @jamedjo and @engwan, I'm helping @sliaquat while he's out with his fix for #39607 (closed) and #55242 (closed), which touches the same spec changed in this MR. Since the spec is currently quarantined the ee:group_saml-quarantine job in package-and-qa job needs to be triggered manually to run this spec and confirm that it passes. Is it okay to trigger this job?

Just saw this is merged. I'll rebase @sliaquat's MR and run the job there

mentioned in merge request !22759 (closed)

mentioned in commit e8d88f9b

mentioned in issue #212150 (closed)

changed the description

added sectiondev label