-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Support iTwin share keys #12530
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
Support iTwin share keys #12530
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
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.
Thanks @jjspace! Just those two notes.
Thanks @ggetz, updated |
|
||
Cesium.ITwinPlatform.defaultAccessToken = token; | ||
// If you do use oauth for user login set: | ||
// Cesium.ITwinPlatform.defaultAccessToken = 'your token' |
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.
@jjspace Do we need to have this here? Perhaps an update can point to the tutorial and developer documentation when ready. Same comment for the other sandcastle too.
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.
@shehzan10 We can certainly add more docs or tutorials later but I don't think there's an issue with providing this information in multiple locations. These sandcastles may end up being one of the main ways to quick reference how to interact with iTwin data in CesiumJS. I think it's worth keeping these 2 lines just for convenience/reference
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.
@jjspace With the goal being to introduce this to the Cesium community via CesiumJS first, IMO its important to keep it simple. By having the comment If you do use oauth for user login set
, the statement is vauge-ish because OAuth login has a very specific meaning. So either:
- We remove this comment and instead point to the tutorials/documentation when ready.
- Make the comment more accurate, yet simple to understand and allows the developer on which approach to use.
Open to other suggestions as well.
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 agree with @jjspace here, I think its nice to show users that they're able to use either form of authentication to use the sandcastle demo -- sharing or oauth. If you'd like to add more detail to suggesting oauth, you could point them do our oauth docs: https://developer.bentley.com/apis/overview/authorization/
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.
Would it be better to then have a comment like:
// For alternative forms of authentication you can use, visit https://developer.bentley.com/apis/overview/authorization/.
And combine this with line 35?
Thanks @jjspace, as well as @shehzan10 and @davidhjones for your feedback! |
Description
ITwinPlatform.defaultShareKey
to support using the iTwin Share API for public accessITwinPlatform.defaultAccessToken
if it's setBasic
auth instead ofBearer
so I added a helper function for generating the Auth headersIssue number and link
No issue
Testing plan
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change