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

Reimplement refresh token flow #75

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kennyshittu
Copy link
Collaborator

No description provided.

@kennyshittu
Copy link
Collaborator Author

Note: We need to add user_api_offline scope to our prod tableau client before releasing this.

@lydiaguarino
Copy link

I can add the scope. I want to make sure we understand what will happen there, because all users will have to re-grant authorization permission to the client once the scope is added, since the authorization is on a per scope basis. I think the worst that will happen is that some people might need to close and reopen tableau - but I'm not sure what they will see to let them know that's what's going on.

Copy link

@lydiaguarino lydiaguarino left a comment

Choose a reason for hiding this comment

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

I'll add the scope to the client in just a few. One very tiny code comment. I have not pulled this down to test yet, but will do that momentarily. Might reach out if I have trouble getting my local environment set up in a testable state.

@@ -75,12 +75,12 @@ class TableauConnector {
tableau.registerConnector(this.connector)
}

authenticate () {
async authenticate () {

Choose a reason for hiding this comment

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

I think this can be removed, as there is no accompanying await statement.

@lydiaguarino
Copy link

lydiaguarino commented Feb 19, 2019

@andela-Kshittu Can you give me a little bit more background on this solution? I remember there being some reason why we could only store one token (because of the password field required by Tableau) and that we decided to store the refresh token instead of the access token so that we could appropriately request a new token rather than letting the access token expire, leaving the user in a bad state because Tableau doesn't correctly kick you out to the auth flow when your token expires. It looks to me like we are only holding on to the refresh token in this implementation. Does that mean that we are doing some sort of token exchange for the access token on each call to the API, or is that exchange only happening once while the access token is not expired? @tle had some concerns about this option if the implementation is such that we are making frequent exchanges of refresh tokens for access tokens.

@kennyshittu
Copy link
Collaborator Author

@lydiaguarino Yes, we are doing a token exchange for the access token on each call to the API and we decided to go that route for us to be able to support scheduled refreshes on Tableau Server.

@kennyshittu
Copy link
Collaborator Author

I can add the scope. I want to make sure we understand what will happen there, because all users will have to re-grant authorization permission to the client once the scope is added, since the authorization is on a per scope basis. I think the worst that will happen is that some people might need to close and reopen tableau - but I'm not sure what they will see to let them know that's what's going on.

The additional scope (user_api_offline) will appear in the UI during authorization flow.

screenshot 2019-02-20 at 4 13 31 pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants