-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/implement webchart sso #8
base: main
Are you sure you want to change the base?
Conversation
Hi @wreiske, I have made the changes to the npm package, Request you to kindly merge the PR. |
sessionCookie, | ||
connectTokenRefreshedAt: new Date(), | ||
expiration: new Date(new Date().getTime() + 5 * 60 * 1000), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abroa01 any progress on these?
src/MIEApi.js
Outdated
|
||
const getCookieResponse = await axios.get(`${this.baseUrl}?f=wcrelease&json`, { | ||
headers: { | ||
'User-Agent': 'BlueHive AI (Get x-db_name)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a default of mieapi but allow for an override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abroa01 any progress on this?
src/MIEApi.js
Outdated
|
||
const response = await axios.get(refreshUrl, { | ||
headers: { | ||
'User-Agent': 'BlueHive AI (Refresh Connection)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please do not use this user agent. You should call it something like mieapi and possibly include the version string for the NPM version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (cachedSession && cachedSession.sessionCookie) { | ||
const now = new Date(); | ||
const timeElapsed = now - new Date(cachedSession.connectTokenRefreshedAt); | ||
if (timeElapsed < 5 * 60 * 1000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic code not linked to the other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abroa01 any progress on this?
@abroa01 there are several outstanding code review changes from @horner that are not marked as resolved. Can you take a look at these? It also looks like the MIE api code is duplicated twice in the repo. once here: https://github.com/mieweb/mieapi-js/pull/8/files#diff-a239f18ba4318d1f80aed3fa1d58ca19d3e51e4923091aeb682b25df3b521c44R1 There's also still a dependency here for the BlueHive refresh token... I'm not sure if we want to use this in our official "MIE" api and we should strongly push for using something like oauth or jwt / another form of authentication for our official npm package. |
I have made the following changes -