Skip to content

Improve UX and simplify implementation of "Invite Members" modal

Bogdan Denkovych requested to merge improve-ux-and-simplify-implementation-of-invite-members-modal into master

What does this MR do and why?

UX

Before After(Search result) After(Invite)
Screenshot_from_2024-03-24_16-07-23 Screenshot_from_2024-03-24_17-21-17 Screenshot_from_2024-03-24_17-21-29

Video(Internal only, switch to GitLab Unfiltered account since the video is private): https://www.youtube.com/watch?v=x0_wWeFrr94. Sorry about the quality of the video, I still haven't figured out how to record video properly without lagging on Linux.

This MR proposes to reconsider UX for Invite Members UI that group/subgroup/project Maintainers use to add users to members.

By default Invite Members UI provides search that allows Maintainers search for all GitLab users to allow them to try to add them to members. However, it does not mean that any user could be added to members. When an Maintainer tries to add a user that cannot to be added for some reason, they will see an error message that should explain why the user they try to add cannot be added members. I think that provides superb UX, it guides user why something cannot be done, which improves Support Efficiency by reducing number of reports/questions from customers to The Support team.

If SSO enforcement is enabled for group, users without group's SAML or SCIM identity cannot be added to groups. When UX for Invite Members was considered for that case in https://gitlab.com/gitlab-org/gitlab/-/issues/333625, that modal wasn't completed and it din't show error messages, so it was source of confusion when trying to add user to members that is not supposed to be added, as per the following video. What we decided at that time is to limit the search on Invite Members UI to users with group's SAML or SCIM identity. Since the UI didn't show error message at that time, it made sense. However, it could be source of confusion when somebody sure there is a user account present in GitLab and they want to add them to members, but they can't, because they can't find that account because Invite Members search does not return that user and they don't understand why. That could lead to reports/questions to The Support. We can prevent that by allowing user to try to add any user they want, as it is when SSO enforcement is disabled for a group, and show error message that should explain the reason instead. It was confirmed in https://gitlab.com/gitlab-org/gitlab/-/issues/333625 that there were no security reason or any other strong reasons to limit the search, and it was just UX question. If there were security concerns, just limiting search or do any similar checks on the frontend side would not be enough, because (it should be done on the backend)

  • malicious users could mock the frontend code to try to invite any user via UI
  • users can use the API endpoint directly to try to add any user to members. Example:
curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" --data "user_id=77&access_level=30" "https://gdk.test:3443/api/v4/groups/95/invitations"
{"message":{"alice":"The member's email address is not linked to a SAML account or has an inactive SCIM identity. For information on how to resolve this error, see the \u003ca target=\"_blank\" rel=\"noopener noreferrer\" href=\"/help/user/group/saml_sso/troubleshooting_scim\"\u003etroubleshooting SCIM documentation\u003c/a\u003e."},"status":"error"}

This UX change proposal is aligned with the API's behavior, it explains why some users cannot be added to members.

To reassure us, we are safe here, the backend checks whether user can be be added to members and we have tests to cover that.

Not only could the existing UX be confusing and lead to reports and questions to The Support Team, but also it has caused lots of implementation concerns and negatively impacted maintainability and it could lead to bugs and breaking changes in the future. I explain all that in the next section. The UX changes this MR proposes completely eliminate all those issues and improves maintainability of the application.

The main question is Do you have any objections about this UX change? If you reviewed proposal and have no objections please leave comment below too, It will increase our confidence in this UX change.

If you have objections but not significant or are okay with both the existing UX and proposed change, please also read the Implementation section since it is also very significant. It may help to make trade-off between the existing UX and the proposed.

Implementation

This MR

When we limited search result on Invite Members to users with group's SAML or SCIM identity for groups with SSO enforcement is enabled, it caused https://gitlab.com/gitlab-org/gitlab/-/issues/424505. We need to unblock the solution to resolve that issue.

The policy which users can and cannot be added to members, as per the group settings, is subject to change along with developing GitLab application. The existing implementation is not designed for its extension. Any extension of the existing implementation we have done is a hack and could lead to bugs and even breaking change since we use API... Let me rationale below.

Since introducing Service Accounts, we changed policy to allow service account users to be added to members if SSO enforcement is enabled for the group. We allowed that on the backend side. However, to allow users to do so from UI, we had to change the search implementation. We needed to do this change quickly to allow customers that use SSO enforcement to use service accounts. But there was no any possible way to do this change correctly so we did a hack: We changed User.limit_to_saml_provider(saml_provider_id) method and /api/v4/users?saml_provider_id=ID API endpoint to return group's service accounts. This method name and API endpoint with saml_provider_id emphasize that it should return only users with the group's SAML identity, however it now returns the group's service accounts users which has no relation to SAML at all. We should change that since it could lead to bugs in the future.

Then we discovered https://gitlab.com/gitlab-org/gitlab/-/issues/424505. To keep the existing UX for Invite Members and to unblock the solution to resolve that issue we are about to add new API endpoints !134570 (merged) and !144796 (closed). At first glance they looked promising, but:

  • Since the policy which users can and cannot be added to members, as per the group settings, is subject to change along with developing GitLab application, those endpoints are subject to change too. It breaks promises that any endpoint changes under /api/v4/ should follow REST API deprecations and removals process if there is no security reason for that. I know we mentioned that those endpoints are for internal purposes in the docs, but It won't stop some users to start using it. And in case if any of this endpoint is changed in few month or years, we will break those customers workflows and it will lead to an incident because any change to an API endpoint is a breaking change.
  • Those endpoints looked promising and useful for users and for evolving, however after the following change !141040 (comment 1773138463), we made those endpoint available only when SSO enforcement is enabled. As a user, I find it very confusing that I cannot use neither GET /groups/:id/users nor GET /projects/:id/users endpoints for my groups that do not have SSO enforcement enabled. Those endpoints has no value for group that do not use SSO enforcement despite those endpoints names do not emphasize that they are supposed to work for SSO enforced groups only. I don't see any reason why those endpoints should only be allowed for use when SSO enforcement is enabled, at least we should change that.

As we can see, maintaining the existing UX for Invite Members('search'), takes lots of efforts, time, causes lots of issues, and increases the application complexity. With changing the policy which user could be added to members will only increase amount such issues.

If we agree on the UX proposal, it will eliminate all those issues and implementation complexity and significantly improve maintainability of the application. If we get strong reason why the existing UX should remain, here is my Plan B:

  1. Remove the existing implementation the same way this MR does to eliminates concerns I mentioned.
  2. Add internal endpoint, not in /api/v4/ namespace, but just as a simple Rails action/endpoint that is designed specifically for purposes Invite Members search to keep the existing UX. In that endpoint check whether and how search result should be limited. That endpoint should be accessible to those users that can admin the resource members. We will be able to change this endpoint anytime we need along with changes to the the policy which users can and cannot be added to members and not be concerned that it could cause breaking change, like it was done for /api/v4/users?saml_provider_id=ID API endpoint, or lead to ambiguous code and hacks like in User.limit_to_saml_provider(saml_provider_id) method.

But I hope that the UX change proposal will be accepted instead.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

  1. Create paid top-level group.
  2. Go to SAML Single Sign On Settings of the group and enable Enforce SSO-only authentication for web activity for this group.
  3. Go to Group members page, open Invite members modal.
  4. Search for a user that has no the group's SAML identity. Select that user and try to add them to group members.
  5. You should see the error message The member's email address is not linked to a SAML account or has an inactive SCIM identity., as shown on the screen in UX section: !147732
Edited by Bogdan Denkovych

Merge request reports