User mapping - API Development (Controllers)

changed milestone to %Backlog

added Category:Importers Importer:Direct Transfer backend devopsmanage documentation groupimport and integrate ready for next refinement sectiondev typefeature workflowrefinement labels

removed ready for next refinement label

added to epic &12378 (closed)

mentioned in epic &12378 (closed)

changed the description

marked this issue as blocked by #443556 (closed)

changed milestone to %16.11

set weight to 3

added ready for next refinement workflowblocked labels and removed workflowrefinement label

mentioned in issue gitlab-org/manage/general-discussion#17668

changed milestone to %17.0

mentioned in issue #454829 (closed)

changed the description

mentioned in merge request !147488 (merged)

mentioned in issue gitlab-org/manage/general-discussion#17672

I have a minor nitpick about the statuses: I thought pending and pending_approval were the same thing at first. Maybe one of the "pending"s should be called something else to avoid confusion? Perhaps pending_approval could be awaiting_approval?

Agreed! I find the asymmetry a little confusing, too. If we call one awaiting_approval, seeing pending on its own makes me think "pending what?"

Maybe awaiting_approval and awaiting_assignment?

removed the relation with #443556 (closed)

changed the description

marked this issue as blocking #455912 (closed)

mentioned in issue #455912 (closed)

marked this issue as blocking #455902 (closed)

changed the description

When using GET accept, the Import::SourceUser status should be changed to complete. In this case, the reassign worker has already been implemented. The action should enqueue the worker.

@wortschi @m_frankiewicz I think this might be the only reason this issue is blocked. If so, I think it's worth just leaving a comment in the code that shows where we'd start the reassignment process, and set the status to complete manually for now. At worst, we'd need a follow-up MR to fix that one endpoint once the reassign worker is implemented so that more can be worked on simultaneously

Thank you @SamWord sounds good! Please add whatever is relevant to the issue description and update the labels.

added milestonep1 label

added Deliverable label

changed the description

Setting health status to on track as the milestone has just begun.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to on track

changed title from User mapping - API Development to User mapping - API Development (Controllers)

@m_frankiewicz @wortschi Initially, we're planning to allow group owners to reassign users via the User reassignment UI, but are there any requirements on what endpoints we need to make so that this can be done via the API? Should all of the controller actions listed above be available as API endpoints as well?

@SamWord If I understand your question correctly, you are asking if there's a need to enable users to make reassignments via the API rather then the UI?

Regardless of whether we allow this workflow to be done via the API or not, I believe all the following controller actions should be implemented as API endpoints anyway for the UI to consume?

GET index (to list all import source users)

POST reassign (to assign the user that the contributions will go to)

DELETE reassign (to remove the user that the contributions were assigned to)

@SamWord we talked about it during design session, if I remember well, there are recording of those meetings. Rodrigo said I'd possible to have in API the bulk reassignment action, the one that in UI will be done with CSV upload.

I have a question to

DELETE reassign (to remove the user that the contributions were assigned to)

does that mean cancelling re-assignment? When I read this:

When using DELETE reassign, the Import::SourceUser status should be changed to pending_assignment, and the reassign_to_user should be unset.

it seems like it's about cancelling reassignment, that should be possible before user accepts or rejects it.

@justin_ho

I noticed that we're using a mix of both the internal and the public API for bulk import features. For example, the history and the details page are using the public API, whereas the status information is being retrieved from the internal API. What's your preference here - should we implement the following controller actions under the public API?

GET index (to list all import source users)

POST reassign (to assign the user that the contributions will go to)

DELETE reassign (to remove the user that the contributions were assigned to)

/cc @georgekoltsov in case you have any thoughts/suggestions

I might need a bit more research on this to form an opinion. Currently, the group members page uses data from a helper at doesn't even use any APIs to fetch data / for pagination. For example, when you click on page 2 on the gitlab-org members page, the whole page refreshes and gets its data from the helper.

I am not sure if we will follow this convention or use something custom. At the very least, we will need to update the helper to include some pagination data, as it's used to show the tab and the users count.

