From 0f49982ceeca48e17dfcd6c00ae396d0dd4e4a90 Mon Sep 17 00:00:00 2001
From: John Cowen <johncowen@users.noreply.github.com>
Date: Tue, 24 Aug 2021 16:58:45 +0100
Subject: [PATCH] ui: [BUGFIX] Properly encode non-URL safe characters in OIDC
 responses (#10901)

This commit fixes 2 problems with our OIDC flow in the UI, the first is straightforwards, the second is relatively more in depth:

1: A typo (1.10.1 only)

During #10503 we injected our settings service into the our oidc-provider service, there are some comments in the PR as to the whys and wherefores for this change (https://github.com/hashicorp/consul/pull/10503/files#diff-aa2ffda6d0a966ba631c079fa3a5f60a2a1bdc7eed5b3a98ee7b5b682f1cb4c3R28)

Fixing the typo so it was no longer looking for an unknown service (repository/settings > settings)
fixed this.

2: URL encoding (1.9.x, 1.10.x)

TL;DR: /oidc/authorize/provider/with/slashes/code/with/slashes/status/with/slashes should be /oidc/authorize/provider%2Fwith%2Fslashes/code%2Fwith%2Fslashes/status%2Fwith%2Fslashes

When we receive our authorization response back from the OIDC 3rd party, we POST the code and status data from that response back to consul via acallback as part of the OIDC flow. From what I remember back when this feature was originally added, the method is a POST request to avoid folks putting secret-like things into API requests/URLs/query params that are more likely to be visible to the human eye, and POSTing is expected behaviour.

Additionally, in the UI we identify all external resources using unique resource identifiers. Our OIDC flow uses these resources and their identifiers to perform the OIDC flow using a declarative state machine. If any information in these identifiers uses non-URL-safe characters then these characters require URL encoding and we added a helper a while back to specifically help us to do this once we started using this for things that required URL encoding.

The final fix here make sure that we URL encode code and status before using them with one of our unique resource identifiers, just like we do with the majority of other places where we use these identifiers.
---
 .changelog/10901.txt                                 |  3 +++
 .../consul-ui/app/components/token-source/index.hbs  |  9 ++++++++-
 .../app/services/repository/oidc-provider.js         |  2 +-
 .../unit/services/repository/oidc-provider-test.js   | 12 ++++++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 .changelog/10901.txt
 create mode 100644 ui/packages/consul-ui/tests/unit/services/repository/oidc-provider-test.js

diff --git a/.changelog/10901.txt b/.changelog/10901.txt
new file mode 100644
index 000000000..4a00a7da1
--- /dev/null
+++ b/.changelog/10901.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ui: Properly encode non-URL safe characters in OIDC responses
+```
diff --git a/ui/packages/consul-ui/app/components/token-source/index.hbs b/ui/packages/consul-ui/app/components/token-source/index.hbs
index e83e5da07..6bedcd930 100644
--- a/ui/packages/consul-ui/app/components/token-source/index.hbs
+++ b/ui/packages/consul-ui/app/components/token-source/index.hbs
@@ -25,7 +25,14 @@
     </State>
     <State @matches="token">
       <DataSource
-        @src={{concat path '/oidc/authorize/' provider.Name '/' jwt.authorizationCode '/' jwt.authorizationState}}
+        @src={{uri
+          (concat path '/oidc/authorize/${provider}/${code}/${state}')
+            (hash
+              provider=provider.Name
+              code=jwt.authorizationCode
+              state=(or jwt.authorizationState '')
+            )
+        }}
         @onchange={{action 'change'}}
         @onerror={{action onerror}}
       />
diff --git a/ui/packages/consul-ui/app/services/repository/oidc-provider.js b/ui/packages/consul-ui/app/services/repository/oidc-provider.js
index c89522665..85d6d2926 100644
--- a/ui/packages/consul-ui/app/services/repository/oidc-provider.js
+++ b/ui/packages/consul-ui/app/services/repository/oidc-provider.js
@@ -8,7 +8,7 @@ const modelName = 'oidc-provider';
 const OAUTH_PROVIDER_NAME = 'oidc-with-url';
 export default class OidcProviderService extends RepositoryService {
   @service('torii') manager;
-  @service('repository/settings') settings;
+  @service('settings') settings;
 
   init() {
     super.init(...arguments);
diff --git a/ui/packages/consul-ui/tests/unit/services/repository/oidc-provider-test.js b/ui/packages/consul-ui/tests/unit/services/repository/oidc-provider-test.js
new file mode 100644
index 000000000..8164fbbcf
--- /dev/null
+++ b/ui/packages/consul-ui/tests/unit/services/repository/oidc-provider-test.js
@@ -0,0 +1,12 @@
+import { module, test } from 'qunit';
+import { setupTest } from 'ember-qunit';
+
+module('Unit | Repository | oidc-provider', function(hooks) {
+  setupTest(hooks);
+
+  // Replace this with your real tests.
+  test('it exists', function(assert) {
+    let service = this.owner.lookup('service:repository/oidc-provider');
+    assert.ok(service);
+  });
+});