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

Don't clear oAuth2 session on restart #2701

Conversation

KarolineKarkosch
Copy link
Contributor

Offer button to user where it can be cleared without restart

Closes #1550

@develohpanda develohpanda self-requested a review October 8, 2020 10:27
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice! I generally like this, I think it would be beneficial for folks who have different workflows.

I have a couple of observations from the implementation though, could you please check the behavior? 😄

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Progress! 😄

@develohpanda
Copy link
Contributor

develohpanda commented Oct 8, 2020

Given we want to make it opt-in for people to disable the clearing of the OAuth session on startup, here's what you'll need:

  1. a new property in the settings (use "Follow Redirects" as an example for linking the checkbox to a database property
  2. conditionally clear the token on launch

For the second item: looking at main.development.js (app entry point), there is logic to initialize the database, and then initialize the app in _launchApp().

// When the app is first launched
app.on('ready', async () => {
// Init some important things first
await database.init(models.types());
await errorHandling.init();
await windowUtils.init();
// Init the app
await _trackStats();
await _launchApp();
// Init the rest
await updates.init();
});

Before/during/after the _launchApp() invocation, you should be able to load the setting from the database and clear if necessary.

const settings = await models.settings.getOrCreate();

if (settings.clearOAuthSessionOnStart) {
	clearOAuthSession();
}

Please reach out if you need any help! Thanks for working through this 🤗

@KarolineKarkosch
Copy link
Contributor Author

KarolineKarkosch commented Oct 8, 2020

Given we want to make it opt-in for people to disable the clearing of the OAuth session on startup, here's what you'll need:

1. a new property in the settings (use "Follow Redirects" as an example for linking the checkbox to a [database property](https://github.com/Kong/insomnia/blob/03d83a8f52f9d9f4d4f38e014d406a95ef1d2611/packages/insomnia-app/app/models/settings.js#L86)

2. conditionally clear the token on launch

For the second item: looking at main.development.js (app entry point), there is logic to initialize the database, and then initialize the app in _launchApp().

// When the app is first launched
app.on('ready', async () => {
// Init some important things first
await database.init(models.types());
await errorHandling.init();
await windowUtils.init();
// Init the app
await _trackStats();
await _launchApp();
// Init the rest
await updates.init();
});

Before/during/after the _launchApp() invocation, you should be able to load the setting from the database and clear if necessary.

const settings = await models.settings.getOrCreate();

if (settings.clearOAuthSessionOnStart) {
	clearOAuthSession();
}

Please reach out if you need any help! Thanks for working through this hugs

Thanks for your advice. I tried to implement it as you described but it seems to me as if the task in main.development runs in a different thread than where I am trying to use oauth2/misc.js. When I set the currentSessionId in _launchApp it is still undefined when the OAuth2 window is opened. Also console.logs inside misc.js go into the terminal console when calling functions from launchApp and to the browser console when I later try to access the value which furthers my suspicion that they are running in different environments. Additionally window is not yet defined at that point, so localstorage is not usable at all.
So I moved the initialization from settings to ui/index.js where I saw that for example themes or fonts are initialized from settings as well. I hope that is ok too.
It now works as expected: Depending on the clearOnRestart-Setting the session is deleted or not after starting and when using the sessionClear button the last session is persisted for after restart.

Also I fixed the styling per your suggestion, this is what it currently looks like:

Screenshot from 2020-10-08 21-03-08

@develohpanda
Copy link
Contributor

Also console.logs inside misc.js go into the terminal console when calling functions from launchApp and to the browser console

Nice sleuthing, that seems to make sense. When I was looking through that code, I noticed that windowUtils.init() was also initializing local storage, so thought it should be available at that point. I think main.development.js is the electron entry point and ui/index.js is the entry point for the web application being run in electron, so that might explain the differences.

It looks like you've put it in the right place now! 😃

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Code looks good to me, thank you for working through this!

Comment on lines +14 to +15
// the value of this variable needs to start with 'persist:'
// otherwise sessions won't be persisted over application-restarts
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice comment, some additional context here

Copy link
Contributor

@DMarby DMarby left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for your contribution! 👍

@develohpanda develohpanda merged commit 984881c into Kong:develop Oct 9, 2020
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.

[Improvement] Let users turn off Deletion of OAuth2 Session on restart
6 participants