Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cesium ion support to the Add Data panel #7193

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Add Cesium ion support to the Add Data panel #7193

wants to merge 43 commits into from

Conversation

kring
Copy link
Member

@kring kring commented Jun 9, 2024

This adds a new "Cesium ion" option to the My Data -> Add web data panel:

image

Clicking the "Connect to Cesium ion" button pops up a separate window to sign in to Cesium ion:
image

Press "Allow", and Terria will query your Cesium ion account for tokens and assets:
image

Choose an appropriate token and then click the + next to a dataset. The dataset is added to the Workbench and the My Data list as usual:
image

Just like in the normal Terria catalog, you can hold down shift or control to keep the panel open so you can quickly add multiple datasets.

The token selection is important because this token is used to access the asset, which means that if you create a Share URL, that token will be embedded in it and available to anyone that has the Share URL. You probably don't want to use a token that has access to all of your assets.

To set this up in your Terria Map, you need to first create an OAuth2 Application on Cesium ion. Here's the one I set up for local testing:

image

You can use this same one for testing on localhost. Open up your TerriaMap config.json and add this to the parameters section:

"cesiumIonOAuth2ApplicationID": 643,

For a TerriaMap not running on localhost, though, you'll need to set up your own. The Redirect URI should be

https://example.com/TerriaMap/build/TerriaJS/cesium-ion-oauth2.html

Replace https://example.com/TerriaMap with the base URL of your TerriaMap, of course. And then put the "ClientID" that Cesium ion provides in the cesiumIonOAuth2ApplicationID property in config.json.

Note that the Cesium ion support will only work on localhost and on https URLs, so it can't work on ci.terria.io because it only supports http. This is because it uses crypto.subtle.

All of the Cesium ion asset types can be imported, including 3D Tiles, glTF, KML, CZML, GeoJSON, Terrain, and Imagery. I had to extend some of these CatalogItem types to support Cesium ion, which I did via a CesiumIonMixin.

There's one quirk with glTF, though. I'm currently importing models to be located at null island, because that's better than the center of the Earth. It should probably integrate with the Model Editor instead, but I didn't explore this.

Sort of fixes #6388

@na9da
Copy link
Collaborator

na9da commented Aug 14, 2024

@kring - looks awesome.

I have done a first pass checking the UX/behavior. I'll do another for the code.

For the UX, a few things that stood out to me in testing:

  1. Currently we lists all the assets under the logged in account. Can we filter it down to what's readable with the selected token?

  2. We often restrict the access token to specific sites. This is returned as allowedUrls by the Cesium API. I guess we could only list the tokens that are usable from the current site. But maybe it is better if we show a warning if the selected token is not usable from the current site - that gives the user enough hint on what to do if the token they are using doesn't work.

  3. The token selection is important because this token is used to access the asset, which means that if you create a Share URL, that token will be embedded in it and available to anyone that has the Share URL. You probably don't want to use a token that has access to all of your assets.

    Currently you are setting the traits in the definition stratum, so the token is not shared. But if the intention is to make it shareable, I think we should also provide enough warning in the UI (needs more discussion).

  4. Does the login token have an expiry? If not persisting it by default is probably a bit risky. Maybe we could add a remember me checkbox if the user wants to persist the token although it makes the UI more complicated. (needs more discussion).

  5. There's an error (white screen) if I deny access during the connect process.

Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kring - looks great overall.

Made a few comments.
It would also be nice if we could convert most of the text to i8n strings so that they can be translated.

@@ -1,10 +1,12 @@
import i18next from "i18next";
import { action, observable } from "mobx";
import { ReactComponentLike } from "prop-types";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably use this type instead?

import { type ComponentType } from "react";

lib/ModelMixins/CesiumIonMixin.ts Show resolved Hide resolved
Comment on lines 38 to 40
// This doesn't work, probably because I don't know what I'm doing:
// name: t("core.dataType.cesium-ion")
// So, hard-coding it instead.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is because when this block executes, the Internationalization set up hasn't yet run. One solution might be to modify this bit to call translateDataType for customRemoteDataTypes also.

remoteDataType: option
});
},
this.props.viewState.remoteDataType = option;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be wrapped in an action.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would not be writing new .scss files and use styled components instead but I guess we have no other option here as some of the underlying components still use .scss.

}

interface CesiumIonAsset {
id?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a number?

}

const CesiumIonConnectorObserver = observer<React.FC>(CesiumIonConnector);
export default withTranslation()(CesiumIonConnectorObserver);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the pattern we use now instead of withTranslation for functional components is the the const {t} = useTranslation() hook. We could also remove the global import for t from i18next. Not sure, but I think using the global import might have some issues if the translation set up hasn't been initialized (although, that won't be a problem in this particular case).

example:

