Skip to content

Commit

Permalink
[AXON-17] (WIP) fix weird site list behaviour
Browse files Browse the repository at this point in the history
  • Loading branch information
@sdzh-atlassian
sdzh-atlassian committed Jan 21, 2025
1 parent ddb7197 commit 07415bd
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 43 deletions.
5 changes: 5 additions & 0 deletions src/atlclients/authInfo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface RemoveAuthInfoEvent extends AuthInfoEvent {
type: AuthChangeType.Remove;
product: Product;
credentialId: string;
host: string;
}

export interface Product {
Expand Down Expand Up @@ -120,6 +121,10 @@ export interface DetailedSiteInfo extends SiteInfo {
credentialId: string;
}

export function getSiteInfoKey(site: DetailedSiteInfo): string {
return `${site.product.key}:${site.host}:${site.credentialId}`;
}

// You MUST send source
// You SHOULD send both AAID and Anonymous ID when available (if only one is available, send that)
// Anonymous ID should match the ID sent to amplitude for analytics events
Expand Down
71 changes: 30 additions & 41 deletions src/atlclients/authStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
UpdateAuthInfoEvent,
emptyAuthInfo,
getSecretForAuthInfo,
getSiteInfoKey,
isOAuthInfo,
oauthProviderForSite,
} from './authInfo';
Expand Down Expand Up @@ -91,7 +92,7 @@ export class CredentialManager implements Disposable {
}
}

this._memStore.set(site.product.key, productAuths.set(site.credentialId, info));
this._memStore.set(site.product.key, productAuths.set(getSiteInfoKey(site), info));

const hasNewInfo =
!existingInfo ||
Expand All @@ -107,7 +108,7 @@ export class CredentialManager implements Disposable {
}

