-
Notifications
You must be signed in to change notification settings - Fork 61
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
Adrienne / Integrate Hydra authentication #821
Adrienne / Integrate Hydra authentication #821
Conversation
a120881
to
157fa60
Compare
const isAuthEnabled = isOAuth2Enabled(); | ||
|
||
if (!isAuthEnabled) ReactDOM.render(<DerivIFrame />, document.getElementById('deriv_iframe')); | ||
}; |
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.
I moved the DerivIframe
component to the javascript
folder so that it can be conditionally rendered to be included/excluded based on the feature flag returned by isOauth2Enabled
.
If the Hydra authentication is enabled, we will not be using the Deriv iframe to sync login states anymore
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.
so we want to disable syncing in certain scenarios?
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 only disable it when the Hydra authentication feature flag is enabled, which we turn on/off from Growthbook
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.
Because one of our aim for this feature is to remove the use of Deriv iframe sync entirely and migrate these syncing logic into the Hydra service, which is on BE's side
isOAuth2Enabled: function() { | ||
return false | ||
} | ||
}); |
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.
@@ -22,6 +23,9 @@ jobs: | |||
uses: "./.github/actions/build" | |||
with: | |||
target: production | |||
RUDDERSTACK_KEY: ${{ vars.RUDDERSTACK_KEY }} | |||
GROWTHBOOK_DECRYPTION_KEY: ${{ secrets.GROWTHBOOK_DECRYPTION_KEY }} |
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.
[Q] why is this on secrets but client key is on vars?
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.
The Rudderstack and Growthbook client keys are public, you can see them on the network requests and window._growthbook
instances, but only the decryption keys are private
export const getOAuthOrigin = () => { | ||
const { appId, serverUrl } = getServerInfo(); | ||
|
||
const oauthUrl = appId && serverUrl ? `https://${serverUrl}` : DEFAULT_OAUTH_ORIGIN_URL; |
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.
[Q] whats appId check used here for?
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.
Its just an extra check to ensure we have the app ID and server URL before returning the origin url
src/javascript/_common/auth.js
Outdated
const allowedOrigin = getOAuthOrigin(); | ||
if (allowedOrigin === event.origin) { | ||
if (event.data === 'logout_complete') { | ||
await onWSLogoutAndRedirect(); |
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.
[R] please wrap await function in a try catch block
src/javascript/_common/auth.js
Outdated
|
||
setTimeout(() => { | ||
onWSLogoutAndRedirect(); | ||
}, 10000); |
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.
[M] static data can be put in a constant variable on top of the file
let checksCounter = 0; | ||
const analyticsInterval = setInterval(() => { | ||
// Check if the analytics instance is available for 10 seconds before setting the feature flag value | ||
if (checksCounter > 20) { |
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.
not sure why is this required, but i see that this is carried over implementation.
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.
No this hook file is no longer needed, will remove it for now 😅
setIsGBLoaded(true); | ||
clearInterval(analyticsInterval); | ||
} | ||
}, 500); |
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.
[M] this can become 1000 and checksCounter > 10?
src/javascript/_common/auth.js
Outdated
|
||
const serverUrl = LocalStorageUtils.getValue(LocalStorageConstants.configServerURL) || localStorage.getItem('config.server_url') || 'oauth.deriv.com'; | ||
|
||
const appId = LocalStorageUtils.getValue(LocalStorageConstants.configAppId); |
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.
[Q] is the value always available that we don't need a fallback?
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.
LGTM!
…trader into setup-hydra-feature
…trader into setup-hydra-feature
Preview Link: https://pr-821.smarttrader-preview.pages.dev
|
Changes:
@deriv-com/analytics
to integrate feature toggling from Growthbook for enabling/disabling Hydra authentication flow@deriv-com/auth-client
to integrate the new Hydra authentication flowlocalstorage-sync
to be enabled/disabled based on the feature flag from GrowthbookType of change