Is there a way that we could rename "View *" privileges in the API and survive it? I could easily see how a global search & replace would work for the core code, but I'm not sure what the ramifications might be for modules trying to span this change. Is there a way we could ease their pain?
The thing I don't like about page-specific privileges as a convention is that it promotes making multiple privileges for the same thing, which would mean exchanging the current problem for another (i.e., we don't use privileges because a single privilege is used to mean two things → we don't use privileges because there are too many privileges to assign to do one thing). -Burke On Thu, May 10, 2012 at 9:53 AM, Mark Goodrich <[email protected]> wrote: > +1 for renaming “View” to “Access” (or perhaps even “Get”?), as “viewing” > really makes no sense in the context of an API and confuses the issue. I > think it would make sense to have the API level privileges mimic the names > of the underlying methods as much as possible. I’d also suggest perhaps > starting the privilege with the name of the domain object and/or service, > like “Concept – Access”. I’ve always found it annoying when I’ve had to > search through the alphabetized list of privileges looking for all the > privileges that apply to a certain domain object.**** > > ** ** > > I’m not quite sure where I fall on naming privileges after pages… but I > think I’m in favor of it. I see Burke’s point, but organizing it via page > allows us a fairly detailed level of granularity which could be valuable. > You might want to a typical user to see one allergy list but not another… I > guess the issue here is that we are using “privileges” as really a means of > customizing the site based on role, rather than to define an actual > privilege.**** > > ** ** > > Mark **** > > ** ** > > *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Burke > Mamlin > *Sent:* Thursday, May 10, 2012 8:45 AM > > *To:* [email protected] > *Subject:* Re: [OPENMRS-DEV] Roles and Privileges Sprint**** > > ** ** > > I don't think privileges should be named after pages; rather, they should > be named according to application behavior (i.e., function). If we adopt a > "Page Name - View Xyz Section" convention, then we'll end up with a whole > bunch of unnecessary privileges. For example, if there are fourteen pages > between core & modules that present a list of allergies, then we don't want > to have to manage 14 privileges to see allergies; rather, all of the pages > should use a single privilege that is specific to displaying allergies to > the user.**** > > ** ** > > The fundamental problem is that we chose "View" in our API privileges. > How about getting the list of all "View Xyz" privileges and doing a global > search & replace like s/View (Abc|Def|Ghi|…|Xyz)/Access $1/g. Seems like > a decent task for a roles & privileges Sprint. Then we can use "View > Patient" for, well, *viewing* a patient, no matter which page or which > context.**** > > ** ** > > -Burke**** > > On Thu, May 10, 2012 at 2:49 AM, Darius Jazayeri <[email protected]> > wrote:**** > > This makes perfect sense. And I agree that we can handle this merely by > using a naming convention. For sake of not changing the names of the bulk > of existing privileges (and privilege.name is the pk of that table), I > propose that we go with:**** > > ** ** > > (api-level privileges, stick with the same naming)**** > > - View Xyz (for metadata and data)**** > - Manage Xyz (for metadata)**** > - Add Xyz, Edit Xyz, Delete Xyz, Purge Xyz (for data)**** > > (application-level privileges, follow the pattern from > patientDashboardForm.jsp)**** > > - Page Name - View Xyz Section**** > > ** ** > > (Ironically patientDashboardForm.jsp is the *only* jsp page I'm aware of > where we've actually done this right. So yes, good choice of example. :-) ) > **** > > ** ** > > So, I guess the proper solution is to look at all of our jsp and jsp-like > files (.tag, and anything else?) for <openmrs:hasPrivilege, and identify > every one of those in core (and ideally also bundled modules) that uses an > API-level privilege. (This will be 90% of them.)**** > > ** ** > > Then, for this list of privilege checks, we need to:**** > > 1. create a new, matching application-layer privilege**** > > 2. switch the jsp to check against the new privilege**** > > 3. write a liquibase changeset that will grant the new application-level > privilege to every role that has the old api-level privilege.**** > > ** ** > > -Darius**** > > ** ** > > On Wed, May 9, 2012 at 11:16 PM, Dave Thomas <[email protected]> wrote:*** > * > > First I apologise for using the dashboard as an example -- it was > hypothetical, and therefore crappy. > > And yes, your suggestion is exactly right, but it's a pattern that I think > should be applied globally, and not just to the patient dashboard > components. > > Here's another example: 'View Concepts' is required by gutter.jsp to show > the Dictionary gutter item. There's no reason why our data officers or lab > technicians need to be looking up concepts through the UI. They just need > to be able to fill out forms and look at patient records for data quality. > We've also had instances where research staff have looked for Concepts > associated with specific forms without really knowing the correct way to do > this, which resulted in bad data being exported. Without access to the > Dictionary, they would have been forced to ask us for help. > > So there are a couple of groups of people for whom i'd like to hide the > Dictionary gutter item. > > But if you take away 'View Concepts' for a user, this basically breaks all > of OpenMRS for that user. More than 1/2 the ConceptService requires this > privilege. > > This is kind of an extreme example in terms of the functionality that > would be lost by removing the privilege. I feel like we've run into more > subtle 'accidental' restrictions mistakes that might only manifest > themselves in 1 or 2 unexpected places, sometimes in a module, or maybe in > the various reporting mechanisms we've used. > > What i think would be a good step forward would be a complete separation > between privileges used by the app layer and privileges used by the api, > which I think could be handled through naming conventions for privileges. > This doesn't require any new architecture, but would greatly increase the > granularity of what you can restrict and for whom. Privileges used > concurrently in app and api layers are too blunt of an instrument, and lead > to (sometimes strikingly) sub-optimal role configurations. > > I know that increased granularity seems like it would lead to greater > complexity for implementations, but i'd argue that at least in our > experience its the opposite. > > Does this make sense? > > d**** > > ** ** > > On Wed, May 9, 2012 at 5:24 PM, Darius Jazayeri <[email protected]> > wrote:**** > > All dashboard tabs *do* have application-level privileges already. At > least in 1.9.x, patientDashboardForm.jsp has:**** > > <openmrs:hasPrivilege privilege="Patient Dashboard - View Overview > Section">**** > > <openmrs:hasPrivilege privilege="Patient Dashboard - View Regimen Section"> > **** > > <openmrs:hasPrivilege privilege="Patient Dashboard - View Visits Section"> > **** > > <openmrs:hasPrivilege privilege="Patient Dashboard - View Encounters > Section">**** > > <openmrs:hasPrivilege privilege="Patient Dashboard - View Demographics > Section">**** > > <openmrs:hasPrivilege privilege="Patient Dashboard - View Graphs Section"> > **** > > <openmrs:hasPrivilege privilege="Form Entry">**** > > ** ** > > However looking at patientOverview.jsp I see we refer to the underlying > API-level privileges:**** > > <openmrs:hasPrivilege privilege="View Patient Programs">**** > > <openmrs:hasPrivilege privilege="View Relationships">**** > > <openmrs:hasPrivilege privilege="View Allergies">**** > > <openmrs:hasPrivilege privilege="View Problems">**** > > ** ** > > Seems like we should write a ticket to:**** > > 1. introduce new privileges like "Patient Overview - View Programs > Section", etc**** > 2. use those in all dashboard portlets rather than the API counterparts > **** > 3. do a liquibase changeset so that every role that has "View Patient > Programs" also gets "Patient Overview - View Programs Section", etc. (That > way nobody loses the ability to see something they currently see.)**** > > Dave, would that help?**** > > ** ** > > Daniel, want to do this?**** > > ** ** > > -Darius**** > > ** ** > > On Wed, May 9, 2012 at 2:33 PM, Dave Thomas <[email protected]> wrote:**** > > Burke, sorry for not being clear before. I think what you said in #1 is > pretty much what i'm asking for. > > The terminology can be worked out (to use your example 'Access' could > imply API-level privileges, and 'View' could imply app-level privileges), > but the important thing to me would be to not have 'Access Patient Data' > sometimes be applied at the API level and sometimes applied at the app > level. With 'Access Patient Data' used at both the API and app layers, if > I restrict 'Access Patient Data' for a user, it is incredibly difficult to > tell what i've actually restricted. This is when pieces of the > application, other than the one I was trying to restrict, start throwing > errors unexpectedly (per mark's example). > > Part two of this argument (and maybe this is a different discussion) would > be that once there was a clear distinction between api and app level > privileges, the app level privileges could start to reflect actual UI > components a little more. If each portlet on the patient dashboard had its > own privilege for example, like 'view regimen tab', the UI would be a LOT > more easy to customise in a meaningful way using privileges and roles. > > d**** > > ** ** > > On Wed, May 9, 2012 at 1:43 PM, Burke Mamlin <[email protected]> > wrote:**** > > Mark,**** > > ** ** > > This is an interesting point. Your example helps me understand Dave's > point and suggests that we should distinguish between "Access Patient Data" > and "View Patient Data" – i.e., the use of "View" in the privilege term > describing API-level access to data per our conventions implies that > getting data at the API level also means that the user should be able to > view it within the application, which is not always the case. In your > example (a person being able to view aggregate patient data without viewing > individual patient records), we have two choices:**** > > 1. Grant the user "Access Patient Data" as the API-level privilege, > meaning that they would be permitted to execute API calls that return > patient information, but would not imply that the application should let > them view the data directly. > **** > 2. Allow the methods that need to access patient data to proxy > privileges for the user.**** > > The first seems like the better option to me. Proxying privileges seems > like a hacky way to override privileges that are there for a reason. The > first option will take some effort (maybe more than the upcoming sprint can > muster), but seems like a better long-term solution. Maybe we can come up > with a way to start heading that direction.**** > > ** ** > > -Burke**** > > ** ** > > On Wed, May 9, 2012 at 3:55 PM, Mark Goodrich <[email protected]> wrote:** > ** > > +1 for this. We’ve run into issues with this a lot. For instance, say > you have a user who should not be able to view patients, but should have > access to a report of aggregate patient data that calls getPatients() > behind the scenes to perform the necessary calculations. If you take away > the “view patient” privilege from the user, they’ll get a stack trace when > they attempt to execute the report.**** > > **** > > This issue has been prevalent enough for us that we basically are unable > to use privileges and roles for access control … **** > > **** > > Mark**** > > **** > > **** > > *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Dave > Thomas > *Sent:* Wednesday, May 09, 2012 2:51 PM > *To:* [email protected] > *Subject:* Re: [OPENMRS-DEV] Roles and Privileges Sprint**** > > **** > > If i can weigh in on a discussion of roles and privileges briefly, I've > found (and maybe things are improved in versions greater than 1.6.x) that > we've run into trouble when a privilege is used both at the api and web > layer. This happens when we want to hide something in the UI that is > wrapped in a <privilege/> tag, but then when we remove that privilege for a > user, it turns out that the same privilege was wrapped around a couple of > API methods, causing unexpected and hard-to-track-down UI problems across > all of openmrs. > > If there's a dedicated pass at roles+privileges, has there been any > thought to separating api privileges from ui privileges? I kind of feel > like this is low-hanging fruit. This wouldn't even need re-architecting, > maybe just privilege naming conventions? > > Or is the general sense that this type of problem has disappeared in newer > versions? > > d**** > > On Wed, May 9, 2012 at 10:29 AM, Burke Mamlin <[email protected]> wrote:** > ** > > (bringing this conversation about preparing for the Roles & Privileges > sprint onto the dev list)**** > > **** > > Reviewing the notes <http://notes.openmrs.org/2012-roadmap> on Roles & > Permissions section from Jembi & PIH, it looks like there are some > fundamental improvements requested:**** > > - Ship OpenMRS with pre-defined roles**** > - Better documentation on managing roles (avoiding pitfalls)**** > - More informative handling of privilege exceptions (make it easier to > understand where/which privileges are missing)**** > - Data-level permissions (restricting access to specific data based on > privileges)**** > > We've had prior conversations about improving roles/privileges:**** > > - Avoiding the common pitfall of conflating organizational roles (job > title) with application roles (authorization within OpenMRS); they may > align early on in simple systems, but exceptions are common over time or as > a a system grows.**** > - Creating privilege groups vs. programmatically defined roles – e.g., > a web page wants to limit access to those who have a set of privileges. > **** > - Introducing location-based privileges**** > > There seem to be some potential short-term wins that could be done in the > sprint:**** > > - Improve our documentation to better introduce people to roles & > privileges and cover the common pitfalls.**** > - Improve privilege error messages in core and/or create a module that > makes it easier to troubleshoot privilege errors (e.g., log all privilege > checks during an operation and present the unique list of privileges and/or > roles that would cover the operation, allowing someone to step through a > workflow as superuser and then see the list of privileges required to > complete the workflow).**** > - Come up with some basic application roles that can be pre-defined > within OpenMRS (ship with the application)**** > - Design (and, if possible, implement) an approach for privilege > groups or system roles (i.e., uneditable sets of privileges that > applications can program against)**** > > Data-level privileges (limiting access to data based on privileges) would > be a terrific addition, but I'm afraid it will take more design that we can > muster between now & the beginning of this sprint. Maybe we could come up > with some small but useful first attempts at solving this problem (e.g., a > module requiring permissions to access certain observations … or a module > that limits access to specific patients based on permissions).**** > > **** > > Cheers,**** > > **** > > -Burke**** > > **** > > On Wed, May 9, 2012 at 9:49 AM, Burke Mamlin <[email protected]> wrote:*** > * > > Looking back at notes from AMPATH, the only reference to anything close to > roles & privileges I found was the desire for the Data Entry Statistics > Module to have a basic view privilege that allows a data assistant to see > only his/her own statistics.**** > > **** > > -Burke**** > > **** > > On Wed, May 9, 2012 at 9:44 AM, Ben Wolfe <[email protected]> wrote:**** > > Dawn found this link for me: > http://notes.openmrs.org/2012-roadmap > > Is has the (mostly raw) notes from the calls we had with Jembi/PIH/AMPATH. > > Daniel, can you tease out the topics from that and the other text below in > the next 4 hours? > > Ben**** > > **** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > **** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > **** > _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
