recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Treat invalid policies as broadly similar to "no one"

Summary:
Ref T3903. Ref T603. We currently overreact to invalid policies. Instead:

- For non-omnipotent users, just reject the viewer.
- For omnipotent users, we already shortcircuit and permit the viewer.
- Formalize and add test coverage for these behaviors.

Also clean up some strings.

The practical effect of this is that setting an object to an invalid policy (either intentionally or accidentally) doesn't break callers who are querying it.

Test Plan:
- Created a Legalpad document and set view policy to "asldkfnaslkdfna".
- Verified this policy behaved as though it were "no one".
- Added, executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603, T3903

Differential Revision: https://secure.phabricator.com/D7185

epriestley 4dfdd0d3 ca85c457

+58 -8
+40 -1
src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
··· 82 82 'admin' => false, 83 83 ), 84 84 'No One Policy'); 85 - 86 85 } 87 86 88 87 ··· 160 159 public function testOmnipotence() { 161 160 $results = array( 162 161 $this->buildObject(PhabricatorPolicies::POLICY_NOONE), 162 + ); 163 + 164 + $query = new PhabricatorPolicyAwareTestQuery(); 165 + $query->setResults($results); 166 + $query->setViewer(PhabricatorUser::getOmnipotentUser()); 167 + 168 + $this->assertEqual( 169 + 1, 170 + count($query->execute())); 171 + } 172 + 173 + 174 + /** 175 + * Test that invalid policies reject viewers of all types. 176 + */ 177 + public function testRejectInvalidPolicy() { 178 + $invalid_policy = "the duck goes quack"; 179 + $object = $this->buildObject($invalid_policy); 180 + 181 + $this->expectVisibility( 182 + $object = $this->buildObject($invalid_policy), 183 + array( 184 + 'public' => false, 185 + 'user' => false, 186 + 'admin' => false, 187 + ), 188 + 'Invalid Policy'); 189 + } 190 + 191 + 192 + /** 193 + * An omnipotent user should be able to see even objects with invalid 194 + * policies. 195 + */ 196 + public function testInvalidPolicyVisibleByOmnipotentUser() { 197 + $invalid_policy = "the cow goes moo"; 198 + $object = $this->buildObject($invalid_policy); 199 + 200 + $results = array( 201 + $object, 163 202 ); 164 203 165 204 $query = new PhabricatorPolicyAwareTestQuery();
+18 -7
src/applications/policy/filter/PhabricatorPolicyFilter.php
··· 225 225 $this->rejectObject($object, $policy, $capability); 226 226 } 227 227 } else { 228 - throw new Exception("Object has unknown policy '{$policy}'!"); 228 + // Reject objects with unknown policies. 229 + $this->rejectObject($object, false, $capability); 229 230 } 230 231 } 231 232 ··· 241 242 return; 242 243 } 243 244 244 - // TODO: clean this up 245 - $verb = $capability; 245 + switch ($capability) { 246 + case PhabricatorPolicyCapability::CAN_VIEW: 247 + $message = pht("This object has an impossible view policy."); 248 + break; 249 + case PhabricatorPolicyCapability::CAN_EDIT: 250 + $message = pht("This object has an impossible edit policy."); 251 + break; 252 + case PhabricatorPolicyCapability::CAN_JOIN: 253 + $message = pht("This object has an impossible join policy."); 254 + break; 255 + default: 256 + $message = pht("This object has an impossible policy."); 257 + break; 258 + } 246 259 247 - throw new PhabricatorPolicyException( 248 - "This object has an impossible {$verb} policy."); 260 + throw new PhabricatorPolicyException($message); 249 261 } 250 262 251 263 public function rejectObject( ··· 350 362 $handle->getFullName()); 351 363 break; 352 364 } 353 - 354 365 } else { 355 - $who = pht("This object has an unknown policy setting."); 366 + $who = pht("This object has an unknown or invalid policy setting."); 356 367 } 357 368 break; 358 369 }