UTAH

Merge lp://staging/~nuclearbob/utah/bug1073617 into lp://staging/utah

bug1073617
Merge into dev

Proposed by Max Brustkern on 2012-11-20

Status:

Work in progress

Proposed branch:

lp://staging/~nuclearbob/utah/bug1073617

Merge into:

lp://staging/utah

Diff against target:

48 lines (+18/-4)

2 files modified

utah/client/common.py (+4/-4)
utah/client/tests/test_common.py (+14/-0)

To merge this branch:

bzr merge lp://staging/~nuclearbob/utah/bug1073617

Medium

Triaged

Link a bug report

Reviewer	Date Requested	Status
Max Brustkern (community)		Needs Fixing on 2012-12-11
Javier Collado (community)		Needs Fixing on 2012-12-11
Joe Talbott (community)	2012-11-20	Needs Information on 2012-11-21
Review via email: mp+135239@code.staging.launchpad.net

Description of the change

This branch adds -i to the sudo command used to execute commands using run_as. This preserves the environment of the intended user.

A test for this environment preservation is also added, and an existing test that was broken by this change is updated to work.

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2012-11-20:

This is intended to fix this:
https://bugs.launchpad.net/utah/+bug/1073617

Revision history for this message

Joe Talbott (joetalbott) wrote on 2012-11-21:

Your fix for the existing test reveals a problem. I think -i will only work for users with a valid shell. We will either need to fallback to non -i if the user doesn't have a valid shell or explicitly forbid using non-shell users with 'run_as'.

review: Needs Information

Revision history for this message

Joe Talbott (joetalbott) wrote on 2012-11-21:

"sudo su - nobody -c 'ls'" might be another alternative that doesn't require a valid shell.

lp://staging/~nuclearbob/utah/bug1073617 updated on 2012-12-10

757. By Max Brustkern on 2012-12-10: Merged latest changes from dev branch
758. By Max Brustkern on 2012-12-10: Changed manner of privilege switching so all tests pass, including the old test reverted back to the nobody user

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2012-12-10:

I've implemented that solution and it seems to test okay.

review: Needs Resubmitting

Revision history for this message

Javier Collado (javier.collado) wrote on 2012-12-11:

When running the test cases, I get a couple of errors that I don't get when
running the test cases from trunk. The affected test cases are:
- test_load_state_partial
- test_load_state_partial_run_all

Joe, by the way, since there are a few test cases with the same docstring, it
isn't straightforward to figure out the method for the test case that failed.

Aside from that, please escape curly braces doubling them. For example, instead
of this:
cmd_wrapper = "sudo su - {} -c '{}'".format(run_as, '{}')

use this:
cmd_wrapper = "sudo su - {} -c '{{}}'".format(run_as)

review: Needs Fixing

lp://staging/~nuclearbob/utah/bug1073617 updated on 2012-12-11

759. By Max Brustkern on 2012-12-11: Fixed formatting per Javier's suggestion

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2012-12-11:

I fixed the formatting. The problem with the test cases is that when we switch to the user nobody, we're losing the directory we're currently in, so commands that are supposed to run relative to that directory are failing. I'm not sure of the best way to get around that. I tried prepending a cd to the cwd passed into run_cmd, but that doesn't seem to be helping.

review: Needs Fixing

Unmerged revisions

759. By Max Brustkern on 2012-12-11: Fixed formatting per Javier's suggestion
758. By Max Brustkern on 2012-12-10: Changed manner of privilege switching so all tests pass, including the old test reverted back to the nobody user
757. By Max Brustkern on 2012-12-10: Merged latest changes from dev branch
756. By Max Brustkern on 2012-11-20: Added test for run_as environment and updated existing test to work with environment setting
755. By Max Brustkern on 2012-11-20: Added -i option to sudo to get user's environment