Removed internal Riddler getKey calls

[dhr-verma]

Description

This PR removes internal calls from Historian and Scribe to Riddler to get tenant keys. Instead, it adds an API in Riddler that signs and returns an access token directly. This is needed to support disabling shared access keys for server tenants to support keyless access. With this change access token signing is Riddler's responsibility. It itself managed and signs tokens using private/shared keys instead of exposing the private keys to other services. To maintain backward compatibility, shared keys can still be retrieved using the getKeys API.

Overall changes summary:

1. Riddler /accesstoken API: This new API will sign an access token for a given tenant. It will decide what keys to use based on the tenant's configuration. If private access keys are enabled, it will prioritize using those keys. Else it will use shared keys.
2. Riddler /keys API: This API has been modified to remove the usePrivateKeys flag. This flag was intended to be for internal API calls to fetch private keys. However, to support disabling key and secondaryKey for a tenant, this flag has been removed. Now this API only returns shared keys. If the tenant does not have shared keys, it returns an object where key1 and key2 are empty.
3. RIddler /tenant/:id API: Adds a new parameter in the create tenant API - enableSharedKeyAccess. When this is false, key and secondaryKey are undefined. Both enableSharedKeyAccess and enablePrivateKeyAccess cannot be false.
4. Riddler /keyaccess API: The /keylessaccess API has been modified and made the /keyaccess API. This API is used to change a given tenant's key access config. It allows to enable/disable shared key access and private key access. In case shared key access is disabled, key and secondaryKey are updated to be undefined. Both private and shared key access cannot be false.
5. Historian and Scribe: Removed internal calls to Riddler's getKeys API. Instead using the signToken API introduced to the ITenantManager interface.

Breaking Changes

Functionally, these changes maintain backward compatibility, i.e. tenants will continue to function the way they have been. However, code wise this introduces some changes that might be breaking:

1. Adds a new API to the ITenantManager interface. This is used to make calls to Riddler to return a signed access token.
2. A package bump is needed in Historian base to consume the latest signToken API changes. Till then the old getKeys API will continue to be used.

Reviewer Guidance

Changes needed to make key and secondaryKey optional in the ITenantDocument interface. General review of any changes that might break backward compatibility.

Copilot reviewed 5 out of 20 changed files in this pull request and generated no comments.

Files not reviewed (15)

• server/routerlicious/packages/routerlicious-base/package.json: Language not supported
• server/routerlicious/packages/services-core/package.json: Language not supported
• server/routerlicious/packages/services/package.json: Language not supported
• server/routerlicious/packages/test-utils/package.json: Language not supported
• server/routerlicious/packages/services/src/test/types/validateServerServicesPrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/riddler/api.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts: Evaluated as low risk
• server/routerlicious/packages/tinylicious/src/services/tenantManager.ts: Evaluated as low risk
• server/routerlicious/packages/test-utils/src/testTenantManager.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/documents.ts: Evaluated as low risk
• server/routerlicious/packages/test-utils/src/test/types/validateServerTestUtilsPrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/services/src/deltaManager.ts: Evaluated as low risk
• server/routerlicious/packages/services-core/src/test/types/validateServerServicesCorePrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/test/types/validateServerRouterliciousBasePrevious.generated.ts: Evaluated as low risk

Comments suppressed due to low confidence (2)

server/routerlicious/packages/services/src/tenant.ts:108

• Ensure that the signToken method is covered by tests, including scenarios where the tenant is not found or shared keys are disabled.

this.signToken(tenantId, documentId, [ScopeType.DocWrite, ScopeType.DocRead, ScopeType.SummaryWrite])

server/routerlicious/packages/services/src/tenant.ts:223

• Ensure that the parameters passed to signToken are correct and that the returned token is used appropriately.

const result = await restWrapper.post<core.IFluidAccessToken>(`${this.endpoint}/api/tenants/${encodeURIComponent(tenantId)}/accesstoken`,

[Copilot]

Copilot reviewed 5 out of 20 changed files in this pull request and generated 1 comment.

Files not reviewed (15)

• server/routerlicious/packages/routerlicious-base/package.json: Language not supported
• server/routerlicious/packages/services-core/package.json: Language not supported
• server/routerlicious/packages/services/package.json: Language not supported
• server/routerlicious/packages/test-utils/package.json: Language not supported
• server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/documents.ts: Evaluated as low risk
• server/routerlicious/packages/services/src/deltaManager.ts: Evaluated as low risk
• server/routerlicious/packages/services-core/src/test/types/validateServerServicesCorePrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts: Evaluated as low risk
• server/routerlicious/packages/services-utils/src/auth.ts: Evaluated as low risk
• server/routerlicious/packages/test-utils/src/test/types/validateServerTestUtilsPrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/routerlicious-base/src/test/types/validateServerRouterliciousBasePrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/services/src/test/types/validateServerServicesPrevious.generated.ts: Evaluated as low risk
• server/routerlicious/packages/services/src/documentManager.ts: Evaluated as low risk
• server/routerlicious/packages/tinylicious/src/services/tenantManager.ts: Evaluated as low risk
• server/routerlicious/packages/services-core/src/tenant.ts: Evaluated as low risk

Comments suppressed due to low confidence (3)

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts:152

• [nitpick] The error message 'Tenant is disabled or does not exist.' could be more specific. Consider changing it to 'Tenant is disabled or does not exist for tenant id ${tenantId}'.

public async signToken(

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts:941

• [nitpick] The error message 'Shared keys are disabled for tenant id ${tenantId}' could be more specific. Consider changing it to 'Shared keys are disabled for tenant id ${tenantId} and cannot be refreshed'.

if (!this.isTenantSharedKeyAccessEnabled(tenantDocument) && !refreshPrivateKey) {

server/routerlicious/packages/services/src/tenant.ts:199

• The new signToken method should have corresponding test cases to ensure its functionality.

public async signToken(

[znewton]

Huge fan of this PR. No major changes to request, looks great

[alexvy86]

Approving for docs. Left a style nit below, but also have a question: the PR description says the /keys API will return a 403 if the tenant does not have shared keys; is that accurate? If so, I'd argue it's an incorrect response. "The HTTP 403 Forbidden error status code means that a server understands a request but refuses to process it", most of the time meaning "you are authenticated but not authorized to do this or ask for this". So if a tenant doesn't have shared keys, I think an empty response seems more appropriate? (for a request that is correctly authenticated and authorized).

[dhr-verma]

Approving for docs. Left a style nit below, but also have a question: the PR description says the /keys API will return a 403 if the tenant does not have shared keys; is that accurate? If so, I'd argue it's an incorrect response. "The HTTP 403 Forbidden error status code means that a server understands a request but refuses to process it", most of the time meaning "you are authenticated but not authorized to do this or ask for this". So if a tenant doesn't have shared keys, I think an empty response seems more appropriate? (for a request that is correctly authenticated and authorized).

That is a great point tbh! I can modify the return to be empty strings for key1 and key2.