-
-
Notifications
You must be signed in to change notification settings - Fork 732
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
Chore/credential migration #2562
base: master
Are you sure you want to change the base?
Changes from all commits
c1c8d9a
546a81b
6f39f08
5c96f22
567997e
97c59c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
const { ipcRenderer } = require('electron') | ||
const PasswordManagers = require('passwordManager/passwordManager.js') | ||
const places = require('places/places.js') | ||
const settings = require('util/settings/settings.js') | ||
|
||
class PasswordMigrator { | ||
#currentVersion = 2; | ||
|
||
constructor() { | ||
this.startMigration() | ||
} | ||
|
||
async _isOutdated(version) { | ||
return version < this.#currentVersion | ||
} | ||
|
||
async _getInUseCredentialVersion() { | ||
const version = await ipcRenderer.invoke('credentialStoreGetVersion') | ||
return version | ||
} | ||
|
||
async startMigration() { | ||
const inUseVersion = await this._getInUseCredentialVersion() | ||
const isOutdated = await this._isOutdated(inUseVersion) | ||
if (!isOutdated) return | ||
|
||
try { | ||
if (inUseVersion === 1 && this.#currentVersion === 2) { | ||
await this.migrateVersion1to2() | ||
console.log('[PasswordMigrator]: Migration complete.') | ||
return | ||
} | ||
} catch (error) { | ||
console.error('Error during password migration:', error) | ||
} | ||
} | ||
|
||
async migrateVersion1to2() { | ||
console.log('[PasswordMigrator]: Migrating keychain data to version', this.#currentVersion) | ||
|
||
const passwordManager = await PasswordManagers.getConfiguredPasswordManager() | ||
if (!passwordManager || !passwordManager.getAllCredentials) { | ||
throw new Error('Incompatible password manager') | ||
} | ||
|
||
const historyData = await places.getAllItems() | ||
const currentCredentials = await passwordManager.getAllCredentials() | ||
console.log('[PasswordMigrator]: Found', historyData.length, 'history entries', historyData) | ||
console.log('[PasswordMigrator]: Found', currentCredentials.length, 'credentials in the current password manager', currentCredentials) | ||
|
||
function createNewCredential(credential) { | ||
// 1) check if the saved url has been visited, if so use that url | ||
const historyEntry = historyData.find(entry => new URL(entry.url).host.replace(/^(https?:\/\/)?(www\.)?/, '') === credential.domain.replace(/^(https?:\/\/)?(www\.)?/, '')) | ||
if (historyEntry) { | ||
return { | ||
username: credential.username, | ||
password: credential.password, | ||
url: new URL(historyEntry.url).origin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this field is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, we could migrate it to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, if you want to update the code to actually save the entire URL, that's fine too. If you want to just save the origin, then we should call the field |
||
} | ||
} | ||
|
||
// 2) check if domain has protocol, if not, add 'https://' | ||
if (!newUrl.startsWith('http://') && !newUrl.startsWith('https://')) { | ||
newUrl = `https://${newUrl}` | ||
} | ||
|
||
return { | ||
username: credential.username, | ||
password: credential.password, | ||
url: newUrl | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing here: The name of this field is |
||
}; | ||
} | ||
|
||
const migratedCredentials = currentCredentials.map(createNewCredential) | ||
console.log('[PasswordMigrator]: Migrated', migratedCredentials.length, 'credentials', migratedCredentials); | ||
|
||
const neverSavedCredentials = settings.get('passwordsNeverSaveDomains') || [] | ||
console.log('[PasswordMigrator]: Found', neverSavedCredentials.length, 'never-saved credentials', neverSavedCredentials) | ||
const migratedNeverSavedCredentials = neverSavedCredentials.map(createNewCredential) | ||
settings.set('passwordsNeverSaveDomains', migratedNeverSavedCredentials) | ||
console.log('[PasswordMigrator]: Migrated', migratedNeverSavedCredentials.length, 'never-saved credentials', migratedNeverSavedCredentials) | ||
|
||
await ipcRenderer.invoke('credentialStoreSetPasswordBulk', migratedCredentials) | ||
|
||
// finally upate the version | ||
await ipcRenderer.invoke('credentialStoreSetVersion', this.#currentVersion) | ||
} | ||
} | ||
|
||
function initialize() { | ||
new PasswordMigrator() | ||
} | ||
|
||
module.exports = { initialize } |
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.
Shouldn't we be saving an origin or a URL into
currentDomain
(which later gets passed tosaveCredential
)? With this way, I think we're not recording the protocol.In my branch I addressed this by passing the entire URL into the manager: master...password-url-matching#diff-dfe869359e479c9fd76fbe8e10c5f46545b3ec71d7a029aafd096869489da6ed
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.
In the passwordFill.js we pass the
origin
part of the url instead of thehostname
which will give us the whole base urlhttps://sub.example.com
and that should be sufficient for thesaveCredential
funtionality.Maybe the
domain
variable name is a little confusing, perhaps it needs to be something likeurlBase
orurlOrigin
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.
Got it, sorry for the delay. It sounds like something with "origin" would be a better name then.