[Federation] Add an Endpoint for Conversation Member Removal based on Qualified User ID

[mdimjasevic]

Note: This PR is based on Paolo's PR #1707, which fixes Swagger for responses with the same status code. Perhaps it would make sense to review and get that PR merged before this one; the difference to develop will reduce once that is done.

The PR introduces a new endpoint that enables removing a conversation member based on their qualified user ID from a local conversation: DELETE /conversations/:convId/members/:domain/:usr. It can also be used for a user to leave a conversation.

The PR also rewrites the unqualified existing endpoint DELETE /conversations/:convId/members/:usr from wai-route to Servant.

All the error responses of both the qualified and unqualified endpoints have been made explicit, by expressing them via types. Now it is clear there are 7 error responses plus a no-change response, in addition to a response when an update took place. Assuming a switch to OpenAPI 3 is made, an automatic additional benefit of the rewrite for clients will be an updated Swagger documentation that enumerates all the responses (currently, due to the servant-swagger package, multiple responses with the same status code get collapsed into one response in Swagger; PR #1707 works around that). Furthermore, the unqualified endpoint had a Swagger that was out of sync with the actual implementation, and the rewrite addresses this issue.

Checklist

• The PR Title explains the impact of the change.
• The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
• If end-points have been added or changed: the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
• If a schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
• Section Unreleased of CHANGELOG.md contains the following bits of information:
  • A line with the title and number of the PR in one or more suitable sub-sections.
  • If /a: measures to be taken by instance operators.
  • If /a: list of cassandra migrations.
  • If public end-points have been changed or added: does nginz need upgrade?
  • If internal end-points have been added or changed: which services have to be deployed in a specific order?

[akshaymankar]

Because only a local conversation ID is given in the endpoint, there is no way to use the endpoint for a remote user to leave a conversation. A follow-up PR will introduce a new endpoint for such a case.

Can we already just make one endpoint then? I don't think it is nice to make clients decide which endpoint to use.

In services/brig/src/Brig/Provider/RPC.hs there is a function removeBotMember that attempts to parse a response body that contains an Event in the JSON format by using the FromJSON instance. @akshaymankar suggested the FromJSON instance might be used only in tests, but I'd argue this is enough evidence that the FromJSON instance is used elsewhere.

Along with the integration tests, I think it is also OK to break backwards compatibility of parsing JSONs between brig and galley as we upgrade them together. So, I am still not convinced that we need to add another event type.

[mdimjasevic]

@akshaymankar , @pcapriotti : This PR is ready for reviewing. Akshay already made two comments:

• I can get rid of that new conversation event type because its FromJSON instance is used only between Brig and Galley. I'll do that next.
• Akshay proposes to modify this endpoint (DELETE /conversations/:convId/members/:domain/:usr) to also cover the case of leaving a remote conversation, though my plan until that proposal was to make a separate endpoint in a separate PR. I'm in favor of unifying the two operations under one endpoint. @pcapriotti , do you see any downsides to it? I'll just have to make the conversation ID qualified in the endpoint path.

[pcapriotti]

Akshay proposes to modify this endpoint (DELETE /conversations/:convId/members/:domain/:usr) to also cover the case of leaving a remote conversation, though my plan until that proposal was to make a separate endpoint in a separate PR. I'm in favor of unifying the two operations under one endpoint. @pcapriotti , do you see any downsides to it? I'll just have to make the conversation ID qualified in the endpoint path.

The problem with having a separate endpoint is that they overlap in the case when you want to leave a local conversation. Since it seems we are going to implement remote admins anyway going forward, I think it makes sense to have a single endpoint with everything qualified and simply return a federation-not-implemented error for the cases that haven't been covered so far.

It seems you're planning to keep making changes to this PR. Should I wait before reviewing?

[mdimjasevic]

It seems you're planning to keep making changes to this PR. Should I wait before reviewing?

Yes, please wait. I'll implement leaving via the same endpoint and get rid of that extra event type. Once I'm done, I'll let you know.

[akshaymankar]

Looks good now 🚀