try {
this.addSiteInformationToSecretStorage(site.product.key, site.credentialId, info);
this.addSiteInformationToSecretStorage(site, info);
const updateEvent: UpdateAuthInfoEvent = { type: AuthChangeType.Update, site: site };
this._onDidAuthChange.fire(updateEvent);
} catch (e) {
Expand All @@ -124,8 +125,8 @@ export class CredentialManager implements Disposable {
let foundInfo: AuthInfo | undefined = undefined;
let productAuths = this._memStore.get(site.product.key);

if (allowCache && productAuths && productAuths.has(site.credentialId)) {
foundInfo = productAuths.get(site.credentialId);
if (allowCache && productAuths && productAuths.has(getSiteInfoKey(site))) {
foundInfo = productAuths.get(getSiteInfoKey(site));
if (foundInfo) {
// clone the object so editing it and saving it back doesn't trip up equality checks
// in saveAuthInfo
Expand All @@ -135,24 +136,20 @@ export class CredentialManager implements Disposable {

if (!foundInfo) {
try {
let infoEntry = await this.getAuthInfoFromSecretStorage(site.product.key, site.credentialId);
let infoEntry = await this.getAuthInfoFromSecretStorage(site);
// if no authinfo found in secretstorage
if (!infoEntry) {
// we first check if keychain exists and if it does then we migrate users from keychain to secretstorage
// without them having to relogin manually
if (keychain) {
infoEntry = await this.getAuthInfoFromKeychain(site.product.key, site.credentialId);
infoEntry = await this.getAuthInfoFromKeychain(site);
if (infoEntry) {
Logger.debug(
`adding info from keychain to secretstorage for product: ${site.product.key} credentialID: ${site.credentialId}`,
);
await this.addSiteInformationToSecretStorage(
site.product.key,
site.credentialId,
infoEntry,
);
await this.addSiteInformationToSecretStorage(site, infoEntry);
// Once authinfo has been stored in the secretstorage, info in keychain is no longer needed so removing it
await this.removeSiteInformationFromKeychain(site.product.key, site.credentialId);
await this.removeSiteInformationFromKeychain(site);
} else if (Container.siteManager.getSiteForId(site.product, site.id)) {
// if keychain does not have any auth info for the current site but the site has been saved, we need to remove it
Logger.debug(
Expand All @@ -179,7 +176,7 @@ export class CredentialManager implements Disposable {
}
}
if (infoEntry && productAuths) {
this._memStore.set(site.product.key, productAuths.set(site.credentialId, infoEntry));
this._memStore.set(site.product.key, productAuths.set(getSiteInfoKey(site), infoEntry));

foundInfo = infoEntry;
}
Expand Down Expand Up @@ -209,54 +206,48 @@ export class CredentialManager implements Disposable {
}
}

private async addSiteInformationToSecretStorage(productKey: string, credentialId: string, info: AuthInfo) {
private async addSiteInformationToSecretStorage(site: DetailedSiteInfo, info: AuthInfo) {
await this._queue.add(
async () => {
try {
await Container.context.secrets.store(`${productKey}-${credentialId}`, JSON.stringify(info));
await Container.context.secrets.store(getSiteInfoKey(site), JSON.stringify(info));
} catch (e) {
Logger.error(e, `Error writing to secretstorage`);
}
},
{ priority: Priority.Write },
);
}
private async getSiteInformationFromSecretStorage(
productKey: string,
credentialId: string,
): Promise<string | undefined> {
private async getSiteInformationFromSecretStorage(site: DetailedSiteInfo): Promise<string | undefined> {
let info: string | undefined = undefined;
await this._queue.add(
async () => {
info = await Container.context.secrets.get(`${productKey}-${credentialId}`);
info = await Container.context.secrets.get(getSiteInfoKey(site));
},
{ priority: Priority.Read },
);
return info;
}
private async removeSiteInformationFromSecretStorage(productKey: string, credentialId: string): Promise<boolean> {
private async removeSiteInformationFromSecretStorage(site: DetailedSiteInfo): Promise<boolean> {
let wasKeyDeleted = false;
await this._queue.add(
async () => {
const storedInfo = await Container.context.secrets.get(`${productKey}-${credentialId}`);
const storedInfo = await Container.context.secrets.get(getSiteInfoKey(site));
if (storedInfo) {
await Container.context.secrets.delete(`${productKey}-${credentialId}`);
await Container.context.secrets.delete(getSiteInfoKey(site));
wasKeyDeleted = true;
}
},
{ priority: Priority.Write },
);
return wasKeyDeleted;
}
private async removeSiteInformationFromKeychain(productKey: string, credentialId: string): Promise<boolean> {
private async removeSiteInformationFromKeychain(site: DetailedSiteInfo): Promise<boolean> {
let wasKeyDeleted = false;
await this._queue.add(
async () => {
if (keychain) {
wasKeyDeleted = await keychain.deletePassword(
keychainServiceNameV3,
`${productKey}-${credentialId}`,
);
wasKeyDeleted = await keychain.deletePassword(keychainServiceNameV3, getSiteInfoKey(site));
}
},
{ priority: Priority.Write },
Expand All @@ -265,13 +256,14 @@ export class CredentialManager implements Disposable {
}

private async getAuthInfoFromSecretStorage(
productKey: string,
credentialId: string,
site: DetailedSiteInfo,
serviceName?: string,
): Promise<AuthInfo | undefined> {
Logger.debug(`Retrieving secretstorage info for product: ${productKey} credentialID: ${credentialId}`);
Logger.debug(
`Retrieving secretstorage info for product: ${site.product.key} credentialID: ${site.credentialId}`,
);
let authInfo: string | undefined = undefined;
authInfo = await this.getSiteInformationFromSecretStorage(productKey, credentialId);
authInfo = await this.getSiteInformationFromSecretStorage(site);
if (!authInfo) {
return undefined;
}
Expand All @@ -283,12 +275,8 @@ export class CredentialManager implements Disposable {
}
return info;
}
private async getAuthInfoFromKeychain(
productKey: string,
credentialId: string,
serviceName?: string,
): Promise<AuthInfo | undefined> {
Logger.debug(`Retrieving keychain info for product: ${productKey} credentialID: ${credentialId}`);
private async getAuthInfoFromKeychain(site: DetailedSiteInfo, serviceName?: string): Promise<AuthInfo | undefined> {
Logger.debug(`Retrieving keychain info for product: ${site.product.key} credentialID: ${site.credentialId}`);
let svcName = keychainServiceNameV3;

if (serviceName) {
Expand All @@ -299,7 +287,7 @@ export class CredentialManager implements Disposable {
await this._queue.add(
async () => {
if (keychain) {
authInfo = await keychain.getPassword(svcName, `${productKey}-${credentialId}`);
authInfo = await keychain.getPassword(svcName, getSiteInfoKey(site));
}
},
{ priority: Priority.Read },
Expand Down Expand Up @@ -360,11 +348,11 @@ export class CredentialManager implements Disposable {
let wasKeyDeleted = false;
let wasMemDeleted = false;
if (productAuths) {
wasMemDeleted = productAuths.delete(site.credentialId);
wasMemDeleted = productAuths.delete(getSiteInfoKey(site));
this._memStore.set(site.product.key, productAuths);
}

wasKeyDeleted = await this.removeSiteInformationFromSecretStorage(site.product.key, site.credentialId);
wasKeyDeleted = await this.removeSiteInformationFromSecretStorage(site);
if (wasMemDeleted || wasKeyDeleted) {
const cmdctx = this.commandContextFor(site.product);
if (cmdctx) {
Expand All @@ -377,6 +365,7 @@ export class CredentialManager implements Disposable {
type: AuthChangeType.Remove,
product: site.product,
credentialId: site.credentialId,
host: site.host,
};
this._onDidAuthChange.fire(removeEvent);

Expand Down
9 changes: 8 additions & 1 deletion src/react/atlascode/config/auth/SiteList.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,14 @@ function generateListItems(
<EditIcon fontSize="small" color="inherit" />
</IconButton>
)}
<IconButton edge="end" aria-label="delete" onClick={() => logout(swa.site)}>
<IconButton
edge="end"
aria-label="delete"
onClick={() => {
console.log('Logging out!', swa.site);
return logout(swa.site);
}}
>
<DeleteIcon fontSize="small" color="inherit" />
</IconButton>
</ListItemSecondaryAction>
Expand Down
4 changes: 3 additions & 1 deletion src/siteManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ export class SiteManager extends Disposable {

onDidAuthChange(e: AuthInfoEvent) {
if (isRemoveAuthEvent(e)) {
const deadSites = this.getSitesAvailable(e.product).filter((site) => site.credentialId === e.credentialId);
const deadSites = this.getSitesAvailable(e.product).filter(
(site) => site.credentialId === e.credentialId && site.host === e.host,
);
deadSites.forEach((s) => this.removeSite(s));
if (deadSites.length > 0) {
this._onDidSitesAvailableChange.fire({
Expand Down

0 comments on commit 07415bd

Please sign in to comment.