search

msupply-foundation / open-msupply

Open mSupply represents our most recent advancement in the Logistics Management Information System (LMIS), expanding on more than two decades of development inherited from the well-established legacy of the original mSupply.
https://msupply.foundation/open-msupply/
Other
20 stars 12 forks source link

Question regarding demographic permissions #4963

Open roxy-dao opened 2 days ago

roxy-dao commented 2 days ago

What went wrong? 😲

If I have edit central data ticked, and now View Assets... I can add indicator but it doesn't show up on my list and the permission toast doesn't give much information about what permission the user doesn't have.

A bit confused about whether the demographics needs the view assets permission?

Also getting a Cannot read properties of undefined (reading '__typename') error

https://github.com/user-attachments/assets/b75809e2-d1f8-4fdd-8fa4-27c7abca5e1a

Expected behaviour 🤔

How to Reproduce 🔨

Steps to reproduce the behaviour:

  1. Tick edit central data permission and untick view asset permission
  2. Go to demographics
  3. Try add new indicator

Your environment 🌱

lache-melvin commented 2 days ago

A few issues here!

  1. Firstly the __typename error

  2. If you don't have view assets permission, you get a permission denied popup trying to edit a vaccine course: Screenshot 2024-10-03 at 11 15 30 AM

  3. No store_id check, so you can view demographics, even if view assets is off for your store, as long as it is on for any store the user has access to... even stores not on this site! https://github.com/msupply-foundation/open-msupply/blob/ef7d01f61170e5d56a712dab9c5b1c11b16a3ab0/server/graphql/demographic/src/lib.rs#L36-L49

  4. Also, view assets doesn't seem a particularly intuitive permission for this feature, and isn't mentioned in the docs 👀

I think 1 & 2 should probably go into 2.3, maybe 3 and 4 can be solved later... but will let triage team decide here 🙏

andreievg commented 2 days ago

Triage, if 3 and 4 is not too much extra work i would suggest doing as part of this issue.

And we agree to other suggestions by Lache

andreievg commented 2 days ago

If you don't have view assets permission, you get a permission denied popup trying to edit a vaccine course:

Target demographic renders no option instead of permission denied

roxy-dao commented 1 day ago

Also don't think view assets is the best permission for demographics...

lache-melvin commented 1 day ago

Whoever does this issue, can they please check demographic indicators aren't used anywhere else? Hoping view assets was just put in in a rush before the edit central data permission existed, and not because demographics need to be queried by non-central admin users?