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.

Sign up for GitHub

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

Jump to bottom

feat(backend): add tenant name #2937

Open
wants to merge 3 commits into
base: 2893-multi-tenant-rafiki
Choose a base branch
from
Open

feat(backend): add tenant name #2937

wants to merge 3 commits into from

Conversation

golobitch
Copy link
Collaborator

Changes proposed in this pull request

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@golobitch golobitch requested a review from njlie September 2, 2024 21:11
@golobitch golobitch self-assigned this Sep 2, 2024
@github-actions 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 pkg: mock-ase pkg: mock-account-service-lib labels Sep 2, 2024
@golobitch golobitch force-pushed the feature/2932-multitenant-display-name branch from 7740234 to 1fd2431 Compare September 4, 2024 21:32
@mkurapov
Copy link
Contributor

mkurapov commented Sep 6, 2024

Some generic comments:

  • Are you thinking we need a separate tenantEndpoints table in order to track changes/deletions? Should the same be done for the tenant endpoints in auth?
  • We probably don't need tenantId for grants in backend, since I think it is tenant agnostic (the backend grants are the stored grants that the Rafiki instance itself has gotten from Open Payment requests it has made to remote/other Rafiki instances)

mkurapov
mkurapov reviewed Sep 6, 2024
View reviewed changes
packages/backend/src/tenant/service.ts
const tenantData = {
name: options.name,
kratosIdentityId: uuidv4(),
endpoints: options.endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

this allows Objection to automagically create the endpoints in the tenantsEndpoints model, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct

@golobitch
Copy link
Collaborator Author

Some generic comments:

  • Are you thinking we need a separate tenantEndpoints table in order to track changes/deletions? Should the same be done for the tenant endpoints in auth?
  • We probably don't need tenantId for grants in backend, since I think it is tenant agnostic (the backend grants are the stored grants that the Rafiki instance itself has gotten from Open Payment requests it has made to remote/other Rafiki instances)

This design is a little bit premature. We should change tenantEndpoints to tenantConfiguration. Down the line we can have basically all ENVs that are tenant specific there. And that can be then configured from Admin UI. I think that we should have audit trail for data change on the tenant, but audit is completely different feature and I think that we will need to tackle that sooner or later.

TenantId is only mandatory for auth in order to get correct grant for tenant.

@mkurapov mkurapov linked an issue Oct 1, 2024 that may be closed by this pull request
Add a public name to tenants #2932
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkurapov mkurapov mkurapov left review comments

@njlie njlie Awaiting requested review from njlie

Assignees

@golobitch golobitch

Labels
pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: mock-account-service-lib pkg: mock-ase type: source Changes business logic type: tests Testing related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a public name to tenants
2 participants
@golobitch @mkurapov