Handle negative user id (m2m user) in project.action.update event

[vikasrohit]

Because now we can have negative values in updatedBy field for a project, we have to fix the handling of the project.action.update (and may be same for project.action.create) to support negative user ids. As of now it is throwing following error:

Error happened in processUpdate
Error: User with id: -101 doesn't exist.
at getUserHandle (/legacy-project-processor/src/services/ProcessorService.js:78:11)

fyi @maxceem

[maxceem]

@vikasrohit this is connected with syncing billing account functionality which is already there. Somehow to check if user can access billing account we need to know user handle and for user with id -101 we cannot find the handle, as we cannot find user:

So the issue is not with negative id, but because such user doesn't exists. I guess the solution should be one of these:

1. if we cannot get user handle, just return that user cannot access billing account, but don't fail
2. if we cannot get user handle, just return that user CAN access billing account, but don't fail - this is dangerous as we might grant access in some situations where we don't want it
3. hardcode a list of special userIds to whom we want to grant access to billing account - this variation of the 2nd

[vikasrohit]

Apologies for replying late on this @maxceem. I have few concerns and queries here:

1. What do you mean with not failing but returning that user can not access billing account? You mean using boolean false value without raising an error? If yes, I guess that does not affect the actual functionality right? It would be just code refactoring or are we missing some further operation from happening because we are throwing error right now?
2. I think the ideal solution to determine the access to the billing account would have been possible if we could look up the scope of the calling application. I mean negative numbers in updatedBy field denotes that it is called by an application using m2m token and we have some plan to use hash of the auth0 app's client id instead of this token in future to better recognize the actual machine application. If we have that client id and some endpoint in auth0 to look up the scopes of that application, we could have authorized the m2m app to update the billing account as well. But I think that is very very high amount of efforts.
3. I also think that we are safe for now w.r.t. billing account update because for near future, caller is going to use the user token to update the billing account not the m2m token. But as soon as they would migrate to use m2m token, we would be in trouble.
4. Considering all situations we have right now, I guess, we have to keep mapping of such negative numbers (or hash of auth client id in future) and their access to billing accounts. Something like -101: assign_billing_account, add_copilot which says m2m application with id -101 has scopes assign_billing_account and add_copilot and we can proceed further, if we have that scope. In other words, we can keep dynamic mapping via env variables, to authorize changes done by m2m calls.

[vikasrohit]

@maxceem carrying from #13, some improvements that we can do to avoid too many errors of negative user ids:

1. We can avoid making query to the database if the userId is negative

   legacy-project-processor/src/services/ProcessorService.js

   Line 76 in 8f7cf1a

   const handleRes = await connection.queryAsync(`select handle from common_oltp:user where user_id = ${userId}`)

2. And as you said in Handle negative user id (m2m user) in project.action.update event #12, we can avoid throwing error and instead just return null handle when either userId is a negative number OR we don't find the user with the given user id in the database and don't make the query

   legacy-project-processor/src/services/ProcessorService.js

   Line 97 in 8f7cf1a

   directAccessIds = await connection.queryAsync(`select p.project_id as id from time_oltp:project as p left join time_oltp:client_project as cp on p.project_id = cp.project_id left join time_oltp:client c on c.client_id = cp.client_id and (c.is_deleted = 0 or c.is_deleted is null) where p.active = 1 and p.start_date <= current and current <= p.end_date and p.active = 1 and p.project_id in (SELECT distinct project_id FROM time_oltp:project_manager p, time_oltp:user_account u WHERE p.user_account_id = u.user_account_id and p.active = 1 and upper(u.user_name) = upper('${userName}') union SELECT distinct project_id FROM time_oltp:project_worker p, time_oltp:user_account u WHERE p.start_date <= current and current <= p.end_date and p.active =1 and p.user_account_id = u.user_account_id and upper(u.user_name) = upper('${userName}'))`)

   if returned handle is null.

[maxceem]

@vikasrohit during the fresh look, it feels that we don't have to make updates as I've suggested before and as you've summed up here #12 (comment).

As method checkCanAccessBillingAccount anyway throws an error if the user doesn't have permissions. So if the method getUserHandle throws an error when it cannot find a user it looks like there is no harm. Just another text message of the thrown error.

I guess the only thing we have to make is to update method checkCanAccessBillingAccount, so before trying to get username:

const userName = await getUserHandle(connection, userId)

it should check if we have a user in the special config as per your suggestion:

• If userId is listed in this special config, then use this config to determine is use has access based on the scope value
• if userId is not listed in the special config, then try get user from DB as usual using const userName = await getUserHandle(connection, userId) and if it fails, then user doesn't have access, same as now.

[maxceem]

@vikasrohit I've created a hotfix for this issue as per discussion on Slack. Let's merge it to DEV first and do the testing on DEV environemnt as it's very time-consuming to make a proper test locally.

• [DEV] Hotfix PR [HOTFIX] [DEV] Remove billing account permission check #18
• [PROD] Hotfix PR [HOTFIX] [PROD] Remove billing account permission check #19

Solution Summary

Stop checking permissions for updating billing account. We cannot do it properly if update has been made using M2M token with a negative userId. Instead we would rely on the fact, that if message to update billing account has been posted to Kafka, than service which posted the message already checked the permissions. And the processor only sync data back to legacy DB without permission check.

[vikasrohit]

Merged the branch in dev. I will check if it works or not.

[maxceem]

As now we have removed the permission check in the Legacy Project Processor we have to consider re-enabling it in the Project Service, see related issue topcoder-platform/tc-project-service#581.

[vikasrohit]

Merging the prod PR as well, as I don't see any specific error in dev though I am not able to verify the exact use case because we don't have quickly accessible data in salesforce which can be used for testing. I would like to monitor it on production in logs, and will revert in case it fails.