Hi Karen
Please find the updated webrev.
webrev rev 4 :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev4/webrev.rev4/
webrev rev 4 diff with rev 3 :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev4-diff/webrev.rev4-diff/
Only change is check_auth -> check_auth_and_euid as suggested.
Thanks
Nirmal
On 04/20/12 11:22, Nirmal Agarwal wrote:
Hi Karen
Thanks for the review.
On 04/20/12 11:29, Karen Tung wrote:
Hi Nirmal,
You mentioned below that you use check_auth() as the function name
because you consider having euid==0 and the specified auth as having
the complete authorization when running the command using pfexec. The
authorization and euid value are 2 different things, even though they
happened to both be assigned to the installadm command for these
profiles. Just like when you issue the "auths" command, it only checks
the authorization, it does not check for other things that can be
assigned to profiles, such as privileges, uid, euid.
So, for clarity, I think you can either break the checking of
authorization and euid into different functions. Alternatively, you
can leave everything the way it is and rename the check_auth()
function into check_auth_and_euid().
I will change the function name to check_auth_and_euid() as suggested.
Do you need updated webrev ?
Regards
Nirmal
Hope this helps.
--Karen
Sent from my phone.
On Apr 18, 2012, at 2:04, Nirmal Agarwal<nirmal.agar...@oracle.com>
wrote:
Hi Karen
Thanks for the review.
webrev rev 2
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev2/webrev.rev2/
webrev diff with rev 1
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev2-diff/webrev.diff-rev1/
Please find my reply inline.
On 04/18/12 11:41, Karen Tung wrote:
Hi Nirmal,
I have a few comments:
usr/src/lib/install_common/__init__.py.src:
- line 114: I think it is more accurate for this comment of this
function
to be: "exception raised when the user does not have a specified
authorization".
fixed.
- line 115: Can we pass in the authorization that is checked as an
argument
in the __init__() function? That way, the string that is constructed
can include that information.
fixed.
- line 543-545: This name of this function is check_auth(), and even
the
comment
of the function says it will check for whether the user has the
specified authorization.
I am surprised that see that in addition to the authorization, the euid
is also checked.
If it's intentional for the euid to be also checked, I think the
name of
the function as well
as the comment should be updated to reflect what exactly it is doing.
check for euid is intentional and to make sure that authorization is
active. This only happens when the user is running the command with
"pfexec". So in my understanding the complete check for authorization
should include whether it is active in current execution or not.
Please advice.
Testing:
-----------
The detail manual testing file is very useful for me to understand what
tests are run.
Did you confirm that you can do an AI install successfully using a
service that's created by a user
that's assigned these profiles?
Yes I verified the installation using one of the services created
using these profiles.
Thanks
Nirmal
Thanks,
--Karen
On 04/17/12 10:33 AM, Nirmal Agarwal wrote:
Hi Sue, Karen
Thanks for review,
I have fixed comments provided.
Please find the updated webrev :
diff webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev1-diff/webrev.diff/
webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7108281-rev1/webrev/
Slim-test results :
/net/indiana-build.us.oracle.com/export/home/na210770/ai/rbac/slim-test
Detailed Manual tests :
/net/indiana-build.us.oracle.com/export/home/na210770/ai/rbac/manual-testing
Pep8 is clean.
Ran pylint and removed "unused imports".
Test cases : I will file a separate CR and write test case for
functions/class introduced.
Let me know if I need to run other tests.
Thanks
Nirmal
On 04/12/12 04:24, Sue Sohn wrote:
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>
Hi Nirmal,
In your manual testing, did you test installadm-convert?? I don't
see it
listed.
Same question for validate_profile and export.
Also, did you test the commands with both with a profile with proper
privileges and one that didn't have them?
installadm_common.py
205 I think it would be better to raise some other exception from
check_auth and then have the various subcommands do the try/except
and
raise the SystemExit if desired.
208 I'm not seeing where this is done?
205 and/or 226: Is it possible to add a unit test for this in
test_installadm_common.py?
test_create_profile.py
47 alphabetize import
221 typo: "do noting"
503 should this be reverted in the tearDown method?
create_client.py
37 this line should be moved back to where it was as it is an
internal
module
delete_client.py
28 Don't think you need to import os anymore
delete_service.py
28 same here
Thanks,
Sue
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
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
