Defect when provisioning access to a deleted role assignment

[ArogeG]

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

1. Go to 'FAM TEST'
2. Select FOM application
3. Click on 'Grant Access screen'
4. Create a new user (BCeID user) with FOM submitter role and Forest client (00082820)
5. Once created, delete role assignment
6. create user again by repeating step 3-6

Actual Result: Application Error has Occurred

Expected Result: User role assignment has been created successfully

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: [e.g. iOS]
• Browser [e.g. chrome, safari]
• Version [e.g. 22]

Smartphone (please complete the following information):

• Device: [e.g. iPhone6]
• OS: [e.g. iOS8.1]
• Browser [e.g. stock browser, safari]
• Version [e.g. 22]

Additional context Add any other context about the problem here.

[MCatherine1994]

Debugging screenshot in Prod RDS database:

https://app.zenhub.com/files/486378283/3f084673-3e02-49ea-bebb-653a49a2dcfd/download

[ianliuwk1019]

Added prod log from Basil. https://app.zenhub.com/files/486378283/81d5ecb1-790e-4473-8fbc-144343c120d2/download

[ianliuwk1019]

From the log and prod data: Backend has a check on if user exists, based on 'user_type_code' (B or I) and 'user_name' (ilike - case insensitive), before creating a user. For this case from production database query, it shows 2 fam_user records ("DVALK" and "DValk"). So, backend returns error when the check found more than 1 user with "the same" name and type.

famdb=> select * from fam_user where UPPER(user_name)='DVALK';
 user_id |            user_guid             |                          cognito_user_id                          | user_name |                   create_user                   |
 create_date          |  update_user  |      update_date       | user_type_code
---------+----------------------------------+-------------------------------------------------------------------+-----------+-------------------------------------------------+---------
----------------------+---------------+------------------------+----------------
     552 | FDFEE614672C4B56A3E168CD91C790F7 | prod-bceidbusiness_fdfee614672c4b56a3e168cd91c790f7@bceidbusiness | DValk     | fam_proxy_api                                   | 2023-04-
12 00:00:00+00        | fam_proxy_api | 2023-04-12 00:00:00+00 | B
     551 |                                  |                                                                   | DVALK     | prod-idir_7f27542b4f1a45478f90b967ff34d74b@idir | 2023-04-
12 18:07:59.618679+00 |               |                        | B
(2 rows)

• Based on prod data: First fam_user record "DVALK" (user_id=551) looks like is created by Julius using FAM frontend to create user/role assignment. Second fam_user record "DValk" (user_id=552) likely is from the user login to FOM (and based on prod data, it should be user's first time and no role returned for this login user to FOM)

• How the auth-lambda function created second fam_user (user_id=552) when there exists already first fam_user (user_id=551): Possibly, the auth-lambda function is based on unique constraint: CONSTRAINT fam_usr_uk UNIQUE (user_type_code, user_name) and this isn't case insensitive. The user with the same user_type could have same name with different case ("DVALK", "DValk"). In this case, auth-lambda could not find and update first user.

      INSERT INTO app_fam.fam_user
          ...
          ...
          ON CONFLICT ON CONSTRAINT fam_usr_uk DO
          UPDATE SET user_guid = {user_guid},  cognito_user_id = {cognito_user_id};

Need to discuss possible solutions for the two issues on this problem:

• Enhance fam_usr_uk constraint:
  • by creating another UNIQUE INDEX constraint (reference)?
  • by using ' citext ' new postgresql column datatype (reference)?
  • then use above to fix auth-function code if needed.
    
• Data-Fix on (user_id=552) record:
  • flyway migration (is it possible to have conditional flyway only to apply to PROD)?
  • new 'delete user' endpoint with special user permission so we can fix it from frontend (or swagger) - thanks to Pete's idea? @basilv @gormless87 @MCatherine1994 @conbrad

Likely we need to have code fix first, then apply datafix, so when user login again the auth-function can work correctly without adding second user again.

[ianliuwk1019]

Note, my thinking are:

• Auth-lambda code will need to be fixed first for not getting more bad data from new users.
• PR #534 (feat: #450 bcsc integration) probably will depends on this code fix before bcsc integration can be released, as the PR contains a flyway script to empty all users (fam_user) cognito_user_id column linking to current PROD Cognito user pool's user due to this pr changes requires to re-create user pool (not only user pool id change, each user's cognito_user_id will also be different)
• The cognito_user_id looks something like this prod-bceidbusiness_fdfee614672c4b56a3e168cd91c790f7@bceidbusiness with random hash (appear that way to me), so it is not likely we can build a generic flyway to do migration for all users for data fix after rolling out bcsc integration pr #534 without this ticket's code fix first to prevent bad data.
• FAM does have a delete_fam_user from old time but currently being commented out.

  @router.delete("/{user_id}", response_model=schemas.FamUser)
  def delete_fam_user(user_id: int, db: Session = Depends(database.get_db)):
  """
  Delete a FAM user
  """

  So what we need probably are: add appropriate security level check (role) and make sure it is working and leaving log trace (for now, or other consideration?); and use it for emergency 'fam_user' table (only) fix.

[MCatherine1994]

Just add some notes:

• When a new user has been created through the FAM UI, the username depends on whatever the user types in, we don't make changes
• When a new user has been created through the first time login to the application integrated with FAM, the username will be the "custom:idp_username" from IDP

In the future, if we integrate with the IDP web service lookup, we could always store the idp username to prevent this issue happen again

[basilv]

I don't understand these comments - the user name must be identical in both scenarios (or at least matched to the same user record), or FAM won't work as intended. Scenario 1: User John Smith logs into FAM. Their IDIR is supplied as JSmith. A user record is created (if it doesn't already exist), or is updated, matching on the user id (and domain). Scenario 2: An admin grants access to user John Smith. They likely type JSMITH or jsmith for the user id. A user record is created (if it doesn't already exist), and an access assignment is created linking to the user identified by the user id (and domain)

FAM must ensure that the user record in scenario 1 and scenario 2 are the same, irrespective of the order they occur in.

[MCatherine1994]

Yes, so when create user through FAM UI, we call this method to check if there is an existing user, think it's case insensitive now

But when the user has been created through the auth function, as Ian mentioned above, it's checking the constraint fam_usr_uk, which is case sensitive

ALTER TABLE app_fam.fam_user ADD CONSTRAINT fam_usr_uk UNIQUE (user_type_code, user_name);

[MCatherine1994]

I just document how we fixed the dev deployment here, we could move it to other place later:

To remove the KMS key, we need to use the terragrunt cli:

• download required cli tools
  • terraform: https://developer.hashicorp.com/terraform/downloads
  • terragrunt: https://terragrunt.gruntwork.io/docs/getting-started/install/
• login to terraform cloud, the token can be found through here
• Go to our repository directory, for FAM DEV environment, go to the terragrunt file directory for our FAM DEV, which is at terraform/dev/terragrunt.hcl
• Set up the environment variable export tfc_workspace=sfha4x-dev, 'sfha4x-dev' is our license plate for dev
• Run terragrunt init and terragrunt state list to see all the state
• And then run terragrunt rm [kms_key_state]

But Basil and I both have problems run the terragrunt command, it hangs there, so we got the help from Warren Uniewski, he helped us remove that bcsc kms key from the terragrunt state. So the key is still in the AWS scheduled for deletion, but terraform will ignore it

[MCatherine1994]

In order to fix this problem,

• we first fixed the FOM-DEV deployment, reference my last comment
• and then we created a branch for the hotfix (we renamed this branch a few time because of not understand the hotfix release previously, think at the end we use the branch called hotfix-522), this branch is created from the last release v1.1.1, and added the following changes:
  • update the constraint that the auth function is using, update that to support case insensitive when check if the user exists or not
  • add flyway script to find and remove duplicate user records
  • update local cognito env variables, so tests can run in pipeline
  • merged a specific commit that we must have, which is Conrad's change for RDS postgres verison
  • merged a specific commit that we must have, which is FOM demo callback urls
• and then we deployed that branch to dev, tested and verified it works in dev (because we have bcsc in test, we used dev to test this hotfix change)
• and then we fixed the hotfix release please pipeline, updated the target branch to be "hotfix", so whenever we merge a branch to the branch called "hotfix", it can trigger the hotfix release
• and then we created a branch called "hotfix" from the last release tag v1.1.1
• and then we merged our fix branch (hotfix-522) into the "hotfix" branch, so it triggered the hotfix release please pipeline
• and then we merged the hotfix release pull request https://github.com/bcgov/nr-forests-access-management/pull/558, a new version tag got created automatically v1.1.2
• and then we did a hotfix prod release from the latest hotfix release tag v1.1.2
• and then we created another pull request to merge the hotfix branch into main https://github.com/bcgov/nr-forests-access-management/pull/559

[ArogeG]

Thanks all for working urgently on this, much appreciated.