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

Make refresh token request only if appKey and appSecret is present #81

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

Conversation

mangatinanda
Copy link
Contributor

  • It doesn't make sense for customer facing apps(Ex: Web App) to expose appKey and appSecret.

  • In all such cases, beforeRefresh event should be used to refresh the token instead of SDK making the refresh token request.

  • In our case, we are making a refresh token request to our own server endpoint which in turn makes a request to ringcentral refresh token API endpoint with HTTP Basic scheme using the app key and app secret.

@kirill-konshin
Copy link
Contributor

In all such cases, beforeRefresh event should be used to refresh the token instead of SDK making the refresh token request.

This is core SDK functionality, it works exactly as it was required :)

The change is OK, but it breaks tests, please fix the CI.

@coveralls
Copy link

coveralls commented Sep 20, 2017

Coverage Status

Coverage decreased (-0.05%) to 91.195% when pulling f2a8310 on happyfoxinc:master into aa3c0e1 on ringcentral:master.

@mangatinanda
Copy link
Contributor Author

@kirill-konshin Fixed tests.

"access_token_ttl": this._auth.data().expires_in + 1,
"refresh_token_ttl": this._auth.data().refresh_token_expires_in + 1
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if otherwise? The rest of the function relies on fact that there is a response.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you still interested in this PR? Can you fix it or should I close?

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.

3 participants