Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: #888 transfer to admin table #1068

Merged
merged 11 commits into from
Dec 9, 2023

Conversation

MCatherine1994
Copy link
Contributor

@MCatherine1994 MCatherine1994 commented Dec 5, 2023

refs: #888

  • add flyway script to migrate user with fam role to fam application admin table
  • update auth function to add fam application admin to role list when user login through fam itself
  • update the auth function test to verify the admin role can be added as expected
  • update the backend and admin management router guard, replace the hardcoded admin role from "{app_name}_ACCESS_ADMIN" to "{app_name}_ADMIN", cause once we merge this pull request, auth function will append admin roles as "{app_name}_ADMIN"

- flyway script to migrate fam role to fam application admin table
- update auth function to read fam application admin when user login through fam itself
- update the auth function test to verify the admin role can be added as expected
refs: #888
@MCatherine1994 MCatherine1994 marked this pull request as draft December 6, 2023 00:00
@MCatherine1994 MCatherine1994 marked this pull request as ready for review December 6, 2023 01:25
@MCatherine1994
Copy link
Contributor Author

I tested the new flyway script locally, and if we feel it looks correct, I'll run in tools space first.

@ianliuwk1019
Copy link
Collaborator

ianliuwk1019 commented Dec 7, 2023

Hope I am wrong, or maybe I am just confused myself, but would still like to ask this question:

Although if this PR is good, should this PR be pending for merge until either "frontend" or somehow "backend api" starts directing "role assignment" for "xyz_ACCESS_ADMIN" role to the new "fam_application_admin" table first?
If this PR flyway script is promoted to production first, but our frontend/api isn't ready to make a switch yet, then will later the records for "xyz_ACCESS_ADMIN" in fam_user_role_xref be out of sync than current state (if, for example, Basil is being added to "SPAR_PROD_ACCESS_ADMIN" in current fam_user_role_xref table (after this PR script is promoted in PROD) then he will not exist in fam_application_admin table.
@MCatherine1994 @basilv

@basilv
Copy link
Collaborator

basilv commented Dec 7, 2023

Agree somewhat with Ian that we can't release this functionality until the rest of it is done. We could merge this and deploy to dev, understanding that we will no longer be able to add admins in dev until the rest of the associated changes are done. (we can add admins, but will be in the wrong table and this won't be used to determine authentication).

@MCatherine1994
Copy link
Contributor Author

I agree Ian's point. I think we can cut a release today without merge this one, and let the work we have done go to prod. We haven't do any prod deployment recently. Once we cut a release, we can merge this one to dev, and work on frontend implementation next sprint, and deploy them together later.

ianliuwk1019
ianliuwk1019 previously approved these changes Dec 7, 2023
Copy link

sonarqubecloud bot commented Dec 8, 2023

[nr-forests-access-management_admin] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@MCatherine1994
Copy link
Contributor Author

Verified in AWS tools space

we have these persons have FAM roles:
AWS_tools_1207

migrate them into the new fam_application_admin table:
AWS_tools_app_admin

@MCatherine1994 MCatherine1994 merged commit fac94f1 into main Dec 9, 2023
14 checks passed
@MCatherine1994 MCatherine1994 deleted the feat/888-transfer-to-admin-table branch December 9, 2023 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants