Update invite related endpoints

[maxceem]

This is one of the tasks on the way to complete migration to V5 standards #435

Overview

Our current implementation of invite related endpoints doesn't follow the RESTful style and has a very custom logic. Also, there are some minor peculiarities that blocks us from implementing new features. So in this challenge we would like to update existent endpoints to be more RESTful and support some minor features we need.

Individual requirements

• All the invite related endpoints have to use the next URL route as a base: /v5/projects/:projectId(\\d+)/invites (currently we have a mix of slightly different routes like /v5/projects/:projectId(\\d+)/members/invite(s)).

POST /v5/projects/:projectId(\\d+)/invites

• [Question 1] As a part of the Connect App task, we want to avoid calling Member Service search endpoint client-side. To achieve this, we also have to change the body of create invite endpoint from:

  {
    "userId": 123, // we send userId now
    "email": "mail@mail.com"
   }
  

  to

  {
    "handle": "userhandle", // send handle instead of userId
    "email": "mail@mail.com"
   }
  

  though, the downside is that such a request would be less RESTfull as MemberInvite model doesn't have handle field and has only userId field, see https://github.com/topcoder-platform/tc-project-service/blob/develop/src/models/projectMemberInvite.js#L9.
  • NOTE that if we send handle instead of userId than it would the job of Project Service to call Member Service search endpoint to find userId for the invited user.

GET /v5/projects/:projectId(\\d+)/invites

As per Topcoder V5 standard in the GET endpoints, we always read data from the ES first and if no data is found, we fallback to DB.

• Currently, when request data from ES we use a very specific query which has 2 disadvantages (see code):

  • we must specify the maximum number of invites to return as we use inner hits, but we want to get all of them
  • this specific query is slower than if we just retrieve the whole project by id
  • So we have to replace it with a simple request to get the whole project document from ES. And just get the list of invites from the project document as project.invites. If the project is not found in ES or the list of invites is empty, we should fallback to DB.

• Currently, only users who can "view" the project may call this endpoint. We should allow ALL logged-in the users to call this endpoint. But, we should filter the results:

  • users who can "view" the project should get the full list of invites as they can do now
  • users who cannot "view" the project but is logged-in should get only invitations for themself. So if invitation has userId OR email of the currently authorized user, we return such invitations.
  • non-logged-in users should get error 401

GET /v5/projects/:projectId(\\d+)/invites/:inviteId(\\d+)

• Currently, this endpoint doesn't use inviteId and it gets the invite for the user who is calling this endpoint. Instead, it should return an invitation by inviteId.
• Any user who can "view" project can get an invitation by id.
• We should get data from ES first, and fallback to DB if invitation with the requested id is not found in ES.

PATCH /v5/projects/:projectId(\\d+)/invites/:inviteId(\\d+)

• Use PATCH instead of PUT
• This endpoint should find an invitation to update inviteId instead of userId or email
• Only status can be updated
• The rest logic should be kept as it is, like: we should only be able to update invites in status pending or requested and so on.

DELETE /v5/projects/:projectId(\\d+)/invites/:inviteId(\\d+)

• [Question 2] We don't have such an endpoint and look like we still don't need it as instead of deleting we updated invitations to have the status canceled.

References

• Create a new "PUT /v4/projects/:projectId(\\d+)/members/invites/:inviteId" endpoint  #422
• Cannot remove invitation by email after masking emails appirio-tech/connect-app#3413
• When successfully invite user by email it's not cleared from the input appirio-tech/connect-app#3412
• Masking email to be shown under Team management container appirio-tech/connect-app#3388

[maxceem]

@vikasrohit I've summarized the changes which we should do for invite related endpoints to make them more RESTful and to be ready to implement other features, like masking emails. It would be great if you could have look, and in particular, there is a couple of questions which I'd like to confirm.

[vikasrohit]

Specs looks good to me. For questions:

1. I guess, we can go with this as there is not option for us to update our model to store handle instead of userId as handle can change frequently.
2. Yes, we should call DELETE endpoint to cancel the invite. And we should restrict the PATCH endpoint to not allow updating the invite with status cancel.

[maxceem]

@vikasrohit We've got all the endpoints updated. Though I've excluded DELETE endpoint from the challenge to reduce the scope and was going to handle it now.

Though during writing the SPEC for DELETE endpoint I've understood, the logic of this endpoint would 60% repeat the logic of PATCH endpoint, like finding the invite to delete https://github.com/topcoder-platform/tc-project-service/blob/feature/restful-invites/src/routes/projectMemberInvites/update.js#L40-L55 and especially various cases of whom and when can delete it (mark as canceled) https://github.com/topcoder-platform/tc-project-service/blob/feature/restful-invites/src/routes/projectMemberInvites/update.js#L57-L68.
Also, the logic in Project ES Processor would 100% duplicate the logic of update event: https://github.com/topcoder-platform/project-processor-es/blob/develop/src/services/ProcessorServiceProjectMemberInvite.js#L60-L71.