@georgekoltsov mentioned that this API and controller implementations are usually separate, so I'll make a separate issue for API endpoints then for another milestone. This issue should be ready for development then, but please reach out if there's any questions!

The other issue for API endpoints is here: #457066 (closed)

GitLab often has our Vue.js frontend make GraphQL calls, in which case the backend implementation happens once in the GraphQL API. GraphQL is well suited for frontend needs. I wonder if we could implement it once in GraphQL, rather than twice in a controller and REST API.

I think in Import and Integrate because much of our product is from the pre-GraphQL era, the frontend tends not to be written to call GraphQL. And since GraphQL has no real support for our product we also select the REST API over GraphQL when adding endpoints for consistency, as the existing endpoints for the particular features are also in REST. Generally the frontend doesn't call our REST API but is supplied data through controllers and our own serializers. We don't tend to call our controllers a "private/internal API" because that can confuse it with our real internal REST API endpoints (most are here), so we tend to just call those endpoints just "controllers".

@justin_ho Correct me if I'm wrong here!

The question is whether this is greenfield enough to be implemented in GraphQL in the backend without introducing an inconsistency with other import-related endpoints being in the REST API? And then also how ready our frontend is to call GraphQL instead of the more traditional (for our domain) ways of being passed its data from a controller.

I think this feature isn't tied to any particular importer, but stands as its own adjacent-feature so it could be done in GraphQL and not REST without it feeling like we're being inconsistent.

