Hi Nirmal,
Here are my comments:
General comments and questions:
-------------------------------
- The PSARC case says this in the materials:
> The System Administrator profile will be updated to include the the
> Install Service Management profile, as setting up a system to be
> an install server should be a task the sys admin should be able to
> carry out.
I assume the above means that we want to add the "Install Service
Management"
profile to the System Administrator profile if the
install-installadm package is installed on the system. If that's the case,
I do not see the corresponding prof_attr entry to add that.
- In the manual testing done, you listed the list of commands options
you run. Can you give more details as to what users you run those
commands as? Also, what profile combinations those users have, and what
results were
expected...etc..
Specific comments:
------------------
usr/src/cmd/installadm/installadm_common.py:
- line 207: I do not think this is a precise description of what this
function is doing. I think it's a sufficient to say that
this function "Checks whether the user has the specified authorization".
There's no need to mention "perform particular operation", because there's
no check on the "operation" being performed.
- line 208: the operation described in this sentence is not done.
- line 210: I think it's kinda harsh to just exit the whole app. I supposed
that's probably the most common action to take for most callers, but some
other programs might just want to check the authorization, and make
decisions
based on whether the auth exists or not. I think it would
make the function better for others to use in the future if it will either
throw an exception or exit, based on the user's input. Perhaps, we can
make the default action to be exit.
- line 217: So, you are allowing users with euid==0 to "pass" even though
the user doesn't have the authorization? Is that desired? That's certainly
not documented.
- line 218: I think it would be useful to display the authorization that's
missing.
- line 214-220: I think it would make the implementation more "python-like"
if you just use the run() function from solaris_install to run the command,
and catch the CalledProcessError. Then, based on what the user
specified, you can exit or just re-raise the error to the user.
- lines 205-239: The check_auth() function and SetUIDasEUID context manager
sounds like something that can be used by other install components.
I think it's better to define these in
usr/src/lib/install_common/__init__.py
Thanks,
--Karen
On 04/11/12 03:41 AM, Nirmal Agarwal wrote:
Hi all
Can I please get the code review for CR 7108281
7108281 create RBAC execution profiles for installadm
Webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev0/webrev/
<https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev0/webrev/;jsessionid=9662DF16CB545E8304942C4DC3F2DA3A>
As a part of this CR, following authorizations for installadm will be
introduced.
1) solaris.installadm.client:RO::Administer Automated Install Clients::
2)solaris.installadm.manifest:RO::Administer Automated Install Manifests::
3)solaris.installadm.profile:RO::Administer System Configuration
Profiles::
4)solaris.installadm.service:RO::Administer Automated Install Services::
Testing :
Pep8 clean
Slim test results :
/net/indiana-build.us.oracle.com/export/home/na210770/ai/rbac/slim-test
Manual Testing :
Below commands were tested :
--> create-service
--> create-service -n /export/home/test2 -i 192.168.56.4 -c 2
-n test2
--> create-service -d /export/home/test3 -i 192.168.56.7 -c 2
-n test3 -s /var/tmp/sol-11_1-12-ai-sparc.iso
--> create-service -t test3 -n test4
--> create-manifest -m test5 -n test4 -f
/usr/share/auto_install/manifest/default.xml
--> set-criteria -m test5 -n test4 -c hostname="test.com"
--> update-manifest
--> delete-manifest
--> set-criteria -p
--> create-profile
--> delete-profile
--> create-client
--> delete-client
--> disable (service)
--> enable (service)
PSARC detail : PSARC/2012/139 FastTrack ( under review)
Ethan : Thanks for all the guidance
Thanks
Nirmal
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
