appirio-tech / connect-app

Build your next project on Connect with the power of crowdsourcing
https://connect.topcoder.com
44 stars 140 forks source link

Avoid calling /v3/members/_search client-side #3387

Open maxceem opened 4 years ago

maxceem commented 4 years ago

At present member team is doing some major fixes for the search of members and security aspects of the member endpoint. We need to start working on getting the connect front end free of _search api call and use _search on backend only via m2m just like we recently did for members and invites endpoints, see https://github.com/topcoder-platform/tc-project-service/pull/414

Places where we call /v3/members/_search endpoint in Connect App

  1. Project Details - Messages When we show topics, we load users to show authors of topics/post with tooltip by userId:

    image

  2. Projects List On the Project List page we are showing projects and member of them with tooltips:

    image

  3. Project Details - Assets Library We are showing uploaded files and links with full name of the owner and tooltip:

    image

  4. Project Details - Team - Members and Invites - We are already making Project Service changes so we don't need to call _search anymore for this place.
    updated!
    this change is already done, and data comes from Project Service, though Project Service internally calls _search endpoint using M2M, see https://github.com/topcoder-platform/tc-project-service/blob/develop/src/util.js#L559-L579

  5. Project Details - Team - New Member Invitation When we are inviting a new member by handle we first find his userId using _search endpoint and after we send request to the Project Service using userId. We may change this and we may send handle directly to the project service while inviting a member. This would require making us a corresponding change in Project Service.
    updated!
    we don't call this endpoint anymore (not yet merged to dev branch, but almost ready), and we invite users by handle. BUT Project Service still calls _search endpoint to get user details by handle during invitation process https://github.com/topcoder-platform/tc-project-service/blob/feature/restful-invites/src/util.js#L581-L605 using M2M

@vikasrohit from the list above it's clear how to handle situations 4 and 5. But 1, 2 and 3 I'm not sure how to cover as these cases are not covered by project members and project invites endpoints.

maxceem commented 4 years ago

@vikasrohit could you please have a look at this when you have time, especially on the cases 1-3. Actually to choose a good approach for cases 4 and 5 it's better to know how we would also handle cases 1-3, so we can achieve a better code structure here.

vikasrohit commented 4 years ago

@maxceem I think for 1-3, in most of the cases the user for which we are going to show the tooltip for, would be the member of the project and all (as per the roles of the logged in user) the data of that member would be easily available to be rendered in the tooltip. Now remains the cases where concerned user is not part of the project now, e.g. the user is admin who made a post using admin powers without joining a project or a regular user who posted a thread or uploaded a file in past but left the project afterwards. For such users, we may call the https://api.topcoder-dev.com/v3/members/_search?query=userId:<user_id>&fields=id,firstName,lastName,handle to get the basic details of such users to render on the tooltips. We may update our tooltip component to be connected with the redux state to load information for such users on run time to reduce the initial API calls on the project load. If that causes too much of work, we may identify and capture information for such users on page load in a separate or possibly in the same redux state on the page load.

maxceem commented 4 years ago

Ok, so basically, we would keep calling /v3/members/_search endpoint for cases 1-3.

Also, I would like to note, that for case 2 - Project List page we would load all the users' data by /v3/members/_search even though they are members. As on the project listing, we are not calling a special endpoint for getting member with additional details GET /projects/{id}/members?fields=handle,firstName,lastName.

vikasrohit commented 4 years ago

As on the project listing, we are not calling a special endpoint for getting member with additional details

Oh, I skipped that part. But I think we need to call _search endpoint anyway as per my thoughts. So, we should be safe in that part as well. In nutshell, we would still be calling _search endpoint from front end for 1-3 use cases, however, we won't request email from the api in such places, even though logged in user is admin because it might require adhoc code to get email for admins as well. For 4-5 we would use the new endpoints which returns augmented member data using m2m calls.

maxceem commented 4 years ago

Got it.

Regarding email. There is also another issue from Rishi https://github.com/appirio-tech/connect-app/issues/3388 to show it with asterisks for Project Members / Invites.

So regarding email as I understand from now on:

vikasrohit commented 4 years ago

