Skip to content

Commit

Permalink
Addressed comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@dhr-verma
dhr-verma committed Dec 26, 2024
1 parent 89e839f commit 8b5a2b6
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 55 deletions.
14 changes: 7 additions & 7 deletions server/routerlicious/packages/routerlicious-base/src/riddler/api.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,12 @@ export function create(
/**
* Updates the keyless access setting for the given tenant
*/
router.put("/tenants/:id/keylessAccess", (request, response) => {
router.put("/tenants/:id/privateKeyAccess", (request, response) => {
const tenantId = request.params.id;
const enableKeylessAccess = request.body.enableKeylessAccess
? request.body.enableKeylessAccess
const enablePrivateKeyAccess = request.body.enablePrivateKeyAccess
? request.body.enablePrivateKeyAccess
: null;
const storageP = manager.updateKeylessAccessPolicy(tenantId, enableKeylessAccess);
const storageP = manager.updatePrivateKeyAccessPolicy(tenantId, enablePrivateKeyAccess);
handleResponse(storageP, response);
});

Expand Down Expand Up @@ -161,15 +161,15 @@ export function create(
const tenantCustomData: ITenantCustomData = request.body.customData
? request.body.customData
: {};
const enableKeylessAccess = request.body.enableKeylessAccess
? request.body.enableKeylessAccess
const enablePrivateKeyAccess = request.body.enablePrivateKeyAccess
? request.body.enablePrivateKeyAccess
: null;
const tenantP = manager.createTenant(
tenantId,
tenantStorage,
tenantOrderer,
tenantCustomData,
enableKeylessAccess,
enablePrivateKeyAccess,
);
handleResponse(tenantP, response);
});
Expand Down
36 changes: 18 additions & 18 deletions server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,12 @@ export class TenantManager {
}
}

private isTenantKeylessAccessEnabled(tenant: ITenantDocument): boolean {
private isTenantPrivateKeyAccessEnabled(tenant: ITenantDocument): boolean {
return tenant.privateKeys ? true : false;
}

// Currently key and secondary are not optional, but this is to make sure we don't break anything if they are optional in the future
private isTenantKeyAccessEnabled(tenant: ITenantDocument): boolean {
private isTenantSharedKeyAccessEnabled(tenant: ITenantDocument): boolean {
return tenant.key || tenant.secondaryKey ? true : false;
}

Expand Down Expand Up @@ -287,8 +287,8 @@ export class TenantManager {
storage: tenant.storage,
customData: tenant.customData,
scheduledDeletionTime: tenant.scheduledDeletionTime,
enableKeylessAccess: this.isTenantKeylessAccessEnabled(tenant),
enableKeyAccess: this.isTenantKeyAccessEnabled(tenant),
enablePrivateKeyAccess: this.isTenantPrivateKeyAccessEnabled(tenant),
enableSharedKeyAccess: this.isTenantSharedKeyAccessEnabled(tenant),
};
}

Expand All @@ -304,8 +304,8 @@ export class TenantManager {
storage: tenant.storage,
customData: tenant.customData,
scheduledDeletionTime: tenant.scheduledDeletionTime,
enableKeylessAccess: this.isTenantKeylessAccessEnabled(tenant),
enableKeyAccess: this.isTenantKeyAccessEnabled(tenant),
enablePrivateKeyAccess: this.isTenantPrivateKeyAccessEnabled(tenant),
enableSharedKeyAccess: this.isTenantSharedKeyAccessEnabled(tenant),
}));
}