const CaptureScene: React.FC<CaptureSceneProps> = (props) => {
const { t } = useTranslation();
return (
<StoryButton
title={t("story.captureSceneTitle")}
btnText={t("story.captureScene")}
onClick={props.onClickCapture}

<th>Name</th>
<th>Type</th>
</tr>
{assets.map(renderAssetRow)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this map element here needs a key={assetId} which should fix the console error.

@na9da
Copy link
Collaborator

na9da commented Aug 16, 2024

@kring

I had a chat with the team on the security aspects. These are some thoughts from a novice user perspective.

  1. Persisting the login token might be risky if the user forgets to disconnect it after use.
  2. Making the access token automatically available in the share URL is also risky as it could expose more datasets than the user intended to share.

The UX for making the novice user aware of these issues could get complex. One thing we could do is remove these two behaviors, which means we limit the feature to a single user, single session for quickly previewing ones own ION assets in Terria.

Additionally, we could also try to align more with the use case here which also has some community interest. That would mean implementing a CesiumIonCatalogGroup.

Happy to discuss :)

@kring
Copy link
Member Author

kring commented Aug 18, 2024

Persisting the login token might be risky if the user forgets to disconnect it after use.

I don't think it's any riskier than holding any other login token in the digital twins, for example. But yes, there's the possibility that a XSS vulnerability or somesuch could be used to steal the login token.

If that happened, the attacker would be able to use any of the scopes that Terria has been authorized to use:

  • assets:read - Read metadata and access the tiled data for individual assets in the account.
  • assets:list - List the metadata for all assets available to the account.
  • tokens:read - Read token metadata and list all tokens for the account.
  • profile:read - Read your username, email address, avatar, and storage quota.

Scope descriptions are from here:
https://cesium.com/learn/ion/rest-api/#section/Authentication

So those scopes would definitely allow access to any asset as well as things like the user's email address. However, it wouldn't allow any changes to the ion account. We could probably get rid of profile:read if we want. It's only used to display the name of the user that is currenty connected.

The login token is only stored in React state and in local storage, so there's no significant risk of accidentally leaking it via sharing (only via vulnerabilities). If we didn't store it in local storage, the user would have to click through each time (they wouldn't have to log in again), so that wouldn't be awful. But we'd have store is somewhere more persistent than React state or else the user would have to click through again every time they open the panel, which I think would be pretty annoying. I guess that would still be a bit safer in that it would eliminate the possibility of having your login token stolen in a session in which you didn't even use Cesium ion.

Making the access token automatically available in the share URL is also risky as it could expose more datasets than the user intended to share.

Yeah, that's why there's all the red text about tokens in the UI. To some extent this is just something that Cesium ion users need to understand. Or anyone granting access to datasets in any online service, not just ion. I'm not sure there's much for it. FWIW, we have the same theoretical problem in Cesium for Unreal and Cesium for Unity and have been generally ok with the risk.

If you're sure this isn't acceptable, I can only think of two possible solutions (short of abandoning the ability to do this completely, which I think would be a shame because it's really useful):

  1. Force the user to copy/paste a token into the UI, rather than select one. This will make it harder to use, but perhaps require the user to be a bit more intentional about what they're doing. It would also allow us to drop tokens:read and assets:read from the login token, which would mean that even the login token would no longer be able to access individual assets (only list them).
  2. Make added Cesium ion assets not shareable by default. The user would have to explicitly enable sharing, perhaps by checking a box before adding the assets, or perhaps performing some other explicit action on the dataset details panel after it's added.

I think making Cesium ion assets not shareable ever would be a huge step back and miss a lot of the point of this feature.

Additionally, we could also try to align more with thehttps://github.com//issues/6388 which also has some community interest. That would mean implementing a CesiumIonCatalogGroup.

I saw that issue before I started working on this, but couldn't think of a way to implement it safely. The problem is that a catalog group that queries the ion account requires a token with assets:list scope. When you use a token with that scope, there's no way to control which assets are returned by the API. You can't define a token that only allows access to a subset. In fact, the ion docs describe assets:list as a "private" scope, which means you should never share a token with that scope. But we'd have to do just that in order to support an ion catalog group.

There's probably a really valid feature request against ion here, but that's the current situation.

Comment on lines 7 to 8
//width: 50%;
//max-width: 600px;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could try:

width: fit-content;
min-width: 50%;
max-width: 600px

To keep the original width but allow room for custom components to expand if required (within the max limit).

@na9da
Copy link
Collaborator

na9da commented Aug 19, 2024

@kring

As discussed offline, we could do a simple version first with no persistent login or sharing.

We could workout the UX changes required for sharing added datasets in a separate PR.

Persistent login by storing in local storage would still be risky for all the reasons you have given. It is also risky from the perspective of a novice user connecting from a shared machine and then forgetting to disconnect. The rest of the app having no concept of a login also makes it less obvious.

@kring
Copy link
Member Author

kring commented Sep 8, 2024

I believe I've addressed all the feedback. Rather than completely remove the ability to share and persist the token, I made these into configuration options. By default:

  1. The login token is not persisted, so you need to need to sign in each time you refresh the page.
  2. Added assets are not shareable. You'll see this message:
    image

However, you can set cesiumIonLoginTokenPersistence in config.json to one of these values:

Specifies where to store the Cesium ion login token. Valid values are:

  • page (default) - The login token is associated with the current page load. Even simply reloading the current page will clear the token. This is the safest option.
  • sessionStorage - The login token is associated with a browser session, which means it is shared/accessible from any page hosted on the same domain and running in the same browser tab.
  • localStorage - The login token is shared/accessible from any page hosted on the same domain, even when running in different tabs or after exiting and restarted the web browser.

You can also set cesiumIonAllowSharingAddedAssets in config.json:

Whether or not Cesium ion assets added via the "Add Data" panel will be shared with others via share links. If true, users will be asked to select a Cesium ion token when adding assets, and this choice must be made carefully to avoid exposing more Cesium ion assets than intended. If false (the default), the user's login token will be used, which is safe because this token will not be shared with others.

And as the text above suggests, the UI doesn't let or require the user to select a token when this is set to false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cesium ion as a Catalog Groups
2 participants