search

taskcluster / taskcluster-client-web

A Taskcluster client library for the browser
Mozilla Public License 2.0
3 stars 8 forks source link

Lock OIDCCredentialAgent getCredentials behind cross-tab mutex #26

Closed eliperelman closed 6 years ago

eliperelman commented 6 years ago

See comments for description about how this works. Using fast-mutex syncs a mutex across tabs via localStorage.

Using this would supersede the work in tools by building this directly into the credential agent, so I think everyone benefits, especially the login service. 😃

I'm still unsure about feasibility, so happy to open a dialogue.

djmitche commented 6 years ago

It seems like i didn't "get it" from reading the code. Can you add some lengthier comments to describe the expected operation?

eliperelman commented 6 years ago

@djmitche haha I was doing that as we speak.

eliperelman commented 6 years ago

Maybe the access_token should be covered by the mutex, too?

@djmitche is your proposition then to also re-set accessToken in the getCredentials flow?

djmitche commented 6 years ago

My thinking is that the set accessToken method should "share" that accessToken across tabs just like the TC credentials. The user would still be responsible for calling that method as necessary (and calling it in only one tab), but at least then all OIDCCredentialAgents would have the same access_token.

eliperelman commented 6 years ago

OK, let me see what I can do.

eliperelman commented 6 years ago

@djmitche hmm, wouldn't this make usage of accessToken async instead of the sync call it is now?

djmitche commented 6 years ago

@djmitche hmm, wouldn't this make usage of accessToken async instead of the sync call it is now?

specifically the set accessToken method? maybe.. I feel like it still doesn't solve the issue for the user, since they would need to have their own way of communicating "hey I updated the access token" to other tabs. Maybe we should just leave it alone? I don't know :/

eliperelman commented 6 years ago

since they would need to have their own way of communicating "hey I updated the access token" to other tabs

Yeah, I get both the get and the set would have to be async in order for this to become a reality. I agree that if userland is the one overriding the value, then maybe they should be responsible.

Let me play around with this; my experiment on this would be a breaking change.

eliperelman commented 6 years ago

@djmitche how would you feel about moving forward with this as-is for now, and I can tackle further optimizations in a follow-up?

eliperelman commented 6 years ago

Dustin and I have decided that we will not land this until next week to let login stuff stabilize a bit after last week's churn.

djmitche commented 6 years ago

I'll look again in the interim