From 7e9f80971bd646823ff804a5eb0c2f649ae99523 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Jul 2020 05:08:06 -0700 Subject: [PATCH] Implement Conduit login prompt behavior as a pure FutureProxy, not a Future-like object Summary: See PHI1802. Currently, we can't raise a "you must login" error in a generic way at the beginning of a workflow because we don't know if a workflow needs credentials or not. For example, "arc help" does not need credentials but "arc diff" does. Additionally, some actual Conduit calls do not need credentials ("conduit.ping", "conduit.getcapabilities") and others do. Although I'd like to simplify this eventually and move away from anonymous/unauthenticated "arc", this isn't trivial today. It's also possible for third-party code to add authenticated calls to "arc help", etc., so even if we could execute these tests upfront it's not obvious we'd want to. So, for now, we raise "you must login" at runtime, when we receive an authentication error from Conduit. This got implemented for Toolsets in a well-intentioned but not-so-great way somewhere in wilds/experimental, with an "ArcanistConduitCall" that behaves a bit like a future but is not really a future. This implementation made more sense when ConduitEngine was serving as a future engine, and FutureProxy could not rewrite exceptions. After the Toolsets code was first written, ConduitEngine has stopped serving as a future engine (this is now in "HardpointEngine"). Since HardpointEngine needs a real future, this "show the user a login message" code gets bypassed. This results in user-visible raw authentication exceptions on some workflows: ``` [2020-06-30 21:39:53] EXCEPTION: (ConduitClientException) ERR-INVALID-SESSION: Session key is not present. at [/src/conduit/ConduitFuture.php:76] ``` To fix this: - Allow FutureProxy to rewrite exceptions (see D21383). - Implement "ArcanistConduitCall" as a FutureProxy, not a future-like object. - Collapse the mixed-mode future/not-quite-a-future APIs into a single "real future" API. Test Plan: - Created a paste with "echo hi | arc paste --". - Uploaded a file with "arc upload". - Called a raw method with "echo {} | arc call-conduit conduit.ping --". - Invoked hardpoint behavior with "arc branches". - Grepped for calls to either "resolveCall()" method, found none. - Grepped for calls to "newCall()", found none. - Grepped for "ArcanistConduitCall", found no references. Then: - Removed my "~/.arcrc", ran "arc land", got a sensible and human-readable (but currently ugly) exception instead of a raw authentication stack trace. Differential Revision: https://secure.phabricator.com/D21384 --- src/__phutil_library_map__.php | 4 +- ...Call.php => ArcanistConduitCallFuture.php} | 105 ++++-------------- src/conduit/ArcanistConduitEngine.php | 21 +--- src/conduit/ConduitSearchFuture.php | 3 +- src/ref/paste/ArcanistPasteRef.php | 2 +- .../query/ArcanistRuntimeHardpointQuery.php | 3 +- src/upload/ArcanistFileUploader.php | 16 +-- src/workflow/ArcanistCallConduitWorkflow.php | 3 +- src/workflow/ArcanistPasteWorkflow.php | 3 +- src/workflow/ArcanistWorkflow.php | 19 +--- 10 files changed, 45 insertions(+), 134 deletions(-) rename src/conduit/{ArcanistConduitCall.php => ArcanistConduitCallFuture.php} (52%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c988291b..9bd23305 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -126,7 +126,7 @@ phutil_register_library_map(array( 'ArcanistComprehensiveLintEngine' => 'lint/engine/ArcanistComprehensiveLintEngine.php', 'ArcanistConcatenationOperatorXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConcatenationOperatorXHPASTLinterRule.php', 'ArcanistConcatenationOperatorXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConcatenationOperatorXHPASTLinterRuleTestCase.php', - 'ArcanistConduitCall' => 'conduit/ArcanistConduitCall.php', + 'ArcanistConduitCallFuture' => 'conduit/ArcanistConduitCallFuture.php', 'ArcanistConduitEngine' => 'conduit/ArcanistConduitEngine.php', 'ArcanistConduitException' => 'conduit/ArcanistConduitException.php', 'ArcanistConfigOption' => 'config/option/ArcanistConfigOption.php', @@ -1170,7 +1170,7 @@ phutil_register_library_map(array( 'ArcanistComprehensiveLintEngine' => 'ArcanistLintEngine', 'ArcanistConcatenationOperatorXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistConcatenationOperatorXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', - 'ArcanistConduitCall' => 'Phobject', + 'ArcanistConduitCallFuture' => 'FutureProxy', 'ArcanistConduitEngine' => 'Phobject', 'ArcanistConduitException' => 'Exception', 'ArcanistConfigOption' => 'Phobject', diff --git a/src/conduit/ArcanistConduitCall.php b/src/conduit/ArcanistConduitCallFuture.php similarity index 52% rename from src/conduit/ArcanistConduitCall.php rename to src/conduit/ArcanistConduitCallFuture.php index bd25ff19..01d9954d 100644 --- a/src/conduit/ArcanistConduitCall.php +++ b/src/conduit/ArcanistConduitCallFuture.php @@ -1,22 +1,9 @@ key = $key; - return $this; - } - - public function getKey() { - return $this->key; - } public function setEngine(ArcanistConduitEngine $engine) { $this->engine = $engine; @@ -27,71 +14,6 @@ final class ArcanistConduitCall return $this->engine; } - public function setMethod($method) { - $this->method = $method; - return $this; - } - - public function getMethod() { - return $this->method; - } - - public function setParameters(array $parameters) { - $this->parameters = $parameters; - return $this; - } - - public function getParameters() { - return $this->parameters; - } - - private function newFuture() { - if ($this->future) { - throw new Exception( - pht( - 'Call has previously generated a future. Create a '. - 'new call object for each API method invocation.')); - } - - $method = $this->getMethod(); - $parameters = $this->getParameters(); - $future = $this->getEngine()->newFuture($this); - $this->future = $future; - - return $this->future; - } - - public function resolve() { - if (!$this->future) { - $this->newFuture(); - } - - return $this->resolveFuture(); - } - - private function resolveFuture() { - $future = $this->future; - - try { - $result = $future->resolve(); - } catch (ConduitClientException $ex) { - switch ($ex->getErrorCode()) { - case 'ERR-INVALID-SESSION': - if (!$this->getEngine()->getConduitToken()) { - $this->raiseLoginRequired(); - } - break; - case 'ERR-INVALID-AUTH': - $this->raiseInvalidAuth(); - break; - } - - throw $ex; - } - - return $result; - } - private function raiseLoginRequired() { $conduit_uri = $this->getEngine()->getConduitURI(); $conduit_uri = new PhutilURI($conduit_uri); @@ -119,7 +41,7 @@ final class ArcanistConduitCall " $ arc install-certificate %s\n", $conduit_uri)); - throw new ArcanistUsageException($block->drawConsoleString()); + throw new PhutilArgumentUsageException($block->drawConsoleString()); } private function raiseInvalidAuth() { @@ -147,7 +69,26 @@ final class ArcanistConduitCall " $ arc install-certificate %s\n", $conduit_uri)); - throw new ArcanistUsageException($block->drawConsoleString()); + throw new PhutilArgumentUsageException($block->drawConsoleString()); + } + + protected function didReceiveResult($result) { + return $result; + } + + protected function didReceiveException($exception) { + switch ($exception->getErrorCode()) { + case 'ERR-INVALID-SESSION': + if (!$this->getEngine()->getConduitToken()) { + $this->raiseLoginRequired(); + } + break; + case 'ERR-INVALID-AUTH': + $this->raiseInvalidAuth(); + break; + } + + throw $exception; } } diff --git a/src/conduit/ArcanistConduitEngine.php b/src/conduit/ArcanistConduitEngine.php index c9171245..cd2b411e 100644 --- a/src/conduit/ArcanistConduitEngine.php +++ b/src/conduit/ArcanistConduitEngine.php @@ -39,28 +39,17 @@ final class ArcanistConduitEngine return $this->conduitTimeout; } - public function newCall($method, array $parameters) { + public function newFuture($method, array $parameters) { if ($this->conduitURI == null && $this->client === null) { $this->raiseURIException(); } - return id(new ArcanistConduitCall()) - ->setEngine($this) - ->setMethod($method) - ->setParameters($parameters); - } - - public function resolveCall($method, array $parameters) { - return $this->newCall($method, $parameters)->resolve(); - } - - public function newFuture(ArcanistConduitCall $call) { - $method = $call->getMethod(); - $parameters = $call->getParameters(); - $future = $this->getClient()->callMethod($method, $parameters); - return $future; + $call_future = id(new ArcanistConduitCallFuture($future)) + ->setEngine($this); + + return $call_future; } private function getClient() { diff --git a/src/conduit/ConduitSearchFuture.php b/src/conduit/ConduitSearchFuture.php index 03c394d9..f4d9564e 100644 --- a/src/conduit/ConduitSearchFuture.php +++ b/src/conduit/ConduitSearchFuture.php @@ -104,8 +104,7 @@ final class ConduitSearchFuture $parameters['after'] = (string)$this->cursor; } - $conduit_call = $engine->newCall($method, $parameters); - $conduit_future = $engine->newFuture($conduit_call); + $conduit_future = $engine->newFuture($method, $parameters); return $conduit_future; } diff --git a/src/ref/paste/ArcanistPasteRef.php b/src/ref/paste/ArcanistPasteRef.php index bfeaa77b..d987656d 100644 --- a/src/ref/paste/ArcanistPasteRef.php +++ b/src/ref/paste/ArcanistPasteRef.php @@ -48,7 +48,7 @@ final class ArcanistPasteRef protected function buildRefView(ArcanistRefView $view) { $view ->setObjectName($this->getMonogram()) - ->setTitle($this->getName()); + ->setTitle($this->getTitle()); } } diff --git a/src/toolset/query/ArcanistRuntimeHardpointQuery.php b/src/toolset/query/ArcanistRuntimeHardpointQuery.php index aec1cd32..ed824da2 100644 --- a/src/toolset/query/ArcanistRuntimeHardpointQuery.php +++ b/src/toolset/query/ArcanistRuntimeHardpointQuery.php @@ -77,8 +77,7 @@ abstract class ArcanistRuntimeHardpointQuery $conduit_engine = $this->getRuntime() ->getConduitEngine(); - $call_object = $conduit_engine->newCall($method, $parameters); - $call_future = $conduit_engine->newFuture($call_object); + $call_future = $conduit_engine->newFuture($method, $parameters); return $call_future; } diff --git a/src/upload/ArcanistFileUploader.php b/src/upload/ArcanistFileUploader.php index 5e1841f4..cde462e7 100644 --- a/src/upload/ArcanistFileUploader.php +++ b/src/upload/ArcanistFileUploader.php @@ -118,11 +118,7 @@ final class ArcanistFileUploader extends Phobject { $params['deleteAfterEpoch'] = $delete_after; } - // TOOLSETS: This should be a real future, but ConduitEngine and - // ConduitCall are currently built oddly and return pretend futures. - - $futures[$key] = new ImmediateFuture( - $conduit->resolveCall('file.allocate', $params)); + $futures[$key] = $conduit->newFuture('file.allocate', $params); } $iterator = id(new FutureIterator($futures))->limit(4); @@ -217,11 +213,12 @@ final class ArcanistFileUploader extends Phobject { private function uploadChunks(ArcanistFileDataRef $file, $file_phid) { $conduit = $this->conduitEngine; - $chunks = $conduit->resolveCall( + $future = $conduit->newFuture( 'file.querychunks', array( 'filePHID' => $file_phid, )); + $chunks = $future->resolve(); $remaining = array(); foreach ($chunks as $chunk) { @@ -258,7 +255,7 @@ final class ArcanistFileUploader extends Phobject { foreach ($remaining as $chunk) { $data = $file->readBytes($chunk['byteStart'], $chunk['byteEnd']); - $conduit->resolveCall( + $future = $conduit->newFuture( 'file.uploadchunk', array( 'filePHID' => $file_phid, @@ -266,6 +263,7 @@ final class ArcanistFileUploader extends Phobject { 'dataEncoding' => 'base64', 'data' => base64_encode($data), )); + $future->resolve(); $progress->update(1); } @@ -282,11 +280,13 @@ final class ArcanistFileUploader extends Phobject { $data = $file->readBytes(0, $file->getByteSize()); - return $conduit->resolveCall( + $future = $conduit->newFuture( 'file.upload', $this->getUploadParameters($file) + array( 'data_base64' => base64_encode($data), )); + + return $future->resolve(); } diff --git a/src/workflow/ArcanistCallConduitWorkflow.php b/src/workflow/ArcanistCallConduitWorkflow.php index f5b302ae..025be3e1 100644 --- a/src/workflow/ArcanistCallConduitWorkflow.php +++ b/src/workflow/ArcanistCallConduitWorkflow.php @@ -53,8 +53,7 @@ EOTEXT } $engine = $this->getConduitEngine(); - $conduit_call = $engine->newCall($method, $params); - $conduit_future = $engine->newFuture($conduit_call); + $conduit_future = $engine->newFuture($method, $params); $error = null; $error_message = null; diff --git a/src/workflow/ArcanistPasteWorkflow.php b/src/workflow/ArcanistPasteWorkflow.php index f9e2a693..c447cf29 100644 --- a/src/workflow/ArcanistPasteWorkflow.php +++ b/src/workflow/ArcanistPasteWorkflow.php @@ -141,8 +141,7 @@ EOTEXT ); $conduit_engine = $this->getConduitEngine(); - $conduit_call = $conduit_engine->newCall($method, $parameters); - $conduit_future = $conduit_engine->newFuture($conduit_call); + $conduit_future = $conduit_engine->newFuture($method, $parameters); $result = $conduit_future->resolve(); $paste_phid = idxv($result, array('object', 'phid')); diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php index c6098ddb..edad2d8d 100644 --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -1808,22 +1808,6 @@ abstract class ArcanistWorkflow extends Phobject { return $parser; } - final protected function resolveCall(ConduitFuture $method) { - try { - return $method->resolve(); - } catch (ConduitClientException $ex) { - if ($ex->getErrorCode() == 'ERR-CONDUIT-CALL') { - echo phutil_console_wrap( - pht( - 'This feature requires a newer version of Phabricator. Please '. - 'update it using these instructions: %s', - 'https://secure.phabricator.com/book/phabricator/article/'. - 'upgrading/')."\n\n"); - } - throw $ex; - } - } - final protected function dispatchEvent($type, array $data) { $data += array( 'workflow' => $this, @@ -1964,7 +1948,8 @@ abstract class ArcanistWorkflow extends Phobject { try { $method = 'repository.query'; - $results = $this->getConduitEngine()->newCall($method, $query) + $results = $this->getConduitEngine() + ->newFuture($method, $query) ->resolve(); } catch (ConduitClientException $ex) { if ($ex->getErrorCode() == 'ERR-CONDUIT-CALL') {