feat: add dialectOptions to allow users to choose their connection options

rap2hpoutre commented 4 years ago

See:

⚠️ Issue

I see one big issue with this implementation. Users could miss that a dialectOptions option has been configured since it is only referenced in models/index.js. For instance, if they selected postgreSQL during their onboarding (see specs), it will automatically add the line above in their code, and they could forget to remove it when going to production, which is a security issue since they are not protected about man in the middle attack:

dialectOptions: {"rejectUnauthorized":false},

Reviewer, before testing and reading code, could you help me decide how to address this issue?

Solution 1

Adding a comment such as:

// You will have to review dialect options when switching environment.
// For instance, if you set `rejectUnauthorized` to false in development mode, 
// you should remove it in production to avoid security issues.
dialectOptions: {"rejectUnauthorized":false},

Caveat: User can still miss this comment (but Forest Admin has done its job: users may be aware thanks to this comment).

Solution 2

Move dialectOptions JSON string in .env.

DATABASE_DIALECT_OPTIONS={"rejectUnauthorized":false}

Then in models/index.js

dialectOptions: JSON.parse(process.env.DATABASE_DIALECT_OPTIONS || '{}'),

Caveat: writing JSON in .env is not that common I guess.

Pull Request checklist:

[x] Write an explicit title for the Pull Request, following Conventional Commits specification
[x] Create automatic tests
[x] No automatic tests failures
[ ] Test manually the implemented changes
[x] Review my own code (indentation, syntax, style, simplicity, readability)
[x] Wonder if you can improve the existing code

arnaudbesnier commented 4 years ago

feat: add dialectOptions to allow users to choose their connection options My understanding is that you'll have to consider this option after a first onboarding failure.

If I am right, after a first onboarding failure, will developers have to:

run lumber --help
understand that dialectOptions is the solution to their issue
understand that dialectOptions refers to Sequelize connection option
read Sequelize documentation to understand how to configure it to solve the problem
run the new lumber generate command ?

rap2hpoutre commented 4 years ago

If I am right

@arnaudbesnier Fortunately, you are wrong 🎉

👉 BONUS: Do you want to be my reviewer? 🥰 (I failed to find one!)

ForestAdmin / lumber