My concern is, that for implementing this endpoint we would have to repeat the same logic that we have for PATCH endpoint. In the future when adding some new permissions cases we would have to update both endpoints, which is prone to issues.

As such a DELETE endpoint doesn't delete invite and just changes the status to canceled it feels like it is just a particular case of PATCH endpoint. So IMHO it may be more clear just to keep PATCH endpoint that updates status to canceled. Having DELETE endpoint which doesn't actually delete, but only changes the status may be confusing and would require additional clarifications when someone comes across with such behavior. While not having a DELETE endpoint clearly indicates that invites cannot be deleted.

So I propose that we don't have DELETE endpoint as we actually don't allow deleting invitations.

What do you think?

[vikasrohit]

I fully agree with the concerns. But while saying to implement the DELETE endpoint and removing support for changing status to cancel in PATCH endpoint, I weighted the v5 standards (in turn REST standards) more than technical implementation and understanding of the API. I am thinking that we can abstract the cancel status from API caller by implementing the said requirements (may be we need to update the GET endpoints as well to not return cancelled invites, if they are returning them right now). I guess, we can some refactoring for common code problem between DELETE and PATCH.
Let me know what do you think? I am still in favor of having the DELETE endpoint with other proposed changes to abstract the cancel status and make things more RESTful. But, I am also ready to give it more time to think about before actually implementing it.

[maxceem]

@vikasrohit Yes, I understand the desire for having all 4 endpoints to follow RESTful pattern.

And in fact, all the GET endpoints don't return canceled invites as we don't keep them in ES and filter from DB. So marking invites as canceled remove them from responses, which kind of equals to deleting them.

On the second thoughts we have only to pieces of code which have to be reused between these endpoints:

• Get invite for updating/deleting https://github.com/topcoder-platform/tc-project-service/blob/feature/restful-invites/src/routes/projectMemberInvites/update.js#L40-L55 - this should be moved to reusable method pretty naturally.
• Check various permissions on who can make actions https://github.com/topcoder-platform/tc-project-service/blob/feature/restful-invites/src/routes/projectMemberInvites/update.js#L61-L68. Only this has slightly different logic between endpoints.
• Ah, and we would have to duplicate the code for handling delete events in project-processor-es - I don't feel like creating a reusable code there would be good.

So I'm fine with giving a try for having a DELETE endpoint.

[maxceem]

@vikasrohit if you remember we are going to return email hash for emails in invites as we are going to maks emails.

First I was thinking to create email hash in runtime at the time we return the response. Though, I think that probably, we would like to use some more strong hash function than md5. We already reveal several characters from the email address and md5 function is quite fast, so it would be theoretically possible to decode email using brute force.

But if we want to use a stronger method to create email hash like SHA256 it probably would be slower, thus it may be not good to create hashes at runtime. So it could make sense to add a new field emailHash to the invite model and create hash when we create invite or update invite email.

This might be overcautious, what do you think?

[vikasrohit]

Ah, and we would have to duplicate the code for handling delete events in project-processor-es - I don't feel like creating a reusable code there would be good.

I guess, even in the event listener we might create a reusable code. It depends on the actual action that both event listeners perform e.g. https://github.com/topcoder-platform/project-processor-es/tree/develop/src/services

This might be overcautious, what do you think?

I think it is okay to have hash stored in the database to keep things simple and not looping through the list of objects in the route. On the other side, we can can make brute force of MD5 a bit more difficult by not disclosing the length of the email.

[maxceem]

we can can make brute force of MD5 a bit more difficult by not disclosing the length of the email.

This is a good idea @vikasrohit I think. We can always show emails in the shape first 2 and last 1 character with 3 stars in the middle:

ab***f@aa***a.com

Though we have to decide how to deal with the shorter emails like:

a@a.com
ab@aa.com
abc@aaa.com

I guess if name/domain is shorter than 4 characters we can always show 3 stars:

***@***.com

What do you think?

[vikasrohit]

We can always show emails in the shape first 2 and last 1 character with 3 stars in the middle:

For now lets go with first 1 and last 1 character and 3 stars in the middle.

I guess if name/domain is shorter than 4 characters we can always show 3 stars:

I think we can keep the same implementation because no one (other than our database) can know that it is actually less than 4 characters. 😬

[maxceem]

@vikasrohit this is already done, integrated with Connect App and pushed to PROD. So this issue can be closed.