Sandcastle oauth with ion by jjspace · Pull Request #13403 · CesiumGS/cesium

jjspace · 2026-04-14T17:13:56Z

Description

Adds basic Login functionality using ion oauth. This also provides a "User" context for the whole app so other components can depend on the currently logged in user. As a proof of concept there is a new Insert > Access token option with the editor that will add your account's default access token to the current sandcastle when logged in. This will be replaced with a selection modal for multiple tokens later.

This PR replaces the #13079 PR which had a mix of changes that have already been merged.

Currently login is only enabled in local development because we need to set up additional domains and deployments to securely enable in deployed environments.

Issue number and link

Private repo issue

Testing plan

run npm run build-sandcastle to build with the new login UI
run npm start and open local sandcastle
Click Login in the upper right and login with oauth
Test importing your token into the editor
- Clicking the Test fetch will send a request that can be checked in the network tab, this is also temporary
Test Logging out and back in
Test clicking Deny on the oauth screen
Navigate to a specific sandcastle and Log-in and make sure you return to that same sandcastle after the auth page.

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code

AI acknowledgment

I used AI to generate content in this PR
If yes, I have reviewed the AI-generated content before submitting

If yes, I used the following Tools(s) and/or Service(s):

If yes, I used the following Model(s):

github-actions · 2026-04-14T17:14:20Z

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

ggetz · 2026-04-21T14:01:46Z

Thanks @jjspace!

Could you please capture this task in a dedicated issue so we can track?

Currently login is only enabled in local development because we need to set up additional domains and deployments to securely enable in deployed environments.

Could you please capture this in a TODO list in this PR with this item as a reminder to clean this up before merging?

Test importing your token into the editor
Clicking the Test fetch will send a request that can be checked in the network tab, this is also temporary

I notice there is a "Logging in..." string that flashes in the navbar. Could we remove that and replace it with a skeleton or less obvious placeholder?
Is there any contributor documentation we should update with links to the relevant Cesium ion docs?

ggetz · 2026-04-21T14:13:59Z

IMO, showing this button at all if the feature is not available is a bit confusing. Can we hide if disabled?

ggetz

Currently, this stops short of comprehensive error handling and communicating that status to the user. Any thoughts on how to address this?
Can we put the OAuth credentials in an environment variable instead of hardcoded, like in buildSandcastle.js and the cite config?

ggetz · 2026-04-21T13:48:47Z

 * @param {ImportList} options.imports Set of imports to add to the import map for the iframe and standalone html pages. These paths should match the URL where it can be accessed within the current environment.
 * @param {string} options.outerOrigin Origin of the outer application
 * @param {string} options.innerOrigin Origin of the inner viewer bucket. Defaults to the outerOrigin if not provided
+ * @param {{clientId: string, callbackUrl: string}} [options.ionClientSettings] Origin of the inner viewer bucket. Defaults to the outerOrigin if not provided


I think this is the wrong parameter desciption.

ggetz · 2026-04-21T13:51:09Z

+        response_type: "code",
+        scope: this.config.scopes.join(" "),
+        state: stateId,
+      }).toString()}`,


I believe the toString call is redundant when inside of a template string.

ggetz · 2026-04-21T13:53:18Z

+  async tokenExchange(code: string, state: UUID) {
+    // @see https://cesium.com/learn/ion/ion-oauth2/#step-3-token-exchange
+    // Retrieve the verifier based on the state
+    const { codeVerifier, previousPage } = (await getPkceState(state)) ?? {};


Should this handle any errors thrown via a try/catch?

ggetz · 2026-04-21T14:03:35Z

+    this.oauth =
+      oauth ?? new IonOAuth({ ion, ionApi, callbackUrl, clientId, scopes });
+  }


It's a bit non-obvious, when treating this class as a black box, that the parameters are either/or. Worth some brief inline jsdocs?

ggetz · 2026-04-21T14:06:34Z

+    }
+
+    this.initStatus = "IN_PROGRESS";
+    let deferredResolve;


Why the deferred resolve and saved initPromise property? This is an anti-pattern we've tried to avoid in other parts of CesiumJS codebase as it can lead to unhandled errors.

Can this all be handle within the Promise constructor? Or could the remainder of this function be captured in it's own async function?

ggetz · 2026-04-21T14:09:00Z

+    await crypto.subtle.digest("SHA-256", new TextEncoder().encode(input)),
+  );
+  const base64 = result.toBase64({ alphabet: "base64url", omitPadding: true });
+  console.log("base64", base64);


Please do a pass and cleanup any rogue console.log statements.

ggetz · 2026-04-21T14:11:02Z

+ * table for this if you can spare it, as it uses ~150 bytes per actively logging in user, and
+ * public abuse could easily kill your server.
+ */
+const MAX_AGE = 60 * 1000;


Is max-age specified in the response headers from ion? Should we be using that instead to account for any upstream changes in ion?

ggetz · 2026-04-21T14:14:21Z

+    if (!resp.username) {
+      return undefined;
+    }
+    // TODO: remove just testing


ggetz · 2026-04-21T14:14:52Z

+      // However we should probably add the option to update it in place
+      return;
+    }
+    // TODO: this is an async edit of the code. Probably need a way to lock the editor?


Should this be handled?

ggetz · 2026-04-21T14:20:54Z

+}
+
+/* eslint-disable-next-line no-extend-native */
+Uint8Array.prototype.toBase64 ??= function toBase64(options) {


I think you'll need to add the polyfill before toBase64 is called in sha256. Moving it above the exported function should be suficient.

jjspace added 8 commits March 27, 2026 13:35

initial user login and context

fcf1c44

track and return to previous page, change token import

f14e381

handle denied pkce urls

3d31fd5

remove extra console log

265a61d

Merge remote-tracking branch 'origin/main' into sandcastle-oauth

fafdf86

move ion client settings to build time config

8179458

add client id for normal local dev server

26553b1

adjust todo comment

2bb68fc

jjspace self-assigned this Apr 14, 2026

jjspace added the category - sandcastle label Apr 14, 2026

jjspace mentioned this pull request Apr 16, 2026

Sandcastle login #13079

Closed

9 tasks

ggetz requested changes Apr 21, 2026

View reviewed changes

Conversation

jjspace commented Apr 14, 2026

Description

Issue number and link

Testing plan

Author checklist

AI acknowledgment

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

ggetz commented Apr 21, 2026

Uh oh!

ggetz commented Apr 21, 2026

Uh oh!

ggetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants