feat(backend): tenants service (#3123) #3140

njlie · 2024-11-27T18:52:29Z

Changes proposed in this pull request

Adds a TenantService to the backend IoC container
Adds updatedAt and createdAt in the backend tenants table.

Context

Closes #3123.

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)

golobitch

Generally looks ok. Me personally, I would like to see method getPage to be implemented, that would be used for getting list of tenants

golobitch · 2024-11-27T19:28:44Z

packages/backend/migrations/20241125224212_create_tenants_table.js

@@ -8,6 +8,9 @@ exports.up = function (knex) {
    table.string('email').notNullable()
    table.string('publicName')
    table.string('apiSecret')
+
+    table.timestamp('createdAt').defaultTo(knex.fn.now())
+    table.timestamp('updatedAt').defaultTo(knex.fn.now())


Maybe also deletedAt ? So we would leverage soft delete. What do you think?

Makes sense to me

golobitch · 2024-11-27T19:37:33Z

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

+      const apolloSpy = jest.spyOn(apolloClient, 'query')
+      const tenant = await tenantService.get(createdTenant.id)
+      assert.ok(tenant)
+      expect(tenant.id).toEqual(createdTenant.id)


Maybe just

expect(tenant).toEqual(createdTenant)

? Don't know if this works :D

It breaks because the updatedAt for one will always be different from the other, sadly.

Why would updatedAt field be different ? You are creating a tenant and getting back data, and then you are querying the exact same entity ... so it should be the same one? Or am I missing something here?

Nevermind that, I conflated that result with a different test. This will work, but createdTenant just needs to be augmented with the idpSecret and idpConsentUrl.

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

golobitch · 2024-11-27T19:41:23Z

packages/backend/src/tenants/service.ts

+    }
+
+    // TODO: add type to this in https://github.com/interledger/rafiki/issues/3125
+    await deps.apolloClient.mutate({ mutation, variables })


Does mutate method throw error in case of 4xx / 5xx errors ?

It does - a manifestation of this is having to catch 400 responses inside a try/catch block in the tests.

My understanding is this will be a new gql server for service-to-service communication so I guess maybe we can simply return a 400 from that server like this expects, but I just wanted to note that our current resolvers dont behave this way.

For errors we do something like throw new GraphQLError('tenant not found', { extensions: { code: 'NOT_FOUND' } }). I tested an existing one of these on Bruno and see the gql server replies with a 200 and error details in the body like:

{ "errors": [ { "message": "No grant", "locations": [ { "line": 2, "column": 5 } ], "path": [ "grant" ], "extensions": { "code": "NOT_FOUND" } } ], "data": null }

Im not necessarily against doing it a different way, if that seems better. I just wanted to note this.

My assumption is the apolloClient doesnt parse this and throw. Even if it did the behavior should be
correct but the mock would need to be updated still. If the client doesnt do this I think the test would incorrectly pass if we updated the mock to something like the above response because the expects in the catch would never be called. We safeguard against this sort of incorrect pass elsewhere by doing expect.assertions(n). I also wondered if apolloClient parses that sort of 200 response and throws but in any case the test mock would need to be updated.

Thanks for pointing that out. I updated the nocks to return proper error structure. Mercifully, the client still parses that as an error and throws, but it's a better test to have the correct response from the API.

Mercifully, the client still parses that as an error and throws

Good to know, was curious about that

mkurapov · 2024-11-28T12:37:53Z

packages/backend/src/tenants/model.ts