I think we can still show emails of project members to admins who are not project member yet because we are already augmenting that data in our new endpoint and we can control the visibility based on role of the requesting user. However, having said that, I am okay for now showing emails even for admins if they are not part of the project, if that makes our life easy.

maxceem commented 4 years ago

Looks like we are talking about slightly different things here.

we would still be calling _search endpoint from front end for 1-3 use cases, however, we won't request email from the api in such places, even though logged in user is admin

From the above comment, I understand that when we use _search endpoint, i. e. for cases 1-3 we don't show email for admins.

While from the task of Rishi https://github.com/appirio-tech/connect-app/issues/3388 I understand that we still show email to admins for members and invited users - cases 4-5 - no matter if admin himself is a member or no.

So what I wanted to emphasize here, is that admins would see email in some cases (4-5) and wouldn't see email in other cases (1-3). This is regardless of admin himself is a member or no.

vikasrohit commented 4 years ago

From the above comment, I understand that when we use _search endpoint, i. e. for cases 1-3 we don't show email for admins. Correct. I am saying about only in the project details page, we can show emails to admins even if they are not part of the project but only if that does not increases our code complexity and efforts. Otherwise, I am okay with showing emails to only admins and only when they are part of the project. Lets discuss that, after the confusion for other things clears here.

While from the task of Rishi #3388 I understand that we still show email to admins for members and invited users - cases 4-5 - no matter if admin himself is a member or no.

The issue is more concerned about invites where a project member has invited other using their email. In such cases, we want no one except the admin to see those emails in unmasked format.

I wanted to emphasize here, is that admins would see email in some cases (4-5)

Sorry, but I still don't get which use case is there where admin won't see the email of project members or invites for 4-5 scenarios.

maxceem commented 4 years ago

The issue is more concerned about invites where a project member has invited other using their email. In such cases, we want no one except the admin to see those emails in unmasked format.

Ah, got it.

Sorry, but I still don't get which use case is there where admin won't see the email of project members or invites for 4-5 scenarios.

Cases 4-5 are good. But in cases, 1-3 admins would not see email as per:

we would still be calling _search endpoint from front end for 1-3 use cases, however, we won't request email from the api in such places, even though logged in user is admin

And regarding the next thing:

we can show emails to admins even if they are not part of the project but only if that does not increases our code complexity

I doesn't increase the complexity. In fact, it's easier to show emails to admins no matter if admin himself is a member or no.

vikasrohit commented 4 years ago

Cases 4-5 are good. But in cases, 1-3 admins would not see email as per:

Okay. For 1-3, I am okay with not showing emails for admins.

I doesn't increase the complexity. In fact, it's easier to show emails to admins no matter if admin himself is a member or no.

Great! Go ahead then.

In nutshell, no one would see emails in connect except admins. Even admins need to view project details page to see emails of the members/invitees, they can not see the emails on the project listing page. Further, related to #3388 even an existing project member won't be able to see, in clear text, the email of user he/she invited, after inviting.

maxceem commented 4 years ago

As discussed with Vikas on Slack we do have to get rid of the dependency on userId in search endpoint. For the case 2: just simply call newly implemented endpoint projects/123/members for all projects rendered on the page in batches e.g. right now our page has 20 projects listed on page load, so we can send 4 batches of 5 requests to fetch the member details after everything is rendered on the page.

maxceem commented 4 years ago

For the cases 1 and 3 i. e. for users who are not members of the project, here is the recommendation from Vikas to go:

And I am okay with showing our standard Connect User for such users. That would indicate that user is not part of the project now. We might show some text stating the same in the tooltip for such users. Further for admins, we can show the userId (which have in our api databases) in the tooltip for monitoring and debugging purposes.

vikasrohit commented 4 years ago

@maxceem I guess you can remove the Have a question tag now and we can start working on it. We are good to have this after our upcoming release (2.4.16/2.5).

RishiRajSahu commented 3 years ago

@maxceem I think we are now fetching member details via project-svc, so could we close this issue ?

maxceem commented 3 years ago

@RishiRajSahu this is done only partially. From the quick look, we have only done cases 4-5, and cases 1-3 are still using this endpoint.

vikasrohit commented 3 years ago

@RishiRajSahu lets keep this issue open than, but I am moving it to Backlog for now.