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

Sugar client: fix Panoptes authentication #142

Merged
merged 3 commits into from
May 6, 2021
Merged

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented May 6, 2021

Missed in #135: Promise.all should take an array as an argument. I've also added better error-logging to debug Sugar client errors.

Missed in #135: Promise.all should take an array as an argument. I've also added better error-logging to debug Sugar client errors.
@eatyourgreens eatyourgreens requested a review from camallen May 6, 2021 15:39
@eatyourgreens eatyourgreens marked this pull request as draft May 6, 2021 15:39
@eatyourgreens
Copy link
Contributor Author

Marking this as draft because I'm seeing token returned as an empty string here, even when logged in to Panoptes:

Promise.all([auth.checkCurrent(), auth.checkBearerToken()])
.then(function([ user, token ]) {
if (user && token) {

Instead of checking the session and token in parallel, check the token after the user has resolved.
@eatyourgreens
Copy link
Contributor Author

Switching from Promise.all to checking the token after the user resolves seems to work much better.

@eatyourgreens eatyourgreens changed the title Sugar client: fix Promise.all Sugar client: fix Panoptes authentication May 6, 2021
lib/sugar.js Show resolved Hide resolved
sugarClient.authToken = token;
auth.checkBearerToken()
.then(function (token) {
sugarClient.authToken = token;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the sugar.connect() (below) needs to be in this promise then block to ensure the connection is after the token is setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I've fixed that and I'm seeing myself show up under the Active Participants heading on Talk.

Wait for auth to complete before connecting to Sugar.
@eatyourgreens eatyourgreens marked this pull request as ready for review May 6, 2021 19:15
@eatyourgreens eatyourgreens requested a review from camallen May 6, 2021 19:16
Copy link
Contributor

@camallen camallen left a comment

Choose a reason for hiding this comment

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

LGTM

@eatyourgreens eatyourgreens merged commit aea3e85 into master May 6, 2021
@eatyourgreens eatyourgreens deleted the sugar-auth branch May 6, 2021 20:02
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.

2 participants