+export type TenantWithIdpConfig = Pick<
+  Tenant,
+  'id' | 'email' | 'apiSecret' | 'publicName' | 'createdAt' | 'updatedAt'
+> & {
+  idpConsentUrl: string
+  idpSecret: string


Do we need idpConsentUrl and idpSecret in backend?

We need this in backend, so that with one call, we add tenant data on both backend, and auth service.

I'm guessing you are referring to this comment @golobitch?

Also, I see, we are not storing the idpConsentUrl and idpSecret in the tenants table, (TenantWithIdpConfig is just a type). At first, not storing the data makes sense to me (we shouldn't care about IDP stuff in backend). However, how would that work for doing pagination over tenants? If we don't store these, then we would need to call getTenant in the auth service for every tenant result

Ah yes, @njlie, @mkurapov has a point here :)

Perhaps we could treat the backend's idpConsentUrl and idpSecret as some sort of cache-like thing. As in for pagination, we return the backend's entries for each tenant row, but in the query for an individual tenant, make the call to the authorization server. Whenever a tenant is mutated such that the IDP fields are changed, it would update the columns in both backend and auth.

but in the query for an individual tenant, make the call to the authorization server

The fields should basically always be the same, so I think calling to the AS isn't needed. The updates shouldn't happen frequently, and if they do, they'll be stored in the backend tenants table anyway as you describe

mkurapov · 2024-11-28T12:39:06Z

test/integration/testenv/cloud-nine-wallet/docker-compose.yml

@@ -31,6 +31,8 @@ services:
      PRIVATE_KEY_FILE: /workspace/private-key.pem
      AUTH_SERVER_INTROSPECTION_URL: http://cloud-nine-wallet-test-auth:3107
      AUTH_SERVER_GRANT_URL: http://cloud-nine-wallet-test-auth:3106
+      AUTH_ADMIN_API_URL: 'http://cloud-nine-wallet-test-auth:3003/graphql'
+      AUTH_ADMIN_API_SECRET: 'test-secret'


Does this need to correspond to auth's ADMIN_API_SECRET? Or will this be a separate API altogether?

@mkurapov I would say that on both of your comments the answer is affirmitive. We need this in backend, so that with one call, we add tenant data on both backend, and auth service.

mkurapov · 2024-12-03T11:58:58Z

packages/backend/src/tenants/service.ts

+    await Tenant.query(trx).patchAndFetchById(id, {
+      deletedAt: new Date(Date.now())
+    })


maybe we can pass through the deletedAt to the mutation, so both tables in backend and auth have the same value

sanducb · 2024-12-03T12:10:39Z

packages/backend/src/tenants/service.ts

+    const mutation = gql`
+      mutation DeleteAuthTenantMutation($input: DeleteTenantInput!) {
+        deleteTenant(input: $input) {
+          sucess


I assume this must be success

It's a basic response, but this could be changed in the PR for the auth graphql API if it is necessary.

koekiebox

LGTM. 🟢

packages/backend/src/tenants/service.ts

mkurapov · 2024-12-05T15:21:11Z

packages/backend/migrations/20241125224212_create_tenants_table.js

@@ -6,8 +6,14 @@ exports.up = function (knex) {
  return knex.schema.createTable('tenants', function (table) {
    table.uuid('id').notNullable().primary()
    table.string('email').notNullable()


lets have email nullable, based on our slack convo?

mkurapov · 2024-12-05T15:24:14Z

packages/backend/migrations/20241125224212_create_tenants_table.js

+    table.string('idpConsentUrl').notNullable()
+    table.string('idpSecret').notNullable()


I think idpConsentUrl and idpSecret can be null, integrators may not actually be integrated with an IDP anyway. (echoing @tadejgolobic on our Tues call)

njlie requested review from koekiebox, BlairCurrey, mkurapov, golobitch, sanducb and oana-lolea November 27, 2024 18:52

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Nov 27, 2024

golobitch reviewed Nov 27, 2024

View reviewed changes

mkurapov reviewed Nov 28, 2024

View reviewed changes

golobitch requested a review from mkurapov November 29, 2024 20:25

njlie force-pushed the nl/3123/backend-tenant-service branch from 68f523a to 3945c35 Compare December 2, 2024 18:24

njlie changed the title ~~feat(backend): tenants table v1 (#3132)~~ feat(backend): tenants service (#3132) Dec 2, 2024

njlie changed the title ~~feat(backend): tenants service (#3132)~~ feat(backend): tenants service (#3123) Dec 2, 2024

njlie linked an issue Dec 2, 2024 that may be closed by this pull request

[Multi-Tenant] backend tenant service #3123

Closed

5 tasks

njlie added 5 commits December 2, 2024 11:35

feat(backend): tenant service

2f74c4e

fix: integration tests

61d45f7

feat: use soft delete

d57bcc6

refactor: compare whole object in test

57a663d

fix: better gql errors in tests

fb3d702

njlie force-pushed the nl/3123/backend-tenant-service branch from 3945c35 to fb3d702 Compare December 2, 2024 19:35

feat: add idp columns to tenant model

e5cc2b5

mkurapov reviewed Dec 3, 2024

View reviewed changes

sanducb reviewed Dec 3, 2024

View reviewed changes

feat: pagination tests, push deletedAt to auth api call

c824c56

BlairCurrey mentioned this pull request Dec 3, 2024

feat: auth service-to-service api #3148

Draft

6 tasks

feat: add cache

9211ca3

njlie requested review from sanducb and golobitch December 3, 2024 22:37

njlie requested a review from mkurapov December 3, 2024 22:37

fix: update localenv environment variables

e2bbc79

koekiebox approved these changes Dec 5, 2024

View reviewed changes

BlairCurrey reviewed Dec 5, 2024

View reviewed changes

packages/backend/src/tenants/service.ts Outdated Show resolved Hide resolved

mkurapov reviewed Dec 5, 2024

View reviewed changes

feat: make some tenants fields optional, small refactors

3625938

njlie requested review from mkurapov and BlairCurrey December 6, 2024 22:53

BlairCurrey approved these changes Dec 7, 2024

View reviewed changes

mkurapov approved these changes Dec 9, 2024

View reviewed changes

njlie merged commit 07630c1 into 2893/multi-tenancy-v1 Dec 9, 2024
24 of 36 checks passed

njlie deleted the nl/3123/backend-tenant-service branch December 9, 2024 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): tenants service (#3123) #3140

feat(backend): tenants service (#3123) #3140

njlie commented Nov 27, 2024

golobitch left a comment

golobitch Nov 27, 2024

njlie Nov 27, 2024

golobitch Nov 27, 2024

njlie Nov 27, 2024

golobitch Nov 27, 2024

njlie Nov 27, 2024

golobitch Nov 27, 2024

njlie Nov 27, 2024

BlairCurrey Dec 2, 2024 •

edited

Loading

njlie Dec 2, 2024

BlairCurrey Dec 3, 2024

mkurapov Nov 28, 2024

mkurapov Dec 2, 2024 •

edited

Loading

golobitch Dec 2, 2024

njlie Dec 2, 2024 •

edited

Loading

mkurapov Dec 3, 2024

mkurapov Nov 28, 2024

golobitch Nov 28, 2024

mkurapov Dec 3, 2024

sanducb Dec 3, 2024

njlie Dec 3, 2024

koekiebox left a comment

mkurapov Dec 5, 2024

mkurapov Dec 5, 2024

		table.string('idpConsentUrl').notNullable()
		table.string('idpSecret').notNullable()

feat(backend): tenants service (#3123) #3140

feat(backend): tenants service (#3123) #3140

Conversation

njlie commented Nov 27, 2024

Changes proposed in this pull request

Context

Checklist

golobitch left a comment

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

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

Choose a reason for hiding this comment

BlairCurrey Dec 2, 2024 • 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

mkurapov Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie Dec 2, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koekiebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Dec 2, 2024 •

edited

Loading

mkurapov Dec 2, 2024 •

edited

Loading

njlie Dec 2, 2024 •

edited

Loading