phorge-arcanist

mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-26 00:32:41 +01:00

History

epriestley 7e9f80971b 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 [<arcanist>/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	2020-07-01 06:37:31 -07:00
..
ArcanistRuntimeHardpointQuery.php	Implement Conduit login prompt behavior as a pure FutureProxy, not a Future-like object	2020-07-01 06:37:31 -07:00

epriestley 7e9f80971b 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 [<arcanist>/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

2020-07-01 06:37:31 -07:00

ArcanistRuntimeHardpointQuery.php

Implement Conduit login prompt behavior as a pure FutureProxy, not a Future-like object

2020-07-01 06:37:31 -07:00