From 7121e3c1543f5f91c08d7a5170c2ccace9c59d98 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 10 Nov 2020 14:25:21 -0800
Subject: [PATCH] dev/core#2141 - Tighten up new page
 `civicrm/oauth-client/return`

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
---
 ext/oauth-client/CRM/OAuth/Page/Return.php    | 30 +++++++++---
 .../templates/CRM/OAuth/Page/Return.tpl       | 47 +++++++++++++++----
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/ext/oauth-client/CRM/OAuth/Page/Return.php b/ext/oauth-client/CRM/OAuth/Page/Return.php
index 4fcb35c4ed..b3df41301a 100644
--- a/ext/oauth-client/CRM/OAuth/Page/Return.php
+++ b/ext/oauth-client/CRM/OAuth/Page/Return.php
@@ -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();
@@ -37,18 +54,17 @@ class CRM_OAuth_Page_Return extends CRM_Core_Page {
       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();
   }
 
diff --git a/ext/oauth-client/templates/CRM/OAuth/Page/Return.tpl b/ext/oauth-client/templates/CRM/OAuth/Page/Return.tpl
index 30e6334f33..0939508462 100644
--- a/ext/oauth-client/templates/CRM/OAuth/Page/Return.tpl
+++ b/ext/oauth-client/templates/CRM/OAuth/Page/Return.tpl
@@ -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>
\ No newline at end of file
+{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}
-- 
2.25.1