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

CL-16691: Support Okta Access token #148

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

Conversation

vyerawar
Copy link

No description provided.

@vyerawar vyerawar requested a review from tusharpari June 15, 2022 14:03
@vyerawar
Copy link
Author

vyerawar commented Jun 15, 2022

We can pass okta generated access token to call endpoints
classy.me.retrieve({ token: { // Okta generated access token access_token: access_token, is_okta_token: true } })

@@ -32,7 +32,7 @@ export default function _makeRequest(path, method, headers = {}, form, data = {}
: undefined;

const requestParams = {
baseUrl: this.baseUrl,
baseUrl: baseUrl ? baseUrl : this.baseUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this.baseUrl get set? I'd prefer that we update that value in the appropriate place if config.oktaBaseUrl is passed in.

Copy link
Author

@vyerawar vyerawar Jun 15, 2022

Choose a reason for hiding this comment

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

this.baseUrl is set in src/ClassyResource/main.js. In that we are not getting request payload to check is_okta_token flag.
I am removing this change from here and updating the this.baseUrl value in src/ClassyResource/createMethod.js file. I have updated the code, please review.

@vyerawar vyerawar requested a review from ESoch June 16, 2022 15:39
@ESoch
Copy link
Contributor

ESoch commented Jun 17, 2022

High level these changes look good. We'd want to make sure we have documentation that details what we're doing and why in the README and also comments in the code.


I'm going to hold off on "approving" for now. We'll want to look into how we can potentially publish your fork to our private registry so that we can do tests before we merge.

@vyerawar
Copy link
Author

High level these changes look good. We'd want to make sure we have documentation that details what we're doing and why in the README and also comments in the code.

I'm going to hold off on "approving" for now. We'll want to look into how we can potentially publish your fork to our private registry so that we can do tests before we merge.

Hi @ESoch
README shows all related to app token, there is no anything related to member token and we have updated this for member token.
Also the changes that we are making are part of internal requirements. Can we discuss this with Kate, Arijeet, Stephan to decide if we want this information to be public.
Thank you.

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