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

Merged
merged 45 commits into from
Sep 30, 2024
Merged

Add Cesium ion support to the Add Data panel #7193

merged 45 commits into from
Sep 30, 2024

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.

lib/Core/getDataType.ts Outdated Show resolved Hide resolved
lib/ModelMixins/CesiumIonMixin.ts Show resolved Hide resolved
lib/ReactViews/Custom/registerCustomComponentTypes.ts Outdated Show resolved Hide resolved
lib/ReactViews/ExplorerWindow/Tabs/MyDataTab/AddData.jsx Outdated Show resolved Hide resolved
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.

@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.

@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.

Copy link
Contributor

@ljowen ljowen left a comment

Choose a reason for hiding this comment

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

All looks good to me, just 2 unused imports.

lib/ModelMixins/CesiumIonMixin.ts Outdated Show resolved Hide resolved
lib/ModelMixins/CesiumIonMixin.ts Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2024

CLA assistant check
All committers have signed the CLA.

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.

LGTM. By default:

  • the imported ion assets cannot be shared
  • the ion login is lost on page reload, so that the user doesn't have to remember to disconnect/logout.

(I have merged in main and made some lint fixes.)

@na9da na9da merged commit fa7c7f0 into main Sep 30, 2024
9 checks passed
@na9da na9da deleted the cesium-ion branch September 30, 2024 03:15
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
4 participants