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

Return db user instead of Auth User in me endpoint. #1247

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

Sujanadh
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Related Issue

user_role was not updated in me endpoint. The default mapper role was passed, irrespective of other roles.

Describe this PR

This PR updates me endpoint, which returns dbuser with respective roles instead of authuser, as authuser doesn't possess any roles.

Screenshots

N/A

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@Sujanadh Sujanadh requested a review from nrjadkry February 22, 2024 09:24
@Sujanadh Sujanadh self-assigned this Feb 22, 2024
@github-actions github-actions bot added the backend Related to backend code label Feb 22, 2024
@spwoodcock
Copy link
Member

spwoodcock commented Feb 22, 2024

This was an intentional choice at the start to prevent the extra database call, if it's not needed.

I'm assuming the frontend needs to store the role from one of: READ_ONLY, MAPPER, or ADMIN for something?

I'm not that confident in enforcing roles via the frontend - this is easily manipulated. Each endpoint we call has associated auth/role requirements anyway. It depends what the role is needed for πŸ‘

@Sujanadh
Copy link
Collaborator Author

This was an intentional choice at the start to prevent the extra database call, if it's not needed.

I'm assuming the frontend needs to store the role from one of: READ_ONLY, MAPPER, or ADMIN for something?

I'm not that confident in enforcing roles via the frontend - this is easily manipulated. Each endpoint we call has associated auth/role requirements anyway. It depends what the role is needed for πŸ‘

Yes, the frontend needs to know the role of the user for the verification tag in the organization list, which looks like this:
image

@Sujanadh
Copy link
Collaborator Author

Only superusers are allowed to view these tags.

@spwoodcock
Copy link
Member

spwoodcock commented Feb 22, 2024

The organisation page is low priority for now, but I wanted to get some feedback related to #1234 @NSUWAL123 @Sujanadh @manjitapandey

The idea here is that we can distinguish more easily between verified and unverified orgs - which sounds great on paper.
The thing is, it complicates the logic (this PR will require two database calls whenever login_required is used in any other endpoint, and it also adds admin specific component flags in the UI).

The admin for FMTM is generally only one user, maybe a handful at most. They will generally be technically proficient, with access to the database etc.

Do we think it's necessary to make UI components and endpoints specific for admin users, when they can just update things via the database?

@spwoodcock spwoodcock mentioned this pull request Feb 22, 2024
11 tasks
@manjitapandey
Copy link
Contributor

I understand that if its only about super admin role and we are sure that we wont be needing it on future, calling two databases won't probably a good idea.
Can you provide any suggestions regarding?
Or else we can go through viewing the list from database directly per now until we come up with some alternative solution.

@spwoodcock
Copy link
Member

spwoodcock commented Feb 23, 2024

If the user is an admin then all orgs are returned.

We just need to tag the unapproved orgs to give better oversight of that for the admin.

To do that we don't need to check if the user is admin, we only need to check if the org has approved=false (note that regular users can't list unapproved orgs from the backend).

Then in the future we can convert the tag to a button, so the admin clicks and it calls the approve endpoint.

Would that work?

@spwoodcock
Copy link
Member

Closing unless a good use case is determined.

@spwoodcock spwoodcock closed this Mar 5, 2024
@spwoodcock
Copy link
Member

Apologies, I stupidly closed this as it doesn't actually impact anything.
I was thinking this was for login_required, but it's actually on the /auth/me endpoint, so is absolutely fine to call the db (as we already do this anyway).

@spwoodcock spwoodcock reopened this Mar 6, 2024
@spwoodcock spwoodcock merged commit 3d03ef1 into development Mar 6, 2024
6 of 7 checks passed
@spwoodcock spwoodcock deleted the fix-me-endpoint branch March 6, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants