-
Notifications
You must be signed in to change notification settings - Fork 361
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
base: main
Are you sure you want to change the base?
Conversation
Because it doesn't work anyway, due to use of http instead of https.
@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:
|
There was a problem hiding this 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
@@ -1,10 +1,12 @@ | |||
import i18next from "i18next"; | |||
import { action, observable } from "mobx"; | |||
import { ReactComponentLike } from "prop-types"; |
There was a problem hiding this comment.
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";
// 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. |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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:
terriajs/lib/ReactViews/Story/StoryBuilder.tsx
Lines 543 to 549 in f29650d
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)} |
There was a problem hiding this comment.
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.
I had a chat with the team on the security aspects. These are some thoughts from a novice user perspective.
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 Happy to discuss :) |
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:
Scope descriptions are from here: 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 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.
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):
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.
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 There's probably a really valid feature request against ion here, but that's the current situation. |
//width: 50%; | ||
//max-width: 600px; |
There was a problem hiding this comment.
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).
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. |
When false (the default), users need not select a token because assets are not shareable.
This adds a new "Cesium ion" option to the My Data -> Add web data panel:
Clicking the "Connect to Cesium ion" button pops up a separate window to sign in to Cesium ion:
![image](https://private-user-images.githubusercontent.com/924374/337956487-f80e1c1d-dd5e-41bb-8d9b-bd34649baf40.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMzAzOTcsIm5iZiI6MTcyNjEzMDA5NywicGF0aCI6Ii85MjQzNzQvMzM3OTU2NDg3LWY4MGUxYzFkLWRkNWUtNDFiYi04ZDliLWJkMzQ2NDliYWY0MC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMlQwODM0NTdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01NGM5MWJmMDM0Njk0ZmM4MjM4NmVhZjE2MzgxZWRlM2JjZTlhN2NkYWEzMWQ0MDMyZGUwYTczYzQ5MjM3ZWFiJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.tUvn4YLUNamYKUnDQQxKvWyYNsev7TeyiSK_zrU8XOs)
Press "Allow", and Terria will query your Cesium ion account for tokens and assets:
![image](https://private-user-images.githubusercontent.com/924374/337956522-207acc7b-5d41-467c-8a5e-937448658041.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMzAzOTcsIm5iZiI6MTcyNjEzMDA5NywicGF0aCI6Ii85MjQzNzQvMzM3OTU2NTIyLTIwN2FjYzdiLTVkNDEtNDY3Yy04YTVlLTkzNzQ0ODY1ODA0MS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMlQwODM0NTdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jYmM4MTU0OTYwMzg5YmVmZjZjNjUzMTYzNDZjNmJiODI0OTUyOTE5ZDEwOGJjOTg2MjE2ODAyNzNiNDJkN2ZlJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.hU-_PeFbQuZZBFkm7K0SnYK7V6PxbOIE4BREpJKKFG4)
Choose an appropriate token and then click the
![image](https://private-user-images.githubusercontent.com/924374/337956563-85f79bb3-502b-4264-8f97-c484a287e20e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMzAzOTcsIm5iZiI6MTcyNjEzMDA5NywicGF0aCI6Ii85MjQzNzQvMzM3OTU2NTYzLTg1Zjc5YmIzLTUwMmItNDI2NC04Zjk3LWM0ODRhMjg3ZTIwZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMlQwODM0NTdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT02NTAwNjQ4ZGZiMDUwZjBjZWE0MDdhODU4N2YxZjI1NDg4ODIyMjk2NDFmMTJjOTA2ZDRjNjhlYWEyMjhlYzM0JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.eLZ73MHWoAYgTEQQ4mZdJMh2ET44gM5pugLcaLloGuE)
+
next to a dataset. The dataset is added to the Workbench and the My Data list as usual: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:
You can use this same one for testing on localhost. Open up your TerriaMap config.json and add this to the
parameters
section:For a TerriaMap not running on localhost, though, you'll need to set up your own. The Redirect URI should be
Replace
https://example.com/TerriaMap
with the base URL of your TerriaMap, of course. And then put the "ClientID" that Cesium ion provides in thecesiumIonOAuth2ApplicationID
property in config.json.Note that the Cesium ion support will only work on localhost and on
https
URLs, so it can't work onci.terria.io
because it only supportshttp
. 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