refactor (plus fixes): rewrite auth logic for simplification #7

pepopowitz · 2024-05-10T22:00:56Z

[BLOCKED]: requires additional changes outside this PR to be merged at roughly the same time.

This PR rewrites the auth logic in our API tutorials, to simplify the logic, and to make it more clear to readers how the client ID, secret, and audience are used for authorization.

It also fixes a couple bugs in the Optimize API client code. I'll call these out with inline comments. I'm open to the idea that these bugs should be fixed in a separate PR, so that they don't have to wait for this and its related PRs (described in "Outside impact") to land.

Outside impact

There are multiple points of impact outside this PR:

✅ Our associated API tutorial docs will need to be updated to account for these changes. (Christina has done this in Create optimize api tutorial and revise admin api tutorial camunda-docs#3708!)
Create modeler api tutorial #5 will need to be updated with the new approach to authorization. (this can happen in a follow-up PR, because Create modeler api tutorial #5 is still in progress.)

Why are we rewriting the auth logic again?

While walking through #5, and reading comments there, I realized that the reusability I've been trying to accomplish in the auth logic contains an abstraction that is confusing, and also adds complexity to the auth.js logic that distracts readers from understanding how their code acquires authorization tokens.

This new approach to auth removes the abstraction ("administration" APIs vs "component" APIs) in favor of requiring the calling client to pass in its own client ID, secret, and audience. This makes it easier to read the auth.js code, and it makes it more obvious that those three bits are required to authorize an API client.

…ect one-config-per-api

…couple bugs

pepopowitz · 2024-05-10T22:01:36Z

.env.example

I reordered keys in this file to better associate them with each other.

pepopowitz · 2024-05-10T22:02:17Z

.env.example

+# Required for Optimize API tutorials, to acquire auth tokens
+OPTIMIZE_CLIENT_ID=fill-this-in
+OPTIMIZE_CLIENT_SECRET=fill-this-in
+OPTIMIZE_AUDIENCE=optimize.camunda.io


These keys are now specific to the Optimize API only. There will be no shared use of these keys across other "component" APIs.

I get hung up here if I put on my super novice user hat. If I download my credentials as environment variables, I get values that look like this:

export ZEEBE_ADDRESS='xx.zeebe.camunda.io:443'
export ZEEBE_CLIENT_ID='xx'
export ZEEBE_CLIENT_SECRET=xx'
export ZEEBE_AUTHORIZATION_SERVER_URL='https://login.cloud.camunda.io/oauth/token'
export ZEEBE_REST_ADDRESS='https://lhr-1.zeebe.camunda.io/xx'
export ZEEBE_GRPC_ADDRESS='grpcs://xx-1.zeebe.camunda.io:443'
export ZEEBE_TOKEN_AUDIENCE='zeebe.camunda.io'
export CAMUNDA_CLUSTER_ID='xx'
export CAMUNDA_CLUSTER_REGION='lhr-1'
export CAMUNDA_CREDENTIALS_SCOPES='Zeebe,Tasklist,Operate,Optimize,Secrets'
export CAMUNDA_TASKLIST_BASE_URL='https://lhr-1.tasklist.camunda.io/xx'
export CAMUNDA_OPTIMIZE_BASE_URL='https://lhr-1.optimize.camunda.io/xx'
export CAMUNDA_OPERATE_BASE_URL='https://lhr-1.operate.camunda.io/xx'
export CAMUNDA_OAUTH_URL='https://login.cloud.camunda.io/oauth/token'

Zeebe is the only component that shows a CLIENT_ID and CLIENT_SECRET. This remains true even if the scope is only Optimize.

Do you think this is confusing or misleading?

Maybe but I'm not sure how to address the fact that client IDs/secrets can be used across multiple APIs, but they don't have to be, and some of them come from the organization level and others come from the cluster level. Supporting the re-use of credentials across multiple APIs seems to provide multiple points of confusion; assigning one secret/key per API in the environment variables still allows for people to reuse credentials, if they want, but also makes it obvious that they need creds that will work for that specific API.

Do you have any suggestions? Can this be addressed in the supporting tutorial?

In the .env.example file, I would note what values you will expect to see in the downloaded credentials. So, in this case, I needed to use the ZEEBE_CLIENT_ID as the OPTIMIZE_CLIENT_ID, etc. You can also note this in the docs with an admonition.

I would encourage you or Christina to follow up with the Console team because this looks misleading. I don't think these values are specific to Zeebe but old historical artifacts from a time when there were only Zeebe clients.

Is this what you're looking for?

Yes, I think this is super helpful and something we should keep an eye on in case there is confusion around where to get these values. Thank you!

@pepopowitz were you going to add these changes to this PR? I'm on the fence about whether this is blocking or not.

I thought I had! I just pushed these changes.

pepopowitz · 2024-05-10T22:04:25Z

auth.js

+ * @param {string} config.audience - The audience associated with the target API.
+ * @returns {string}
+ */
+export async function getAccessToken(config) {


I moved this function to the top of the file, because it is the entry point to this logic, and for users who want to read the code and understand how to use a library to handle auth for their code, this is where they'll want to start.

It also now takes a config object, which contains three properties: clientId, clientSecret, and audience. Previously we were passing a magic string and doing some lookups within this file to get the correct environment variables, but that was obfuscating important details -- you need the client id, secret, and audience in order to authorize.

pepopowitz · 2024-05-10T22:05:41Z

completed/administration.js

+const authorizationConfiguration = {
+  clientId: process.env.ADMINISTRATION_CLIENT_ID,
+  clientSecret: process.env.ADMINISTRATION_CLIENT_SECRET,
+  audience: process.env.ADMINISTRATION_AUDIENCE
+};


I like the idea of assigning this config object once per API client, rather than every single time. Open to feedback on it though.

pepopowitz · 2024-05-10T22:06:05Z

completed/administration.js

-    "administration",
-    administrationAudience
-  );
+  const accessToken = await getAccessToken(authorizationConfiguration);


this call gets simplified, and a little more clear IMO.

pepopowitz · 2024-05-10T22:07:11Z

completed/optimize.js


-  const url = `${optimizeApiUrl}/public/dashboard/${dashboardId}`;
+  const optimizeApiUrl = process.env.OPTIMIZE_BASE_URL;


Bug here: the key name didn't match what's in our .env.example, or the other client function (listDashboards).

pepopowitz · 2024-05-10T22:07:47Z

completed/optimize.js


-  const url = `${optimizeApiUrl}/public/dashboard/${dashboardId}`;
+  const optimizeApiUrl = process.env.OPTIMIZE_BASE_URL;
+  const url = `${optimizeApiUrl}/api/public/dashboard/${dashboardId}`;


Bug here: the /api was missing at the beginning, resulting in a failed API call.

pepopowitz · 2024-05-10T22:08:20Z

completed/optimize.js

@@ -55,7 +59,7 @@ async function deleteDashboard([dashboardId]) {

    // Process the results from the API call.
    if (response.status === 204) {
-      console.log(`Dashboard ${clientId} was deleted!`);
+      console.log(`Dashboard ${dashboardId} was deleted!`);


Bug here: clientId was copy-pasta, and this line was throwing an undefined variable error.

akeller

Went through this for Optimize and it works!

pepopowitz added 10 commits May 10, 2024 16:03

refactor: simplify auth.js to take auth configuration as args

1f5d2b1

refactor: reorganize .env.example and rename some keys to better refl…

f819ded

…ect one-config-per-api

refactor: rewrite administration.list to use new auth.js args

24c22ad

refactor: rewrite administration.add to use new auth.js args

29e3d96

refactor: rewrite administration.view to use new auth.js args

fe93971

refactor: rewrite administration.delete to use new auth.js args

7af7dce

chore: revert starting administration.js and move updates to completed

63fc0a8

refactor: rewrite optimize.list to use new auth.js args

aa6f288

refactor: rewrite optimize.delete to use new auth.js args, and fix a …

f025128

…couple bugs

fix: optimize.delete undefined variable

ec7e97a

pepopowitz added bug Something isn't working enhancement New feature or request labels May 10, 2024

pepopowitz requested review from akeller and christinaausley May 10, 2024 22:00

pepopowitz self-assigned this May 10, 2024

pepopowitz commented May 10, 2024

View reviewed changes

pepopowitz mentioned this pull request May 14, 2024

Create optimize api tutorial and revise admin api tutorial camunda/camunda-docs#3708

Merged

9 tasks

docs: better descriptions of environment variables

d8fe427

akeller approved these changes May 20, 2024

View reviewed changes

pepopowitz mentioned this pull request May 20, 2024

Create modeler api tutorial #5

Merged

pepopowitz changed the title ~~[BLOCKED] refactor (plus fixes): rewrite auth logic for simplification~~ refactor (plus fixes): rewrite auth logic for simplification May 20, 2024

pepopowitz merged commit 81b3f5e into main May 28, 2024
1 check passed

pepopowitz deleted the pepopowitz/rewrite-auth-again branch May 28, 2024 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor (plus fixes): rewrite auth logic for simplification #7

refactor (plus fixes): rewrite auth logic for simplification #7

pepopowitz commented May 10, 2024 •

edited

Loading

pepopowitz May 10, 2024

pepopowitz May 10, 2024

akeller May 13, 2024

pepopowitz May 14, 2024

akeller May 14, 2024

pepopowitz May 14, 2024

akeller May 15, 2024

akeller May 20, 2024

pepopowitz May 20, 2024

pepopowitz May 10, 2024

pepopowitz May 10, 2024

pepopowitz May 10, 2024

pepopowitz May 10, 2024

pepopowitz May 10, 2024

pepopowitz May 10, 2024

akeller left a comment


		const url = `${optimizeApiUrl}/public/dashboard/${dashboardId}`;
		const optimizeApiUrl = process.env.OPTIMIZE_BASE_URL;

refactor (plus fixes): rewrite auth logic for simplification #7

refactor (plus fixes): rewrite auth logic for simplification #7

Conversation

pepopowitz commented May 10, 2024 • edited Loading

Outside impact

Why are we rewriting the auth logic again?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akeller left a comment

Choose a reason for hiding this comment

pepopowitz commented May 10, 2024 •

edited

Loading