Expand Down Expand Up @@ -370,7 +370,7 @@ export class TenantManager {
storage: ITenantStorage,
orderer: ITenantOrderer,
customData: ITenantCustomData,
enableKeylessAccess = false,
enablePrivateKeyAccess = false,
): Promise<ITenantConfig & { key: string }> {
const latestKeyVersion = this.secretManager.getLatestKeyVersion();
// TODO: Make shared tenant keys optional
Expand All @@ -379,7 +379,7 @@ export class TenantManager {
latestKeyVersion,
)) as IPlainTextAndEncryptedTenantKeys;
let privateKeys: ITenantPrivateKeys | undefined;
if (enableKeylessAccess) {
if (enablePrivateKeyAccess) {
privateKeys = (await this.createTenantKeys(
tenantId,
latestKeyVersion,
Expand Down Expand Up @@ -432,9 +432,9 @@ export class TenantManager {
return tenantDocument.storage;
}

public async updateKeylessAccessPolicy(
public async updatePrivateKeyAccessPolicy(
tenantId: string,
enableKeylessAccess: boolean,
enablePrivateKeyAccess: boolean,
): Promise<ITenantConfig> {
const latestKeyVersion = this.secretManager.getLatestKeyVersion();
const tenantDocument = await this.getTenantDocument(tenantId);
Expand All @@ -446,7 +446,7 @@ export class TenantManager {
}

let privateKeys: ITenantPrivateKeys | undefined;
if (enableKeylessAccess) {
if (enablePrivateKeyAccess) {
// If the request is to enable keyless access and the tenant is already a keyless tenant, return the tenant config
if (tenantDocument.privateKeys) {
return {
Expand All @@ -455,8 +455,8 @@ export class TenantManager {
storage: tenantDocument.storage,
customData: tenantDocument.customData,
scheduledDeletionTime: tenantDocument.scheduledDeletionTime,
enableKeylessAccess: true,
enableKeyAccess: this.isTenantKeyAccessEnabled(tenantDocument),
enablePrivateKeyAccess: true,
enableSharedKeyAccess: this.isTenantSharedKeyAccessEnabled(tenantDocument),
};
}
// If the tenant is being converted to a keyless tenant, generate new private keys, otherwise update them to be undefined
Expand All @@ -474,8 +474,8 @@ export class TenantManager {
storage: tenantDocument.storage,
customData: tenantDocument.customData,
scheduledDeletionTime: tenantDocument.scheduledDeletionTime,
enableKeylessAccess: false,
enableKeyAccess: this.isTenantKeyAccessEnabled(tenantDocument),
enablePrivateKeyAccess: false,
enableSharedKeyAccess: this.isTenantSharedKeyAccessEnabled(tenantDocument),
};
}
await this.deleteKeyFromCache(tenantId, true /* deletePrivateKeys */);
Expand All @@ -486,7 +486,7 @@ export class TenantManager {
);
const updatedtenantDocument = await this.getTenantDocument(tenantId);
if (updatedtenantDocument === undefined) {
Lumberjack.error("Could not find tenantId after updating keyless access policy.", {
Lumberjack.error("Could not find tenantId after updating private key access policy.", {
[BaseTelemetryProperties.tenantId]: tenantId,
});
throw new NetworkError(404, `Could not find updated tenant: ${tenantId}`);
Expand All @@ -497,8 +497,8 @@ export class TenantManager {
storage: updatedtenantDocument.storage,
customData: updatedtenantDocument.customData,
scheduledDeletionTime: updatedtenantDocument.scheduledDeletionTime,
enableKeylessAccess: this.isTenantKeylessAccessEnabled(updatedtenantDocument),
enableKeyAccess: this.isTenantKeyAccessEnabled(updatedtenantDocument),
enablePrivateKeyAccess: this.isTenantPrivateKeyAccessEnabled(updatedtenantDocument),
enableSharedKeyAccess: this.isTenantSharedKeyAccessEnabled(updatedtenantDocument),
};
}

Expand Down
7 changes: 5 additions & 2 deletions server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,11 @@ describe("Routerlicious", () => {
it("PUT /tenants/:id/customData", async () => {
await assertCorrelationId(`/api/tenants/${testTenantId}/customData`, "put");
});
it("PUT /tenants/:id/keylessAccess", async () => {
await assertCorrelationId(`/api/tenants/${testTenantId}/keylessAccess`, "put");
it("PUT /tenants/:id/privateKeyAccess", async () => {
await assertCorrelationId(
`/api/tenants/${testTenantId}/privateKeyAccess`,
"put",
);
});
it("PUT /tenants/:id/key", async () => {
await assertCorrelationId(`/api/tenants/${testTenantId}/key`, "put");
Expand Down
44 changes: 22 additions & 22 deletions server/routerlicious/packages/routerlicious-base/src/test/riddler/tenantManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,94 +380,94 @@ describe("TenantManager", () => {
});

describe("updateKeylessAccessPolicy", () => {
it("Should have enableKeylessAccess set to true when policy is already enabled", async () => {
it("Should have enablePrivateKeyAccess set to true when policy is already enabled", async () => {
const findOneStub = sandbox
.stub(tenantRepository, "findOne")
.resolves(tenantWithKeyless);
const updateStub = sandbox.stub(tenantRepository, "update").resolves();

const updatedTenant: ITenantConfig = await tenantManager.updateKeylessAccessPolicy(
const updatedTenant: ITenantConfig = await tenantManager.updatePrivateKeyAccessPolicy(
"cordflasher-dolphin",
true,
);

assert.notStrictEqual(updatedTenant.enableKeylessAccess, undefined);
assert.strictEqual(updatedTenant.enableKeylessAccess, true);
assert.notStrictEqual(updatedTenant.enablePrivateKeyAccess, undefined);
assert.strictEqual(updatedTenant.enablePrivateKeyAccess, true);
sandbox.assert.calledOnce(findOneStub);
sandbox.assert.notCalled(updateStub);
});

it("Should have enableKeylessAccess set to true when the request is to enable it and the policy is not enabled", async () => {
it("Should have enablePrivateKeyAccess set to true when the request is to enable it and the policy is not enabled", async () => {
const mongoFindOneStub = sandbox.stub(tenantRepository, "findOne");
mongoFindOneStub.onFirstCall().resolves(tenantWithoutKeyless);
mongoFindOneStub.onSecondCall().resolves(tenantWithKeyless);
const updateStub = sandbox.stub(tenantRepository, "update").resolves();

const updatedTenant: ITenantConfig = await tenantManager.updateKeylessAccessPolicy(
const updatedTenant: ITenantConfig = await tenantManager.updatePrivateKeyAccessPolicy(
"cordflasher-dolphin",
true,
);

assert.notStrictEqual(updatedTenant.enableKeylessAccess, undefined);
assert.strictEqual(updatedTenant.enableKeylessAccess, true);
assert.notStrictEqual(updatedTenant.enablePrivateKeyAccess, undefined);
assert.strictEqual(updatedTenant.enablePrivateKeyAccess, true);
sandbox.assert.calledTwice(mongoFindOneStub);
sandbox.assert.calledOnce(updateStub);
});

it("Should have enableKeylessAccess set to false when policy is already disabled", async () => {
it("Should have enablePrivateKeyAccess set to false when policy is already disabled", async () => {
const mongoFindOneStub = sandbox
.stub(tenantRepository, "findOne")
.resolves(tenantWithoutKeyless);
const updateStub = sandbox.stub(tenantRepository, "update").resolves();

const updatedTenant: ITenantConfig = await tenantManager.updateKeylessAccessPolicy(
const updatedTenant: ITenantConfig = await tenantManager.updatePrivateKeyAccessPolicy(
"cordflasher-dolphin",
false,
);

assert.notStrictEqual(updatedTenant.enableKeylessAccess, undefined);
assert.strictEqual(updatedTenant.enableKeylessAccess, false);
assert.notStrictEqual(updatedTenant.enablePrivateKeyAccess, undefined);
assert.strictEqual(updatedTenant.enablePrivateKeyAccess, false);
sandbox.assert.calledOnce(mongoFindOneStub);
sandbox.assert.notCalled(updateStub);
});

it("Should have enableKeylessAccess set to false when the request is to disable it and the policy is enabled", async () => {
it("Should have enablePrivateKeyAccess set to false when the request is to disable it and the policy is enabled", async () => {
const mongoFindOneStub = sandbox.stub(tenantRepository, "findOne");
mongoFindOneStub.onFirstCall().resolves(tenantWithKeyless);
mongoFindOneStub.onSecondCall().resolves(tenantWithoutKeyless);
const updateStub = sandbox.stub(tenantRepository, "update").resolves();

const updatedTenant: ITenantConfig = await tenantManager.updateKeylessAccessPolicy(
const updatedTenant: ITenantConfig = await tenantManager.updatePrivateKeyAccessPolicy(
"cordflasher-dolphin",
false,
);

assert.notStrictEqual(updatedTenant.enableKeylessAccess, undefined);
assert.strictEqual(updatedTenant.enableKeylessAccess, false);
assert.notStrictEqual(updatedTenant.enablePrivateKeyAccess, undefined);
assert.strictEqual(updatedTenant.enablePrivateKeyAccess, false);
sandbox.assert.calledTwice(mongoFindOneStub);
sandbox.assert.calledOnce(updateStub);
});
});

describe("getTenant", () => {
it("Should have enableKeylessAccess set to true keyless access it enabled", async () => {
it("Should have enablePrivateKeyAccess set to true keyless access it enabled", async () => {
sandbox.stub(tenantRepository, "findOne").resolves(tenantWithKeyless);
sandbox.stub(tenantRepository, "update").resolves();

const tenant: ITenantConfig = await tenantManager.getTenant("cordflasher-dolphin");

assert.notStrictEqual(tenant.enableKeylessAccess, undefined);
assert.strictEqual(tenant.enableKeylessAccess, true);
assert.notStrictEqual(tenant.enablePrivateKeyAccess, undefined);
assert.strictEqual(tenant.enablePrivateKeyAccess, true);
});

it("Should have enableKeylessAccess set to false when policy is disabled", async () => {
it("Should have enablePrivateKeyAccess set to false when policy is disabled", async () => {
sandbox.stub(tenantRepository, "findOne").resolves(tenantWithoutKeyless);
sandbox.stub(tenantRepository, "update").resolves();

const tenant: ITenantConfig = await tenantManager.getTenant("cordflasher-dolphin");

assert.notStrictEqual(tenant.enableKeylessAccess, undefined);
assert.strictEqual(tenant.enableKeylessAccess, false);
assert.notStrictEqual(tenant.enablePrivateKeyAccess, undefined);
assert.strictEqual(tenant.enablePrivateKeyAccess, false);
});
});

Expand Down
14 changes: 10 additions & 4 deletions server/routerlicious/packages/services-core/src/tenant.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,17 @@ export interface ITenantConfig {

customData: ITenantCustomData;

// Indicates if keyless access is enabled for this tenant.
enableKeylessAccess: boolean;
/**
* Indicates if (not-shared secret) key access is enabled for this tenant.
*/
enablePrivateKeyAccess: boolean;

// Indicates if key access is enabled for this tenant.
enableKeyAccess: boolean;
/**
* Indicates if (shared secret) key access is enabled for this tenant.
* @remarks
* This value is never read and cannot be updated via public APIs in Routerlicious.
*/
enableSharedKeyAccess: boolean;

// Timestamp of when this tenant will be hard deleted.
// The tenant is soft deleted if a deletion timestamp is present.
Expand Down
4 changes: 2 additions & 2 deletions server/routerlicious/packages/test-utils/src/testTenantManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ export class TestTenantManager implements ITenantManager {
orderer: this.tenant.orderer,
key: "test-tenant-key",
customData: {},
enableKeyAccess: true,
enableKeylessAccess: false,
enableSharedKeyAccess: true,
enablePrivateKeyAccess: false,
};
}

Expand Down

0 comments on commit 8b5a2b6

Please sign in to comment.