Another thing to weigh is that generally, GitLab prefers we implement API support in GraphQL where possible, rather than REST, if we pick just one. (There's been a long history of this discussion, but recently #446241 (comment 1806802463) ).

There are pragmatics of our domain's frontend readiness/support for GraphQL that I don't understand, which are also a factor in where the backend should implement support.

But, interested to know others' thoughts.

@.luke Thank you for raising this! My perception of this flow is that we anticipate users will primarily interact with it via GitLab's UI, even if public API endpoints are technically available. Is that correct?

If so, and it's the case that (essentially re-phrasing to make sure I understood correctly):

These endpoints share no significant existing with other endpoints in the domain.
Implementing them as GraphQL endpoints is more consistent with the preferred way of fetching data for Vue.
Implementing as GraphQL endpoints potentially avoids duplication between controllers and API endpoints.

On balance, it sounds like GraphQL would be the way to go.

Ok so before I go into the discussion on GraphQL, I should mention a few things:

The Group members page (on which we want to render the new UI) uses a quite complex app that is used for a few other member types (invited users, banner users, etc). With the current setup, it's likely that controller + helper changes are non-negotiable as we need at least the pagination info, which is used by the frontend to determine whether to render our "Placeholders" tab or not (it only renders if there are more than 0 items). I found this recent backend MR that does what we plan to add but for another member type: !146678 (diffs).
Once we have the tab rendered with the above conditions, we can fetch more data (for example, paginated results) pretty much any way we want as it's a fully custom app.

Generally the frontend doesn't call our REST API but is supplied data through controllers and our own serializers.

This is kinda true for simple apps but of course there are a bunch of frontend apps that call either GraphQL / REST API. For this particular page, we use data provided directly from helpers.

Back to the GraphQL / REST API discussion, I think other than our frontend we also need to consider the needs of our customers. I often see requests to add (REST) API endpoints after we rollout a feature, even if there is already UI that works just fine.

Overall, the simplest solution would be to fully fetch data from controllers + helpers (including pagination and filtering; similar to this code) and then the frontend won't even need to use any of GraphQL / REST API. Otherwise if we want to handle pagination and filtering separately, I don't mind using GraphQL if that's the preference.

Thank you @justin_ho! So there's a hard requirement to support some paginated data to the frontend via a helper to render the tab, and then in terms of data served within the new Vue.js app when the tab is clicked we can use whatever source we decide?

There will be some other backend interactions besides just fetching data. (I'm not sure all of them because I'm unable to see the Figma design mentioned in &12378 (closed) and the user flow design in the epic doesn't go into detail - @m_frankiewicz do we know more detail of what exactly the user will see and do when they're mapping users?). Another mutation would at least be needed to persist the mapping. Perhaps we need to allow people to search for users to map to, and could use existing endpoint?

Another angle is backend developer productivity: For backend engineers, on the one hand, a GraphQL implementation may involve some learning. On the other, implementing it once in the backend instead of twice pays off in terms of time saved. So perhaps the backend experience would be about even. There's also a potential benefit to backend engineers to learn new skills.

It sounds like in terms of frontend productivity, it might be easier for the endpoints to be in controllers. @justin_ho do you think a frontend implementation with GraphQL would take away from your productivity?

Lots of things to balance.

Ah yes one of the things I wanted to mention but totally forgot. So there are a few actions in the user mapping flow that will not be backed by any UI. Namely the Approve / Reject contributions by individual users (the mapped user) which they click from within an email. This will likely link to some controller that stores that action, redirects them and shows a flash message with the result (see this design #451028[Email_-_Destination_User_Re-Assignment.png]).

do you think a frontend implementation with GraphQL would take away from your productivity?

If the question is about comparing REST API and GraphQL, I would (personally) say it's about the same for me. It's just a different way of fetching data and I've worked with both in different features, so I'm good with either.

It sounds like in terms of frontend productivity, it might be easier for the endpoints to be in controllers.

Yes but mostly because this is how the other member types on the same page do it and it's always easier to copy that approach. Also to make sure we are on the same page, what I am referring to is adding code to existing controllers (the same ones that render the page) and passing data via helpers, not adding new API endpoints.

@.luke you can find designs in User Mapping - UX (#451028 - closed)

@gitlab-org/manage/import-and-integrate/dev How does this sound?

A controller for:

Handling the approve/reject actions by individual users (the mapped user) that they click from within an email.

GraphQL for other backend work to support the UI to map users:

Return paginated results of Import::SourceUser records, filterable by its status (design). Possibly filterable by search on placeholder username (design) which might be a fun SQL query.
Mutation to reassign the Import::SourceUser record.
Have we missed any?

Frontend will still need some data supplied through helpers to initialize the tabs #443555 (comment 1875250638).

@.luke Sounds good!

Have we missed any?

I'm not sure if cancelling an assignment counts as a separate mutation, but if so, that.

@.luke @SamWord If we have the accept/reject endpoints on GET actions, does that open us up to CSRF? A malicious actor could create a contribution for a victim user and then send them an email with <img src="https://gitlab.com/assignments/12/accept"> or similar. Do we have any protections in place against that?

Would it be safer to have the link in the email direct the user to a show action from which they can POST/DELETE to accept or reject?

@jnutt Good idea! We could present accept and decline buttons. We use a similar kind of 2-step flow when a user uses a Slack Slash Command for the first time. What do you think @m_frankiewicz?

Leads to

@justin_ho @jnutt Thinking about the GraphQL design for this, we could add a field to Group and do something like (not all arguments would need to be supported in the first iteration):

{
  group(fullPath: "my-group") {
    # `importUsers` field returns a collection of `Import::SourceUser` records
    # `assignmentStatus` argument (optional), an array of reassignment statuses
    #    (one of https://gitlab.com/gitlab-org/gitlab/-/issues/451028/designs/Reassignment_Statuses.png)
    #    https://gitlab.com/gitlab-org/gitlab/-/issues/451028/designs/User_Mapping_-_Keep_as_Placeholder.png#note_1877820974
    # `search` argument (optional), text to filter associated user records by 
    #    https://gitlab.com/gitlab-org/gitlab/-/issues/451028/designs/User_Mapping_-_Keep_as_Placeholder.png#note_1877821427
    importUsers(reassignmentStatus: [], search: "") { 
      nodes {
        sourceUserName
        sourceName
        assignmentStatus # Enum, one of https://gitlab.com/gitlab-org/gitlab/-/issues/451028/designs/Reassignment_Statuses.png
        assignedUser { # Types::User, the associated User record
          id
          name
        }
      }
    }
  }
}

We could have fine-grained mutations to set assignment, and unset assignment, and also set assignment status to "keep as placeholder" (see design). Our docs suggest not to use fine-grained mutations though. I don't have strong feelings about which way we take because both have benefits and downsides. It feels like the non-fine-grained would be harder for a member of the public to work with, as some combinations of argument values are invalid, whereas the fine-grained we'd be holding their hand better.

Non-fine grained:

mutation {
  # `id` required, all others optional
  # when `assignedUserId` provided and `null` it would be unset
  # if `keepAsPlaceholder` was provided as `true`, and also `assignedUserId` was present, this would be an error
  importUserUpdate(input: { id: "<Global ID of Import::SourceUser>", assignedUserId: "<Global ID of User>", keepAsPlaceholder: "<Boolean>" }) {
    errors
    importUser { ... ImportUser type }
  }
}

Fine-grained:

mutation {
  # `id` and `assignedUserId` required
  importUserAssign(input: { id: "<Global ID of Import::SourceUser>", assignedUserId: "<Global ID of User>" }) {
    errors
    importUser { ... ImportUser type }
    }
  }
}

mutation {
  # `id` required
  importUserUnassign(input: { id: "<Global ID of Import::SourceUser>" }) {
    errors
    importUser { ... ImportUser type }
  }
}

mutation {
  # `id` required
  importUserSetAsPlaceholder(input: { id: "<Global ID of Import::SourceUser>" }) {
    errors
    importUser { ... ImportUser type }
  }
}

@jnutt @.luke that change would affect user flow, @emilybauman for your attention.

I'm not sure if it's a valid example though

A malicious actor could create a contribution for a victim user

Yes, they could create a contribution on their SM instance, but not on an actual SM instance the victim user would contribute on and expect migration from. Also, how would they intercept/change the email that is sent during contribution mapping? Or you mean attacker would be sending random emails to people with fake buttons making them look like the legitimate emails sent during contribution reassignment?

@jnutt Good idea! We could present accept and decline buttons. We use a similar kind of 2-step flow when a user uses a Slack Slash Command for the first time. What do you think @m_frankiewicz?

@m_frankiewicz @.luke - I actually like having this intermediary step to confirm the reassignment. I don't think it'll be that hard to adjust from a design POV.

@m_frankiewicz I was thinking along the lines of creating a page with an image tag or similar that caused the accept URL to be loaded with the victim session with no further interaction. It wouldn't require a malicious person to intercept the email, but thinking about it, it would require them to guess the invite ID.

I'm not sure it's a concern in this case. Looking at the way invites are handled, it looks like the invite prompt is skipped so long as the invite is valid. @.luke, am I just being paranoid?

def skip_invitation_prompt?
  !member? && current_user_matches_invite?
end

@emilybauman supports the idea @jnutt, so let's do it

@jnutt @m_frankiewicz - We could show something like this:

Thank you Emily! And so we would have link in the email that leads to the above message, right?

Approve reassignment of contributions from source username on source hostname to you within destination top-level group name

In "Contribution details" we have a mismatch: Source group: {source hostname}

@emilybauman In the above design, should that first entry be the hostname, or the group being imported? I don't think we track the source group.

@SamWord Is the "reassigned by" information available anywhere? Is it always the owner of the destination namespace, or are others able to initiate this step?

@m_frankiewicz @jnutt - Thanks for the info. I just had copied over this content from the email, so perhaps we should have another look at #451028[Email_-_Destination_User_Re-Assignment.png] to ensure we can show all the data proposed. I also adjusted the content a little of the placeholder links so it's easier to understand.

@jnutt We don't have a reassigned_by on Import::SourceUser, so no we don't have that information anywhere. We'll need to add that column.

I think it would be a relatively easy to add as a part of the reassignment process though: when a user assigns a real user to the placeholder user, we can just log who did that assignment at the same time we update reassign_to_user_id on the Import::SourceUser.

I've added the column in this MR: !152199 (merged).

changed title from User mapping - API Development (Controllers) to User mapping - API Development

changed title from User mapping - API Development to User mapping - API Development (Controllers)

changed the description

removed ready for next refinement label

added workflowready for development label and removed workflowblocked label

mentioned in issue #457066 (closed)

mentioned in issue #443557 (closed)

added workflowin dev label and removed workflowready for development label

mentioned in issue gitlab-org/quality/triage-reports#17344 (closed)

added workflowblocked label and removed workflowin dev label

assigned to @jnutt

mentioned in commit c2dbbff0

mentioned in merge request !150736 (merged)

mentioned in issue #458765 (closed)

changed weight to 2 from 3

changed the description

mentioned in commit 28d3a792

This issue is scheduled for completion in this milestone but is not yet in development. Changing health status to 'needs attention'.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to needs attention

@m_frankiewicz Just a small thought from this comment #451028[User_Mapping_-_CSV.png] (comment 1895017411) I wonder if when a user chooses to Reject the assignment whether we mention somewhere small on the page the option to report abuse? This is based on the idea that even scoping who can receive an invite email to group membership doesn't prevent people from adding users as members of their group in a spammy way, and with CSV upload as an option perhaps the invite email could be used in a way to cause nuisance?

@.luke ok, good point, let's do that. @emilybauman I think it would requires only adding the button with link to the email user receives, to https://docs.gitlab.com/ee/user/report_abuse.html#report-abuse-from-the-users-profile-page

@m_frankiewicz @.luke - Do we want to add it to the email, banner or both? The next page after a user hits reject would show the banner in GitLab, but we could also add it to the email so they could report without having to take any action at all.

@emilybauman I would add it to email I think.

@m_frankiewicz - Added a small blurb at the bottom of the email!

mentioned in design management/design #451028 (closed)[User_Mapping_-_CSV.png]

This issue is scheduled for completion in this milestone but is in an early development stage. Changing health status to 'at risk'.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to at risk

added workflowin dev label and removed workflowblocked label

@jnutt Do we expect some carry over on this issue to %17.1?

This issue ended up being broken into three parts:

MR to track who set the reassignment - merged
MR for accept/decline controller actions - awaiting final approval/merge after losing approvals due to broken pipeline - hopefully this will be closed out today
MR to add in-app confirmation step - broadly complete, just waiting for previous MR to be merged.

Since the confirmation step was outside of the original scope, we could split it out into a separate issue (weight: 1) for the next milestone and mark this as complete when !150736 (merged) is merged.

Otherwise, this will carry over with weight: 1 remaining.

@jnutt Thanks for the clarfication. Can you please open a separate issue for the confirmation scope and add it to gitlab-org/manage/general-discussion#17673 under the Direct Transfer bullet item?

@wortschi Done.

#461192 (closed)

!150736 (merged) is now running the pipeline with MWPS.

mentioned in issue #461192 (closed)

mentioned in commit 76086b08

The current MRs for this have been merged, and the follow-up has been split into #461192 (closed), so I'll close this issue off.

closed

added workflowcomplete label and removed workflowin dev label

removed the relation with #455912 (closed)

removed the relation with #455902 (closed)

mentioned in issue #468437 (closed)

In the discussion on this issue, we had 1 point which I haven't seen implemented (around adding the initial data in helpers). I've created a separate issue for this which should be implemented as milestonep1 #468437 (closed).

Cc @jnutt @m_frankiewicz

@justin_ho you mean that User mapping - Add paginated placeholders data ... (#468437 - closed) should be done asap to unblock FE work?

Justin, does it mean that a helper is missing?

It's dependent on some work (specifically the SourceUsersFinder) by @rodrigo.tomonari in his MR !156687 (merged), so backend can add it once that gets merged.

User mapping - API Development (Controllers)

Designs

Child items ...

Activity