Require Commandeering for `arc commit`
Closed, InvalidPublic
Actions

Assigned To

None

Authored By

	sirbrillig
	May 1 2017, 4:32 PM

Description

Updating a differential using arc diff --update requires that the user be the owner of that differential. If a user is not the owner, it's necessary to Commandeer the differential in order to make the update. When using arc commit --revision XXXX (with svn) to commit a differential revision, however, there is no requirement that the user is the owner. The result is that a simple typo in the differential ID can commit the wrong message and update and close the differential with incorrect data.

It seems to me that the simple fix would be to make the ownership requirement standard across the different arc actions. I think it would not be an overly difficult requirement to only be able to commit diffs you own, just as with updates. This would make it an explicit action to commit a diff you did not own and help prevent users from accidentally committed the wrong diff (at least someone else's).

Event Timeline

sirbrillig created this task.May 1 2017, 4:32 PM

It's very common to land other people's diffs (who do not have access to the repository) and comandeering changes the commit author.

The expectation is that you should get this prompt when trying to arc commit a change you did not author:

if ($revision['authorPHID'] != $this->getUserPHID()) {
  $confirm[] = pht(
    "You are not the author of '%s: %s'. Commit this revision anyway?",
    "D{$revision_id}",
    $revision_title);
}

Are you not seeing that? Or are you seeing that, but your experience is that it's not sufficient?

That pht seems to have too many %s ?

I think it's right:

The first %s is D1234.
The second %s is Fix flibs in the flobs.
The overall string is 'D1234: Fix flibs in the flobs'.

In modern code we'd probably write that a little differently (e.g., I've tried to move to double quotes in user-facing strings) but I don't think the pattern is mismatched.

Oh I was thrown off by the quotes and not a var.

The warning seems to be working properly for me locally, so I can't reproduce the issue if the claim is that it doesn't work:

epriestley@orbital ~/dev/scratch/svn-test $ arc commit --conduit-uri=http://local.phacility.com/ --revision D224


    You are not the author of 'D224: asdf'. Commit this revision anyway?
    [y/N]

You didn't include version information, but this warning was introduced in D1055 (October, 2011) so even very old versions of arc should include it.

Oh! So it already has this feature! I wonder if perhaps we are on an old version. I tried arc version as specified here, but I just get an error:

Exception
Command failed with error #1!
COMMAND
svn log -1 --format='%H %ct'

STDOUT
(empty)

STDERR
svn: invalid option character: 1
Type 'svn help' for usage.

In any case, this can be marked closed for now since it should be working (I don't seem to have the ability).

What does git show show in arcanist/?

Expected output is similar to this:

arcanist/ $ git show
commit dc65bfbe5434325b15eda6dfb4ca007a8b5488fa
Author: epriestley <git@epriestley.com>
Date:   Tue Feb 7 07:09:23 2017 -0800
...

itsamystery

Hm... the way we our systems folks have it installed it's apparently not a git repository. However, I was able to look in the src directory and found those lines in workflow/ArcanistCommitWorkflow.php. I'm checking with the person who ran the command that prompted me to make this feature request.

Doesn't sound like there is anything the upstream can do here.

Require Commandeering for `arc commit`Closed, InvalidPublicActions

Description

Event Timeline

Require Commandeering for `arc commit`
Closed, InvalidPublic
Actions