Skip to content

Commit

Permalink
dev/core#2141 - Tighten up new page civicrm/oauth-client/return
Browse files Browse the repository at this point in the history
Overview
--------

The route `civicrm/oauth-client/return` is added in 5.32 as the main "Redirect URL".

In normal usage, the page shouldn't be visible to a user (because the
developer should define some alternative UI) -- but one might see it (a)
during development, (b) if there's an error, or (c) if a clever user mucks
about.

Improvements:

* Error handling
    * Present error messages more nicely
    * Record errors in the log
    * Report more info via hook_oauthReturnError
* Other UI
    * Redact token details (dependent upon permission `manage OAuth client secrets`)
    * Set a more sensibile page title
    * Make output blobs conditional and collapsible
  • Loading branch information
totten committed Nov 10, 2020
1 parent 4fdca40 commit 7121e3c
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 15 deletions.
30 changes: 23 additions & 7 deletions ext/oauth-client/CRM/OAuth/Page/Return.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,33 @@ class CRM_OAuth_Page_Return extends CRM_Core_Page {
const TTL = 3600;

public function run() {
$json = function ($d) {
return json_encode($d, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES);
};

$state = self::loadState(CRM_Utils_Request::retrieve('state', 'String'));
if (CRM_Core_Permission::check('manage OAuth client')) {
$this->assign('state', $state);
$this->assign('stateJson', $json($state ?? NULL));
}

if (CRM_Utils_Request::retrieve('error', 'String')) {
CRM_Utils_System::setTitle(ts('OAuth Error'));
$error = CRM_Utils_Array::subset($_GET, ['error', 'error_description', 'error_uri']);
$event = \Civi\Core\Event\GenericHookEvent::create([
'error' => $error['error'] ?? NULL,
'description' => $error['description'] ?? NULL,
'uri' => $error['uri'] ?? NULL,
'state' => $state,
]);
Civi::dispatcher()->dispatch('hook_civicrm_oauthReturnError', $event);

Civi::log()->info('OAuth returned error', [
'error' => $error,
'state' => $state,
]);

$this->assign('error', $error ?? NULL);
}
elseif ($authCode = CRM_Utils_Request::retrieve('code', 'String')) {
$client = \Civi\Api4\OAuthClient::get(0)->addWhere('id', '=', $state['clientId'])->execute()->single();
Expand All @@ -37,18 +54,17 @@ public function run() {
if ($nextUrl !== NULL) {
CRM_Utils_System::redirect($nextUrl);
}

CRM_Utils_System::setTitle(ts('OAuth Token Created'));
if (CRM_Core_Permission::check('manage OAuth client')) {
$this->assign('token', CRM_OAuth_BAO_OAuthSysToken::redact($tokenRecord));
$this->assign('tokenJson', $json(CRM_OAuth_BAO_OAuthSysToken::redact($tokenRecord)));
}
}
else {
throw new \Civi\OAuth\OAuthException("OAuth: Unrecognized return request");
}

$json = function ($d) {
return json_encode($d, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES);
};
$this->assign('state', $json($state));
$this->assign('token', $json($tokenRecord ?? NULL));
$this->assign('error', $json($error ?? NULL));

parent::run();
}

Expand Down
47 changes: 39 additions & 8 deletions ext/oauth-client/templates/CRM/OAuth/Page/Return.tpl
Original file line number Diff line number Diff line change
@@ -1,10 +1,41 @@
<h3>Welcome back</h3>
{if $error}
<div class="crm-accordion-wrapper">
<div class="crm-accordion-header">
{ts}OAuth Error Details{/ts}
</div>
<div class="crm-accordion-body">
<ul>
<li><strong>{ts}Error type:{/ts}</strong> {$error.error|escape:'html'}</li>
<li><strong>{ts}Error description:{/ts}</strong>
<pre>{$error.error_description|escape:'html'}</pre>
</li>
<li><strong>{ts}Error URI:{/ts}</strong> <code>{$error.error_uri|escape:'html'}</code></li>
</ul>
</div>
</div>
{else}
<p>{ts}An OAuth token was created!{/ts}</p>
<p>{ts}There is no clear "next step", so this may be a new integration. Please update the integration to define a next step via "hook_civicrm_oauthReturn" or "landingUrl".{/ts}</p>
{/if}

<h4>State:</h4>
<pre>{$state}</pre>
{if $stateJson}
<div class="crm-accordion-wrapper collapsed">
<div class="crm-accordion-header">
{ts}OAuth State{/ts}
</div>
<div class="crm-accordion-body">
<pre>{$stateJson}</pre>
</div>
</div>
{/if}

<h4>Token:</h4>
<pre>{$token}</pre>

<h4>Error</h4>
<pre>{$error}</pre>
{if $tokenJson}
<div class="crm-accordion-wrapper collapsed">
<div class="crm-accordion-header">
{ts}OAuth Token{/ts}
</div>
<div class="crm-accordion-body">
<pre>{$tokenJson}</pre>
</div>
</div>
{/if}

0 comments on commit 7121e3c

Please sign in to comment.