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(). Hope this helps. --Karen Sent from my phone. On Apr 18, 2012, at 2:04, Nirmal Agarwal <[email protected]> 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 >>>>> [email protected] >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>> >> > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
