AOM-120: Non-admin users have same privileges as a admin #122

justmesam · 2018-01-24T10:06:37Z

JIRA TICKET NAME:

AOM-120: Non-admin users have same privileges as a admin

SUMMARY:

Non admin users should not be able to start, stop, delete, start all using the add-on manager.

Charpell · 2018-01-24T10:17:46Z

Kindly look into the test that is failing

Annettesunday · 2018-01-24T14:30:10Z

Looks good to me

kodero · 2018-01-24T15:50:54Z

app/js/components/manageApps/ManageApps.jsx

+  fetchCurrentUser(){
+    const apiHelper = new ApiHelper(null);
+    const getUserData = new Promise(function(resolve, reject) {
+      apiHelper.get('/v1/session').then(response => {


How is an error condition from the HTTP call handled, since a promise is created locally? Basically, am talking about an error resolve.

working on it

coveralls · 2018-01-31T08:54:04Z

Pull Request Test Coverage Report for Build 610

34 of 59 (57.63%) changed or added relevant lines in 2 files are covered.
102 unchanged lines in 7 files lost coverage.
Overall coverage increased (+1.8%) to 41.479%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/js/components/manageApps/Addon.jsx	22	31	70.97%
app/js/components/manageApps/ManageApps.jsx	12	28	42.86%

Files with Coverage Reduction	New Missed Lines	%
app/js/components/App.jsx	1	80.0%
app/js/helpers/apiHelper.js	1	86.67%
app/js/components/manageApps/ManageSettings.jsx	3	66.67%
app/js/components/manageApps/SingleAddon.jsx	3	67.27%
app/js/components/common/Header.jsx	7	71.11%
app/js/components/manageApps/Addon.jsx	22	45.11%
app/js/components/manageApps/ManageApps.jsx	65	23.39%

Totals
Change from base Build 607:	1.8%
Covered Lines:	217
Relevant Lines:	473

💛 - Coveralls

Charpell · 2018-01-31T09:02:16Z

app/js/components/manageApps/Addon.jsx

      messageType,
      showMessage } = this.state;
-


Fix indentation, this can also be seen in other areas of your code.

Some codebase fix were done and my PR was not merged then so when i pull rebased this changes auto fixed themselves.

malmike

Looks good to me

patlub · 2018-01-31T10:07:33Z

app/js/components/manageApps/Addon.jsx

+  checkIsAdmin() {
+    this.fetchCurrentUser().then((response) => {
+      const user = response.user;
+      if (user) {


Make the assignment, after the check. In other words put the assignment in the if statement and use response.user in the if

aenkya

Looks good to me

dkayiwa · 2018-02-01T12:38:18Z

app/js/components/manageApps/ManageApps.jsx

+            return userCanManageModules = false;
+          });
+        }
+        if (user.systemId === "admin" || user.display === "admin" || userCanManageModules){


"admin" is just a value, there is no requirement for a system admin to have a system id of any value. It all depends on the System Admin role, irrespective of the user name or system id.

so I should change using the system id ???

Am not talking about using system id. What talking about is that can't an admin be samuel?

yeah, I also took note on that and that why I also opted using also the privileges as a condition on which it will check either of the 3 conditions.

so if one is an admin and the name is different it will check if they have the priviledges.
@dkayiwa

Why do you even have to assume any name called admin?

Or what happens if my name or system id is admin but am not an administrator?

I was checking on the response of the rest and if the user is an admin the system id is listed as admin.

I have added an image of the response.

justmesam · 2018-02-01T13:58:37Z

@dkayiwaI have added the snippet of how the systemid response looks like .

dkayiwa · 2018-02-01T14:00:19Z

If i make a REST call and get one of the patients with name "daniel", should i have my code to have a check for name "daniel"?

justmesam · 2018-02-02T08:38:03Z

Working on it

Annettesunday · 2018-02-05T10:01:06Z

Looks good to me

aenkya

Looks good. Look into some suggested modifications

aenkya · 2018-02-05T10:13:27Z

app/js/components/manageApps/Addon.jsx

+        let userCanManageModules;
+        let userHasPermittedRoles;
+        userPrivileges = userPrivileges.concat(user.privileges);
+        if (user.roles.length > 0){


What happens if the user object has no roles key? Some browsers may throw errors.

Saying if(user.roles & user.roles.length>0) may be better

if I do this it adds a lot of code which will be redundant and also I will be repeating the same check twice since all am checking is if the array has content so it can proceed.

aenkya · 2018-02-05T10:15:51Z

app/js/components/manageApps/Addon.jsx

+            return userHasPermittedRoles = false;
+          });
+        }
+        if (userPrivileges.length > 0) {


if(userPrivileges.length > 0) is equivalent to if(userPrivileges.length)

Boolean values are some form of binary integer values so length 0 evaluates to false

if I do this I will be evaluating if the length not if the array has content,
doing userPrivileges.length will return an integer which is not a good way of checking conditions but doing userPrivileges.length > 0 will be a boolean checking if the threshold of the length required to proceed is true or false

patlub · 2018-02-05T10:02:37Z

app/js/components/manageApps/SingleAddon.jsx

@@ -153,7 +154,7 @@ export default class SingleAddon extends React.Component {
          id="view-icon-wrapper"
        >
          {
-            app.install === false ?
+            app.install === false || isAdmin === false ?
              <Link


use !app.install || !isAdmin

dkayiwa · 2018-02-05T11:54:01Z

app/js/components/manageApps/Addon.jsx

+        if (user.roles.length > 0){
+          const roles = user.roles;
+          const userRoles = roles.find((userRole) => {
+            if (userRole.display === "System Developer" || userRole.display === "Provider" ) {


There is no need to test for system developer or provider roles. All that one needs is the "Manage Modules" prvilege

I tested for roles since the admin user we use for development do not have the list of priviledges so they are being locked out of the system and they cant view the add-ons manager that's why I resulted to using the roles as another way of giving access to developers so this also happens to the admin user on ref-app.
@dkayiwa

Since the user has the System Developer role, they by default assume that privilege. So you do not need to test for the system developer role

I have fixed and stripped giving the Provider the access

justmesam force-pushed the AOM-120 branch from 940ecc6 to 37647ce Compare January 24, 2018 10:12

justmesam force-pushed the AOM-120 branch 2 times, most recently from f9180cf to 218d771 Compare January 24, 2018 13:22

kodero suggested changes Jan 24, 2018

View reviewed changes

justmesam force-pushed the AOM-120 branch from 218d771 to 2dce5a3 Compare January 31, 2018 08:50

justmesam force-pushed the AOM-120 branch from 2dce5a3 to f803217 Compare January 31, 2018 08:56

Charpell reviewed Jan 31, 2018

View reviewed changes

malmike reviewed Jan 31, 2018

View reviewed changes

patlub reviewed Jan 31, 2018

View reviewed changes

justmesam force-pushed the AOM-120 branch from f803217 to 931ff3f Compare January 31, 2018 12:49

aenkya reviewed Jan 31, 2018

View reviewed changes

dkayiwa reviewed Feb 1, 2018

View reviewed changes

justmesam force-pushed the AOM-120 branch 3 times, most recently from 8034a77 to b6c5346 Compare February 5, 2018 09:58

aenkya reviewed Feb 5, 2018

View reviewed changes

patlub reviewed Feb 5, 2018

View reviewed changes

justmesam force-pushed the AOM-120 branch from b6c5346 to 1d2da01 Compare February 5, 2018 10:37

dkayiwa reviewed Feb 5, 2018

View reviewed changes

justmesam force-pushed the AOM-120 branch from 1d2da01 to e4691ee Compare February 5, 2018 14:07

AOM-120: Non-admin users have same privileges as a admin

17b40dd

justmesam force-pushed the AOM-120 branch from e4691ee to 17b40dd Compare February 5, 2018 14:09

dkayiwa merged commit 0a3b60f into openmrs:master Feb 5, 2018

AOM-120: Non-admin users have same privileges as a admin #122

AOM-120: Non-admin users have same privileges as a admin #122

Conversation

justmesam commented Jan 24, 2018

JIRA TICKET NAME:

SUMMARY:

Charpell commented Jan 24, 2018

Annettesunday commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 31, 2018 • edited Loading

Pull Request Test Coverage Report for Build 610

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malmike left a comment • edited Loading

Choose a reason for hiding this comment

patlub Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aenkya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justmesam Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justmesam commented Feb 1, 2018

dkayiwa commented Feb 1, 2018

justmesam commented Feb 2, 2018

Annettesunday commented Feb 5, 2018

aenkya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 31, 2018 •

edited

Loading

malmike left a comment •

edited

Loading

patlub Jan 31, 2018 •

edited

Loading

dkayiwa Feb 1, 2018 •

edited

Loading

justmesam Feb 1, 2018 •

edited

Loading