Skip to content

Commit

Permalink
Lock while refreshing OAuth2 tokens or SSH certificates
Browse files Browse the repository at this point in the history
This guards against most of the race condition bugs that occur while running
multiple instances of the CLI in parallel. It doesn't fix all of them.

This behavior can be disabled using the environment variable
`{PREFIX}DISABLE_LOCKS`, or the config entry `api.disable_locks`.
  • Loading branch information
pjcdawkins committed Nov 30, 2023
1 parent 3e0cf9c commit 030b3b7
Show file tree
Hide file tree
Showing 9 changed files with 235 additions and 41 deletions.
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"guzzlehttp/guzzle": "^5.3",
"guzzlehttp/ringphp": "^1.1",
"platformsh/console-form": ">=0.0.37 <2.0",
"platformsh/client": ">=0.77.0 <2.0",
"platformsh/client": ">=0.78.0 <2.0",
"symfony/console": "^3.0 >=3.2",
"symfony/yaml": "^3.0 || ^2.6",
"symfony/finder": "^3.0",
Expand Down
68 changes: 39 additions & 29 deletions composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions config-defaults.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ api:
# Overridden by the {application.env_prefix}DISABLE_CACHE env var.
disable_cache: false

# Disable local file-based locking for concurrency control.
# Overridden by the {application.env_prefix}DISABLE_LOCKS env var.
disable_locks: false

# Disable SSL verification (not recommended).
# Overridden by the {application.env_prefix}SKIP_SSL env var.
skip_ssl: false
Expand Down
8 changes: 6 additions & 2 deletions services.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ services:

api:
class: '\Platformsh\Cli\Service\Api'
arguments: ['@config', '@cache', '@output', '@token_config']
arguments: ['@config', '@cache', '@output', '@token_config', '@file_lock']

app_finder:
class: '\Platformsh\Cli\Local\ApplicationFinder'
Expand All @@ -43,7 +43,7 @@ services:

certifier:
class: '\Platformsh\Cli\SshCert\Certifier'
arguments: ['@api', '@config', '@shell', '@fs', '@output']
arguments: ['@api', '@config', '@shell', '@fs', '@output', '@file_lock']

config:
class: '\Platformsh\Cli\Service\Config'
Expand All @@ -56,6 +56,10 @@ services:
class: '\Platformsh\Cli\Service\Drush'
arguments: ['@config', '@shell', '@local.project', '@api', '@app_finder']

file_lock:
class: '\Platformsh\Cli\Service\FileLock'
arguments: ['@config']

fs:
class: '\Platformsh\Cli\Service\Filesystem'
arguments: ['@shell']
Expand Down
4 changes: 2 additions & 2 deletions src/Command/Auth/AuthInfoCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ protected function execute(InputInterface $input, OutputInterface $output)

// Exit early if it's the user ID.
if ($property === 'id' && $this->api()->authApiEnabled()) {
$userId = $this->api()->getMyUserId();
$userId = $this->api()->getMyUserId($input->getOption('refresh'));
if ($userId === false) {
$this->stdErr->writeln('The current session is not associated with a user ID');
return 1;
Expand All @@ -61,7 +61,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
return 0;
}

$info = $this->api()->getMyAccount();
$info = $this->api()->getMyAccount($input->getOption('refresh'));

$propertiesToDisplay = ['id', 'first_name', 'last_name', 'username', 'email', 'phone_number_verified'];
$info = array_intersect_key($info, array_flip($propertiesToDisplay));
Expand Down
25 changes: 25 additions & 0 deletions src/Service/Api.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ class Api
/** @var TokenConfig */
private $tokenConfig;

/** @var FileLock */
private $fileLock;

/**
* The library's API client object.
*
Expand Down Expand Up @@ -138,18 +141,21 @@ class Api
* @param OutputInterface|null $output
* @param TokenConfig|null $tokenConfig
* @param EventDispatcherInterface|null $dispatcher
* @param FileLock|null $fileLock
*/
public function __construct(
Config $config = null,
CacheProvider $cache = null,
OutputInterface $output = null,
TokenConfig $tokenConfig = null,
FileLock $fileLock = null,
EventDispatcherInterface $dispatcher = null
) {
$this->config = $config ?: new Config();
$this->output = $output ?: new ConsoleOutput();
$this->stdErr = $this->output instanceof ConsoleOutputInterface ? $this->output->getErrorOutput(): $this->output;
$this->tokenConfig = $tokenConfig ?: new TokenConfig($this->config);
$this->fileLock = $fileLock ?: new FileLock($this->config);
$this->dispatcher = $dispatcher ?: new EventDispatcher();
$this->cache = $cache ?: CacheFactory::createCacheProvider($this->config);
}
Expand Down Expand Up @@ -282,6 +288,25 @@ private function getConnectorOptions() {
$connectorOptions['token_url'] = $this->config->get('api.oauth2_token_url');
$connectorOptions['revoke_url'] = $this->config->get('api.oauth2_revoke_url');

// Acquire a lock to prevent tokens being refreshed at the same time in
// different CLI processes.
$refreshLockName = 'refresh--' . $this->config->getSessionIdSlug();
$connectorOptions['on_refresh_start'] = function ($originalRefreshToken) use ($refreshLockName) {
$connector = $this->getClient(false)->getConnector();
return $this->fileLock->acquireOrWait($refreshLockName, function () {
$this->stdErr->writeln('Waiting for token refresh lock', OutputInterface::VERBOSITY_VERBOSE);
}, function () use ($connector, $originalRefreshToken) {
$session = $connector->getSession();
$session->load(true);
$accessToken = $this->tokenFromSession($session);
return $accessToken && $accessToken->getRefreshToken() !== $originalRefreshToken
? $accessToken : null;
});
};
$connectorOptions['on_refresh_end'] = function () use ($refreshLockName) {
$this->fileLock->release($refreshLockName);
};

$connectorOptions['on_refresh_error'] = function (BadResponseException $e) {
return $this->onRefreshError($e);
};
Expand Down
1 change: 1 addition & 0 deletions src/Service/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ private function applyEnvironmentOverrides()
'COPY_ON_WINDOWS' => 'local.copy_on_windows',
'DEBUG' => 'api.debug',
'DISABLE_CACHE' => 'api.disable_cache',
'DISABLE_LOCKS' => 'api.disable_locks',
'DRUSH' => 'local.drush_executable',
'SESSION_ID' => 'api.session_id',
'SKIP_SSL' => 'api.skip_ssl',
Expand Down
Loading

0 comments on commit 030b3b7

Please sign in to comment.