Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(backend): tenanted assets #3206

Merged
merged 23 commits into from
Jan 15, 2025
Merged

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Jan 8, 2025

Changes proposed in this pull request

  • Backfills existing assets with operator tenant id
  • Adds tenantId to asset model
  • Requires tenantId in aset delete, update, getPage methods (optional in get)
  • Adds separate tenant Admin API middleware for tests
  • Adds a config value to allow setting/creating custom knex/db schema

Checking the correct asset is used during wallet address creation will be done in #3114

Context

Fixes #3033

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)

@mkurapov mkurapov changed the base branch from main to 2893/multi-tenancy-v1 January 8, 2025 20:47
@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Jan 8, 2025
Copy link

netlify bot commented Jan 8, 2025

Deploy Preview for brilliant-pasca-3e80ec ready!

Name Link
🔨 Latest commit 2edea03
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/677ee469d03401000821f5dd
😎 Deploy Preview https://deploy-preview-3206--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -14,7 +14,8 @@ export async function truncateTables(
'knex_migrations',
'knex_migrations_lock',
'knex_migrations_backend',
'knex_migrations_backend_lock'
'knex_migrations_backend_lock',
'tenants' // So we do not truncate the operator tenant
Copy link
Contributor Author

@mkurapov mkurapov Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise, we lose the operator tenant in the tenants table, and we can't insert any assets (leading to almost all tests to fail, basically).

@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 8cfb09f to c2a2563 Compare January 8, 2025 22:23
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from c2a2563 to 25f6248 Compare January 8, 2025 22:54
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 25f6248 to 5dce25b Compare January 9, 2025 13:53
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 7696f5e to 5a31294 Compare January 9, 2025 14:02
Comment on lines +436 to +442
// For tests, we still need to get the tenant in the middleware, but
// we don't need to verify the signature nor prevent replay attacks
koa.use(
this.config.env !== 'test'
? tenantSignatureMiddleware
: testTenantSignatureMiddleware
)
Copy link
Contributor Author

@mkurapov mkurapov Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, had an issue where the replay attack prevention via redis (ie canApiSignature function) was resulting a 401 Unauthorized error for many resolver tests (for cases where we call the same resolver back to back)

@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 6399d96 to b4ad6b7 Compare January 9, 2025 23:49
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from b4ad6b7 to 557e78a Compare January 9, 2025 23:50
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 6538887 to f3eadec Compare January 10, 2025 17:29
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 51ea55e to 0fe4d19 Compare January 10, 2025 19:26
@mkurapov mkurapov force-pushed the 3033/mk/tenanted-assets branch from 0fe4d19 to d3fa2b2 Compare January 10, 2025 19:28
assetService.getPage({
pagination,
sortOrder,
tenantId: ctx.tenant.id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing in tenant id

@@ -42,9 +43,13 @@ describe('Tenant Service', (): void => {
let config: IAppConfig
let apolloClient: ApolloClient<NormalizedCacheObject>
let knex: Knex
const dbSchema = 'tenant_service_test_schema'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows us to have the tenant service tests run cleanly without dropping the tenants table other tests are using

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my PR, I have updated the truncateTables to not delete the tenant.
#3152

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. I also added a way to configure db schema, because for the tenant service tests, we do want to truncate the tenants table. But then of course, we break all of the other tests :) so I needed to isolate it somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that. I will merge yours into mine @mkurapov

@mkurapov mkurapov marked this pull request as ready for review January 13, 2025 13:22
Copy link
Collaborator

@koekiebox koekiebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. Have a couple of queries regarding conflicting functionality.

getPage: (pagination: Pagination, sortOrder?: SortOrder) =>
assetService.getPage({
pagination,
sortOrder,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkurapov Will you have a look at what I said here:
#3152 (comment)

We may want to use that mechanism of being able to provide alternative tenantId for the operator?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might even be worth allowing no tenant filtering if the requester is an operator

const asset = await assetService.getByCodeAndScale({
code: args.code,
scale: args.scale,
tenantId: ctx.tenant.id
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also see:
#3152 (comment)

@@ -93,7 +105,8 @@ export const updateAsset: MutationResolvers<ApolloContext>['updateAsset'] =
const assetOrError = await assetService.update({
id: args.input.id,
withdrawalThreshold: args.input.withdrawalThreshold ?? null,
liquidityThreshold: args.input.liquidityThreshold ?? null
liquidityThreshold: args.input.liquidityThreshold ?? null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see: #3152 (comment)

@@ -168,6 +181,7 @@ export const deleteAsset: MutationResolvers<ApolloContext>['deleteAsset'] =
const assetService = await ctx.container.use('assetService')
const assetOrError = await assetService.delete({
id: args.input.id,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see: #3152 (comment)

@@ -42,9 +43,13 @@ describe('Tenant Service', (): void => {
let config: IAppConfig
let apolloClient: ApolloClient<NormalizedCacheObject>
let knex: Knex
const dbSchema = 'tenant_service_test_schema'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my PR, I have updated the truncateTables to not delete the tenant.
#3152

@mkurapov mkurapov linked an issue Jan 13, 2025 that may be closed by this pull request
3 tasks
# Conflicts:
#	packages/backend/src/app.ts
#	packages/backend/src/tenants/service.test.ts
@mkurapov mkurapov merged commit 0b6fb1a into 2893/multi-tenancy-v1 Jan 15, 2025
30 of 36 checks passed
@mkurapov mkurapov deleted the 3033/mk/tenanted-assets branch January 15, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Multi-Tenant] Tenanted Assets
4 participants