> On April 18, 2017, 8:35 p.m., Oliver Szabo wrote: > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java > > Lines 131 (patched) > > <https://reviews.apache.org/r/58493/diff/1/?file=1693595#file1693595line131> > > > > Do not use inline comments, maybe its better to use private methods > > with names that describe the behaviour.
Hello Oliver, Thank you for the usggestion. I have removed inline comments from the patch. I have also removed the logic to test for "AMBARI.ADMINISTRATOR" as per your suggestion. The new patch is meant for branch-2.5.0, it is named as "AMBARI-20768_branch-2.5_updated.patch" on the Jira. Thank you, Keta - Keta ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58493/#review172211 ----------------------------------------------------------- On April 20, 2017, 11:07 a.m., Keta Patel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58493/ > ----------------------------------------------------------- > > (Updated April 20, 2017, 11:07 a.m.) > > > Review request for Ambari, Di Li, Miklos Gergely, and Oliver Szabo. > > > Bugs: AMBARI-20768 > https://issues.apache.org/jira/browse/AMBARI-20768 > > > Repository: ambari > > > Description > ------- > > A local Ambari user with no cluster roles assigned to it can successfully log > into the Logsearch UI. > > Logsearch service exercises restriction on who can access its UI using a > property "logsearch.roles.allowed". This property is a comma-separated list > of roles to be allowed access to Logsearch UI. This defect deals with the > following 2 issues: > 1. If Logsearch service requires that only certain roles be allowed to access > its UI, then a local Ambari user with no roles must not be allowed to access > the UI. > 2. If some user with privilege to edit the config properties, updates > "logsearch.roles.allowed" by removing the "AMBARI.ADMINISTRATOR" role from > its list, then the Ambari Admins will not be able to access the Logsearch UI. > This violates the Ambari Administrator privilege which must be able to access > all frames of Ambari UI as well as perform all UI operations. > > > DESIRED BEHAVIOR: > ================= > 1. A local user with no role assigned to it, must not be able to access > Logsearch UI. > 2. Ambari Administrators must be always be allowed to access the Logsearch > UI. No user is allowed to revoke this access right of Ambari Administrator > for the Logsearch UI. > > > Diffs > ----- > > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java > e23f0a2 > > > Diff: https://reviews.apache.org/r/58493/diff/2/ > > > Testing > ------- > > The patch *AMBARI-20768.patch* contains the fix for this issue. The fix > involves correction in 2 places in the > LogsearchExternalServerAuthenticationProvider class. > 1. In order to prevent a local user with no cluster roles assigned to it from > logging into Logsearch UI, we return *false*. > 2. We implicitly check whether the user is an Ambari Administrator or not, > thus removing the requirement of having "AMBARI.ADMINISTRATOR" role in the > "logsearch.roles.allowed" property on the UI. Now, even if some user removes > the "AMBARI.ADMINISTRATOR" property from the UI, it will not affect the > Ambari admin's accessibility to the Logsearch UI. Ambari Admins will always > be allowed to login. > > The results of the logsearch tests after applying the patch are shown in the > screenshot "all_tests_successful.png" on the Jira. > > > Thanks, > > Keta Patel > >
