Add initial seeding support using users #285
Conversation
https://eaflood.atlassian.net/browse/WATER-3981 The [water-abstraction-service](https://gitub.com/DEFRA/water-abstraction-service) has a mechanism for seeding data for testing that relies on reading in YAML fixture files, sort of. Some content comes from these but other stuff is hard coded into the readers. Also, because you often need to add the ID of a record into something else your seeding it supports a system of 'named references' so the fixture-loader knows how to link things. Again though, it depends if the specific fixture-loader has been written to support this. What we've been left with is an extremely complex, brittle seeding mechanism that is also very slow. If only there was something simpler that existed to do this!? There is, and we're already using it. [Knex](https://knexjs.org/guide/migrations.html#seed-files) supports seeding using the same syntax we already use elsewhere in the app. It's something we've used previously on the [charging-module-api](https://github.com/DEFRA/sroc-charging-module-api). We think this is a better way to go and means we can start to move away from the hand-cranked version in **water-abstraction-service**. This change gets the ball rolling by creating a seed for users, and hooking it into the project so it can be triggered as a package.json script or from an endpoint (needed to support [water-abstraction-acceptance-tests](https://github.com/DEFRA/water-abstraction-acceptance-tests)).
This allows you to kick off the seed process from the command line.
We know we want to seed the dev/test users as our first 'test' of seeding. We want to take the opportunity to move away from hard coding the password we use in the source. So, in preparation for this we add a new env var, something we do on all our previous services.
First thing to get your head around is that unlike migrations Knex always runs _all_ seed files. So, it's important to write them in the knowledge the data they are seeding might already exist. For users, our initial seed example, we intend to only ever seed them once. So, we have implemented a method that first checks if a user already exists. If it doesn't we create the new user. This then applies to the user groups, which determines the permissions a user has in the service. To be able to log in with these users the password we generate must be recognised by the legacy code. So, we're forced to use the same mechanism currently used in [water-abstraction-tactical-idm](https://github.com/DEFRA/water-abstraction-tactical-idm) even if the package now appears to be no longer maintained.
Normally you would run the Knex seed process from the command line as part of your deployment. But we intend to use this mechanism within our acceptance tests. This means we need a method to programmatically trigger the process. This adds a new endpoint, which with some help from Stackoverflow allows us to do that!
We admit they are pretty 'noddy'. But all the work is done by Knex and you don't typically write unit tests for the seed files themselves.
@StuAA78 had a great suggestion in the comments to make the pattern of checking if something exists before inserting easier to follow.
Another great suggestion by @StuAA78
I swear this didn't work when I tried it! Co-authored-by: Stuart Adair <[email protected]>
Co-authored-by: Stuart Adair <[email protected]>
Cruikshanks
force-pushed
the
add-initial-seeding-users
branch
from
fd7828b to
5183c45
Compare
| const bcrypt = require('bcryptjs') | ||
| const { randomUUID } = require('crypto') | ||
|
|
||
| const DatabaseConfig = require('../../config/database.config.js') |
There was a problem hiding this comment.
We could destructure defaultUserPassword if we wanted seeing as it's the only bit of DatabaseConfig we use here? Not going to insist on it though 😁
There was a problem hiding this comment.
I think (as in you have forced me to think why I did this!) when we are talking about a value as opposed to a function it's best to leave the object so you have some context when it appears in the code, for example, I'm using it in
function _generateHashedPassword () {
const salt = bcrypt.genSaltSync(10)
return bcrypt.hashSync(DatabaseConfig.defaultUserPassword, salt)
}Remove DatabaseConfig it starts looking a bit like it appears from nowhere.
function _generateHashedPassword () {
const salt = bcrypt.genSaltSync(10)
return bcrypt.hashSync(defaultUserPassword, salt)
}I am conscious they are both just objects declared at the top of the file.
Again, I'm not precious about it either 🤷 . But it feels like a 'convention-in-the-making'. So, one to think about and discuss on the Dev call in my absence!
There was a problem hiding this comment.
As we are having a bit of a discussion about this 😁 I was wondering why we are generating the salt separately? I think we get the same result if we just do this return bcrypt.hashSync(defaultUserPassword, 10).
There was a problem hiding this comment.
Just following orders 🤷 https://github.com/kelektiv/node.bcrypt.js#to-hash-a-password-1
There was a problem hiding this comment.
Have a look at Technique 2 in that readme 😉
I would guess that the technique to generate the salt separately would be more efficient if you were performing multiple hashes as you could reuse the salt to save a bit of CPU? Not fussed either way but thought I'd mention it since we were having a chat about that chunk of code.
There was a problem hiding this comment.
That's a valid point. I didn't dig that far because I was just trying to replicate what they had done so we generate passwords the rest of the service can recognise. But I think technique 2 makes the code just a bit simpler so leave this with me and I'll make the change.
Co-authored-by: Stuart Adair <[email protected]>
https://eaflood.atlassian.net/browse/WATER-3981
The water-abstraction-service has a mechanism for seeding data for testing that relies on loading in YAML fixture files, sort of. Some content comes from these but other stuff is hard-coded into the 'loaders'.
Also, because you often need to add the ID of a record into something else your seeding it supports a system of 'named references' so the fixture-loader knows how to link things. Again though, it depends if the specific fixture-loader has been written to support this.
What we've been left with is an extremely complex, brittle seeding mechanism that is also very slow. If only there was something simpler that existed to do this!?
There is, and we're already using it. Knex supports seeding using the same syntax we already use elsewhere in the app. It's something we've used previously on the charging-module-api.
We think this is a better way to go and means we can start to move away from the hand-cranked version in water-abstraction-service.
This change gets the ball rolling by creating a seed for users, and hooking it into the project so it can be triggered as a package.json script or from an endpoint (needed to support water-abstraction-acceptance-tests).