feat: handle updating operator api secret #3328

BlairCurrey · 2025-03-04T18:12:34Z

Changes proposed in this pull request

Disallows operator from updating their api secret via admin api (implemented at resolver level, as opposed to service)
Adds method to update operator api secret from config (if it has changed)
Changes tenant create form to submit sections independently, similar to our other create forms
Changes truncateTables interface to remove possibility of truncating wrong schema

Context

fixes #3276

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

…submit sections independently

BlairCurrey · 2025-03-04T18:55:15Z

Intended to add some docs explaining how to update operator api secret and how it would require coordination with the integration server owner (would need to update in tandem). However it seems like there is nothing for multi-tenancy so I wasnt sure where to put it and figure we need to think about docs for mt in general. Brought this up on this in our slack workspace here: https://interledgerfoundation.slack.com/archives/C06KZCKEEKW/p1741114127552369

BlairCurrey · 2025-03-04T18:58:13Z

packages/backend/src/tenants/service.test.ts

@@ -447,3 +446,89 @@ describe('Tenant Service', (): void => {
    })
  })
 })
+
+describe('Tenant Service (no tenant truncate)', (): void => {


This adds an entirely new top-level describe block for the service test which re-inits the dependencies and app and everything.

I did this strictly to manage the truncating of the tenants table differently. I do not want to truncate for these tests, but the other tests do. I was unable to unify them. I think it's OK but just noting because it might not be obvious why this was necessary.

BlairCurrey · 2025-03-04T19:01:09Z

packages/backend/src/tenants/service.ts

+async function updateOperatorApiSecretFromConfig(
+  deps: ServiceDependencies
+): Promise<undefined | TenantError> {
+  const { adminApiSecret, operatorTenantId } = deps.config
+
+  const tenant = await Tenant.query(deps.knex)
+    .findById(operatorTenantId)
+    .whereNull('deletedAt')
+    .first()
+
+  if (!tenant) {
+    return TenantError.TenantNotFound
+  }
+  if (tenant.apiSecret !== adminApiSecret) {
+    await Tenant.query(deps.knex)
+      .patch({ apiSecret: adminApiSecret })
+      .where('id', operatorTenantId)
+  }
+}


New method to update the secret from config. I made a new method rather than using the update because:

I wanted to be opinionated about where the secret and operator id are coming from. This is coming from the config, not just some value being passed in which is intended to be from the config.

I didnt want to update unless its changed and I didnt want to add that logic to the update just for this. Perhaps we could just always write it but figured this was safer.

I was just noticing that this exemplifies the value of the repo layer... notice we use the cache in the service get method but we cant use that here (could if we were getting from the repo). I suppose if thats important I can use/update the cache here as well.

Makes sense to have another method. As far as caching goes, I think we should update it if we end up updating the secret.

Ah, of course. updated.

BlairCurrey · 2025-03-04T19:26:52Z

packages/backend/src/index.ts

+  // Update Operator Tenant from config
+  const tenantService = await container.use('tenantService')
+  const error = await tenantService.updateOperatorApiSecretFromConfig()
+  if (error) throw error
+


This is happening in the start method after we run migrations and start the admin server, etc.

Is this the correct behavior? It will error if the operator id from the env var is not found (tenant service returns an error), or some unknown error throws.

This will show an error log and exit the process. I debated catching and logging at warning level because strictly speaking the app will still work. Operator tenant id isnt (at this point) used for anything else in the app (just used in the initial migration and here). So it can work without this but it could be in an unexpected state (still with the old api secret). You wouldnt know unless dutifully watching the logs.

Thinking about it more, we throw an error when parsing the config if OPERATOR_TENANT_ID is not set. So I think its probably correct to throw here if we know its bad.

…or as needed - previous way of controlling if the request was from an operator or not no longer worked after changing app start to sync apiSecret

…iSecret

BlairCurrey · 2025-03-05T21:25:17Z

packages/backend/src/graphql/resolvers/tenant.test.ts

@@ -85,7 +86,7 @@ describe('Tenant Resolvers', (): void => {
  })

  afterEach(async (): Promise<void> => {
-    await truncateTables(appContainer.knex, true)


This is an example of the problem that lead to changing the truncateTables interface. #3328 (comment)

We used a non-default db schema but then didnt pass it into truncateTables. This test ended up having a side effect of deleting the tenants from other schemas and causing unexpected test failures because now the app looks up the operator tenant on start (to maybe update api secret from config).

BlairCurrey · 2025-03-05T21:38:31Z

packages/backend/src/tests/tableManager.ts

+  deps: IocContract<AppServices>,
+  options?: { truncateTenants?: boolean }


Tightens up the truncateTables interface. Most of the changed files in this PR are just changing how truncateTables is called.

When we added the options to truncate tenants and provide a schema we created an opportunity to truncate tenants from the wrong schema.

For example, here, where we truncated with no dbSchema provided: https://github.com/interledger/rafiki/pull/3328/files#r1982206044

This coincidentally wasnt causing problems as most tests just werent interacting with the tenants. However, with these changes we attempt to fetch the tenant every time the app starts (so every test via createTestApp) which yielded many TenantNotFound errors because another test had dropped them.

This removes the (not completely obvious) responsibility from the caller to pass in dbSchema when truncating tenants. It just uses the config directly to truncate from the same dbSchema that the app is using.

packages/backend/src/tenants/service.ts

mkurapov · 2025-03-06T23:04:19Z

packages/backend/src/tenants/service.ts

+async function updateOperatorApiSecretFromConfig(
+  deps: ServiceDependencies
+): Promise<undefined | TenantError> {
+  const { adminApiSecret, operatorTenantId } = deps.config
+
+  const tenant = await Tenant.query(deps.knex)
+    .findById(operatorTenantId)
+    .whereNull('deletedAt')
+    .first()
+
+  if (!tenant) {
+    return TenantError.TenantNotFound
+  }
+  if (tenant.apiSecret !== adminApiSecret) {
+    await Tenant.query(deps.knex)
+      .patch({ apiSecret: adminApiSecret })
+      .where('id', operatorTenantId)
+  }
+}


Makes sense to have another method. As far as caching goes, I think we should update it if we end up updating the secret.

packages/backend/src/tenants/service.test.ts

Co-authored-by: Max Kurapov <max@interledger.org>

mkurapov · 2025-03-07T17:07:42Z

packages/backend/src/tenants/service.ts

-    await Tenant.query(deps.knex)
-      .patch({ apiSecret: adminApiSecret })
-      .where('id', operatorTenantId)
+    await tenant.$query(deps.knex).patch({ apiSecret: adminApiSecret })


I'm not sure that .$query().patch updates tenant in place (ie, cache still ends up saving the old tenant). Based on the test you added I think await tenantCache.get(tenant.id) is actually hitting the DB directly (since it's the first time it's called)

BlairCurrey added 5 commits February 28, 2025 11:14

feat(frontend): disallow api secret update and change tenant form to …

614c713

…submit sections independently

feat(backend): disallow tenant api secret update in admin api

84edc63

feat(backend): add service method to update secret, call on app start

e881e3d

chore: rm log

7b910d6

fix(fronted): 'no any' lint error

77681a3

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. type: source Changes business logic labels Mar 4, 2025

BlairCurrey commented Mar 4, 2025

View reviewed changes

fix(backend): error on failed secret update from config

8eab244

BlairCurrey commented Mar 4, 2025

View reviewed changes

BlairCurrey added 16 commits March 5, 2025 13:06

fix(backend): wallet address resolver tests to be operator/non-operat…

52fa4a7

…or as needed - previous way of controlling if the request was from an operator or not no longer worked after changing app start to sync apiSecret

test(backend): update tenant tests to reflect operator cant update ap…

45f9eea

…iSecret

chore: fix format

b52208b

chore: debug ci test failure (working locally)

23277d7

chore: fix format

6a28574

chore: debug ci test failure (working locally)

bcba035

chore: debug ci test failure (working locally)

7f568de

chore: debug ci test failure (working locally)

e8d0854

chore: debug ci test failure (working locally)

94dfa00

chore: debug ci test failure (working locally)

dd2cd35

chore: debug ci test failure (working locally)

eb56e82

chore: debug ci test failure (working locally)

3b3ab7a

chore: debug ci test failure (working locally)

c065eaa

chore: debug ci test failure (working locally)

b9b424a

chore: debug ci test failure (working locally)

5d451a4

fix: test side effects

c7399be

BlairCurrey commented Mar 5, 2025

View reviewed changes

BlairCurrey marked this pull request as ready for review March 5, 2025 21:41

BlairCurrey requested review from koekiebox, njlie and mkurapov March 5, 2025 21:41

fix(backend): uncomment test

90bf84c

mkurapov reviewed Mar 6, 2025

View reviewed changes

BlairCurrey and others added 3 commits March 7, 2025 09:43

Update packages/backend/src/tenants/service.ts

eeae561

Co-authored-by: Max Kurapov <max@interledger.org>

fix(backend): rm unushesd nock

844e885

feat(backend): set cache on update operator secret method

9f8f0c1

BlairCurrey requested a review from mkurapov March 7, 2025 15:42

mkurapov reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: handle updating operator api secret #3328

feat: handle updating operator api secret #3328

BlairCurrey commented Mar 4, 2025 •

edited

Loading

BlairCurrey commented Mar 4, 2025 •

edited

Loading

BlairCurrey Mar 4, 2025

BlairCurrey Mar 4, 2025 •

edited

Loading

BlairCurrey Mar 4, 2025

mkurapov Mar 6, 2025

BlairCurrey Mar 7, 2025

BlairCurrey Mar 4, 2025 •

edited

Loading

BlairCurrey Mar 4, 2025

BlairCurrey Mar 5, 2025 •

edited

Loading

BlairCurrey Mar 5, 2025

mkurapov Mar 6, 2025

mkurapov Mar 7, 2025

		deps: IocContract<AppServices>,
		options?: { truncateTenants?: boolean }

feat: handle updating operator api secret #3328

Are you sure you want to change the base?

feat: handle updating operator api secret #3328

Conversation

BlairCurrey commented Mar 4, 2025 • edited Loading

Changes proposed in this pull request

Context

Checklist

BlairCurrey commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Mar 4, 2025 •

edited

Loading

BlairCurrey commented Mar 4, 2025 •

edited

Loading

BlairCurrey Mar 4, 2025 •

edited

Loading

BlairCurrey Mar 4, 2025 •

edited

Loading

BlairCurrey Mar 5, 2025 •

edited

Loading