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 AuthorizationClient onAccessTokenChanged event #6122

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ben-polinsky
Copy link
Contributor

Add an optional onAccessTokenChanged BeEvent to the AuthorizationClient interface.
We plan to make this property required in 5.0.

@ben-polinsky
Copy link
Contributor Author

@calebmshafer want to ensure you are okay with this before merging

@calebmshafer
Copy link
Member

@ben-polinsky @aruniverse what is the implementation of this going to look like in the Desktop and Mobile auth? I'm still not clear of the requirement for this.

I'm assuming the reason you would need to get an event is if you're caching the token somewhere and need to make sure it's still valid, plus avoid the overhead of calling the getAccessToken() method all the time. Is that the reasoning here?

This interface, and iModelApp, assume that the implementations passed in have done the most optimal thing on their side and keep the token up-to-date.

There is special cache handling in both the mobile and desktop clients so their specific implementations kept the correct token sync-ed between the backend and frontend processes.

@aruniverse
Copy link
Member

what is the implementation of this going to look like in the Desktop and Mobile auth

Desktop already implements this, see

Mobile, im not sure. Maybe @tcobbs-bentley can chime in there

Copy link
Member

@calebmshafer calebmshafer left a comment

Choose a reason for hiding this comment

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

Going to block this change for now. Let's have a design discussion around this more and look at all 3 implementations (web, mobile, desktop) before moving forward with it.

@tcobbs-bentley
Copy link
Member

what is the implementation of this going to look like in the Desktop and Mobile auth

Desktop already implements this, see

Mobile, im not sure. Maybe @tcobbs-bentley can chime in there

MobileHost already has onAuthAccessTokenChanged that gets posted when the native side auth client indicates the change, so moving this to the AuthorizationClient is probably fine (although I'm not sure about the implementation details of the move).

@ImVeryLost
Copy link
Contributor

@ben-polinsky @calebmshafer any updates on this?

@toddsouthenbentley
Copy link
Contributor

toddsouthenbentley commented Mar 2, 2024

This PR would be very useful for my work with iTwin Studio.

In an effort to get it rolling, I created a draft PR of the changes required for Mobile: #6485

@aruniverse aruniverse marked this pull request as draft May 17, 2024 19:08
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.

None yet

7 participants