-
Notifications
You must be signed in to change notification settings - Fork 234
feat(atlas-service): securely persist and restore atlas login state through keychain #4718
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
Conversation
d8f9d58
to
dd9f882
Compare
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.
Looks good so far, only one minor suggestion (can also try that out manually sometime myself)
packages/atlas-service/src/main.ts
Outdated
mongoLogId(1_001_000_210), | ||
'AtlasService', | ||
'Atlas service initialized' | ||
this.initPromise = (async () => { |
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.
Can you do
this.initPromise = (async () => { | |
return this.initPromise ??= (async () => { |
or would the linter complain about that?
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.
Oh, yes, that's way better! I don't think it will complain, I just always forget about null-ish operators!
587de8d
to
2e9f3b9
Compare
@addaleax do you mind giving it another look when you have a moment? I stumbled on some issues with how I was handling the events before when manually testing and so had to update the way refresh works quite a bit |
// We are not using `refresh-succeeded` / `refresh-failed` events | ||
// here because those happen BEFORE `allowedFlows` method is called | ||
// (see oidc-plugin flow diagram) meaning that we need to wait until | ||
// the whole request token flow went through to make sure that sign | ||
// in flow is not allowed while we are refreshing the token |
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.
That's the main change from the previous implementation: to make sure we don't return any allowedFlows
before they are read and can be used on refresh failing we are listening to auth-*
events here now instead of refresh-*
, even though we are waiting specifically for refresh to finish
Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>
This patch adds some logic to atlas-service package that persists and restores serialized auth state of oidc-plugin using system keychain. This means that if you are signed in and close Compass, you don't need to sign in again after re-opening it.
Opening as draft to keep track of progress, as I need to switch to another task. The feature already works and existing tests were adjusted, but I still need to add some new tests for the added functionalityAdded some tests, so this is fully ready for review now