Disable automatic tenant registration by default#891
Conversation
…_register in tenant configuration
WalkthroughAutomatic tenant registration is changed from enabled to disabled by default. A migration updates existing tenant configuration values and defines rollback behavior, while the environment fallback is changed to ChangesTenant auto-registration defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js (1)
7-11: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider hardcoding the
upmigration default for determinism.The
upmigration usescommon.DEFAULT_TENANT_CONFIGURATIONas the columndefaultValue, which is environment-dependent (process.env.DEFAULT_AUTO_REGISTER === 'true'). IfDEFAULT_AUTO_REGISTER=trueis explicitly set in the environment at migration time, the column default would haveauto_register: true, contradicting the migration's intent. Thedownmigration already hardcodes its default — theupshould follow the same pattern for consistency and determinism.♻️ Proposed fix to hardcode the `up` default
up: async (queryInterface, Sequelize) => { await queryInterface.changeColumn('tenants', 'configuration', { type: Sequelize.JSONB, allowNull: false, - defaultValue: common.DEFAULT_TENANT_CONFIGURATION, + defaultValue: { + allowed_auth_mode: process.env.DEFAULT_ALLOWED_AUTH_MODES.split(','), + auto_register: false, + }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js` around lines 7 - 11, Update the up migration’s changeColumn call for tenants.configuration to use a hardcoded default configuration with auto_register set to false instead of common.DEFAULT_TENANT_CONFIGURATION. Keep the existing JSONB type and non-null constraint unchanged, matching the deterministic default used by the down migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js`:
- Around line 20-27: Update the down migration’s changeColumn default
configuration to source allowed_auth_mode from
common.DEFAULT_TENANT_CONFIGURATION, matching the up migration and preserving
the environment-dependent DEFAULT_ALLOWED_AUTH_MODES value; keep the existing
auto_register setting unchanged.
---
Nitpick comments:
In
`@src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js`:
- Around line 7-11: Update the up migration’s changeColumn call for
tenants.configuration to use a hardcoded default configuration with
auto_register set to false instead of common.DEFAULT_TENANT_CONFIGURATION. Keep
the existing JSONB type and non-null constraint unchanged, matching the
deterministic default used by the down migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3ba2f83-26ba-4ba1-a710-c66d2b3c82a3
📒 Files selected for processing (2)
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.jssrc/envVariables.js
| await queryInterface.changeColumn('tenants', 'configuration', { | ||
| type: Sequelize.JSONB, | ||
| allowNull: false, | ||
| defaultValue: { | ||
| allowed_auth_mode: ['otp', 'password'], | ||
| auto_register: true, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
down migration hardcodes allowed_auth_mode instead of using the env-dependent source.
The up migration sets defaultValue: common.DEFAULT_TENANT_CONFIGURATION, where allowed_auth_mode comes from process.env.DEFAULT_ALLOWED_AUTH_MODES.split(','). The down migration hardcodes ['otp', 'password'] instead. If the environment configures different allowed auth modes, the rollback will set an incorrect column default, breaking the contract validated by src/validators/v1/tenant.js:31-42.
Use the same source for allowed_auth_mode in both directions:
♻️ Proposed fix for `down` migration default
down: async (queryInterface, Sequelize) => {
await queryInterface.changeColumn('tenants', 'configuration', {
type: Sequelize.JSONB,
allowNull: false,
defaultValue: {
- allowed_auth_mode: ['otp', 'password'],
+ allowed_auth_mode: process.env.DEFAULT_ALLOWED_AUTH_MODES.split(','),
auto_register: true,
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await queryInterface.changeColumn('tenants', 'configuration', { | |
| type: Sequelize.JSONB, | |
| allowNull: false, | |
| defaultValue: { | |
| allowed_auth_mode: ['otp', 'password'], | |
| auto_register: true, | |
| }, | |
| }) | |
| await queryInterface.changeColumn('tenants', 'configuration', { | |
| type: Sequelize.JSONB, | |
| allowNull: false, | |
| defaultValue: { | |
| allowed_auth_mode: process.env.DEFAULT_ALLOWED_AUTH_MODES.split(','), | |
| auto_register: true, | |
| }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js`
around lines 20 - 27, Update the down migration’s changeColumn default
configuration to source allowed_auth_mode from
common.DEFAULT_TENANT_CONFIGURATION, matching the up migration and preserving
the environment-dependent DEFAULT_ALLOWED_AUTH_MODES value; keep the existing
auto_register setting unchanged.
| `UPDATE tenants | ||
| SET configuration = jsonb_set(configuration, '{auto_register}', 'true'::jsonb)` | ||
| ) | ||
| }, |
There was a problem hiding this comment.
why do we need this here?
…t value from the column
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js`:
- Around line 7-19: Update the up migration in
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js:7-19
to use common.DEFAULT_TENANT_CONFIGURATION as the column default and modify only
auto_register, preserving each tenant’s allowed_auth_mode. Update the down
migration at
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js:23-34
to use a default based on common.DEFAULT_TENANT_CONFIGURATION with auto_register
set to true and likewise modify only auto_register.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f042e32-afde-46d6-904b-94becda2f4d1
📒 Files selected for processing (2)
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.jssrc/envVariables.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/envVariables.js
| await queryInterface.changeColumn('tenants', 'configuration', { | ||
| type: Sequelize.JSONB, | ||
| allowNull: false, | ||
| defaultValue: null, | ||
| }) | ||
|
|
||
| await queryInterface.sequelize.query( | ||
| `UPDATE tenants | ||
| SET configuration = jsonb_set( | ||
| jsonb_set(configuration, '{auto_register}', 'false'::jsonb), | ||
| '{allowed_auth_mode}', '["password"]'::jsonb | ||
| )` | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Data corruption risk and incorrect schema defaults.
The UPDATE queries forcefully overwrite allowed_auth_mode for all existing tenants in both directions. This will silently wipe out any custom authentication settings, potentially locking users out. Furthermore, the column defaults are misconfigured: the up migration sets null (violating allowNull: false and the model definition in src/database/models/Tenant.js), while the down migration relies on the new environment defaults instead of properly rolling back to the old auto_register: true state.
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js#L7-L19: In theupmigration, setdefaultValue: common.DEFAULT_TENANT_CONFIGURATIONand simplify the raw query to strictly modifyauto_register.src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js#L23-L34: In thedownmigration, setdefaultValue: { ...common.DEFAULT_TENANT_CONFIGURATION, auto_register: true }and simplify the raw query to strictly modifyauto_register.
🛡️ Proposed fixes for the migrations
For the up migration:
await queryInterface.changeColumn('tenants', 'configuration', {
type: Sequelize.JSONB,
allowNull: false,
- defaultValue: null,
+ defaultValue: common.DEFAULT_TENANT_CONFIGURATION,
})
await queryInterface.sequelize.query(
`UPDATE tenants
- SET configuration = jsonb_set(
- jsonb_set(configuration, '{auto_register}', 'false'::jsonb),
- '{allowed_auth_mode}', '["password"]'::jsonb
- )`
+ SET configuration = jsonb_set(configuration, '{auto_register}', 'false'::jsonb)`
)For the down migration:
await queryInterface.changeColumn('tenants', 'configuration', {
type: Sequelize.JSONB,
allowNull: false,
- defaultValue: common.DEFAULT_TENANT_CONFIGURATION,
+ defaultValue: { ...common.DEFAULT_TENANT_CONFIGURATION, auto_register: true },
})
await queryInterface.sequelize.query(
`UPDATE tenants
- SET configuration = jsonb_set(
- jsonb_set(configuration, '{auto_register}', 'true'::jsonb),
- '{allowed_auth_mode}', '["otp","password"]'::jsonb)`
+ SET configuration = jsonb_set(configuration, '{auto_register}', 'true'::jsonb)`
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await queryInterface.changeColumn('tenants', 'configuration', { | |
| type: Sequelize.JSONB, | |
| allowNull: false, | |
| defaultValue: null, | |
| }) | |
| await queryInterface.sequelize.query( | |
| `UPDATE tenants | |
| SET configuration = jsonb_set( | |
| jsonb_set(configuration, '{auto_register}', 'false'::jsonb), | |
| '{allowed_auth_mode}', '["password"]'::jsonb | |
| )` | |
| ) | |
| await queryInterface.changeColumn('tenants', 'configuration', { | |
| type: Sequelize.JSONB, | |
| allowNull: false, | |
| defaultValue: common.DEFAULT_TENANT_CONFIGURATION, | |
| }) | |
| await queryInterface.sequelize.query( | |
| `UPDATE tenants | |
| SET configuration = jsonb_set(configuration, '{auto_register}', 'false'::jsonb)` | |
| ) | |
| await queryInterface.changeColumn('tenants', 'configuration', { | |
| type: Sequelize.JSONB, | |
| allowNull: false, | |
| defaultValue: { ...common.DEFAULT_TENANT_CONFIGURATION, auto_register: true }, | |
| }) | |
| await queryInterface.sequelize.query( | |
| `UPDATE tenants | |
| SET configuration = jsonb_set(configuration, '{auto_register}', 'true'::jsonb)` | |
| ) |
📍 Affects 1 file
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js#L7-L19(this comment)src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js#L23-L34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js`
around lines 7 - 19, Update the up migration in
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js:7-19
to use common.DEFAULT_TENANT_CONFIGURATION as the column default and modify only
auto_register, preserving each tenant’s allowed_auth_mode. Update the down
migration at
src/database/migrations/20260713122708-update-auto-register-tenant-configuration.js:23-34
to use a default based on common.DEFAULT_TENANT_CONFIGURATION with auto_register
set to true and likewise modify only auto_register.
2 participants
Summary by CodeRabbit