fix: check convertSessionIdToMongoObjectId before using idProvider

[mrcleanandfresh]

This fix is related to this bug report: #1204

The configuration option of convertSessionIdToMongoObjectId was not being checked when using the idProvider to create a new session.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4ae3653

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@accounts/mongo-sessions	Minor
@accounts/mongo	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov]

Codecov Report

Merging #1205 (4ae3653) into master (4f5b38e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1205   +/-   ##
=======================================
  Coverage   95.28%   95.29%           
=======================================
  Files         106      106           
  Lines        2377     2381    +4     
  Branches      473      492   +19     
=======================================
+ Hits         2265     2269    +4     
+ Misses        107      103    -4     
- Partials        5        9    +4     

Impacted Files	Coverage Δ
...ages/database-mongo-sessions/src/mongo-sessions.ts	100.00% <100.00%> (ø)
packages/oauth/src/accounts-oauth.ts	88.88% <0.00%> (ø)
packages/server/src/accounts-server.ts	90.70% <0.00%> (ø)
...s/graphql-api/src/utils/authenticated-directive.ts	27.27% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f5b38e...4ae3653. Read the comment docs.

[pradel]

Thanks for the fix!
I am wondering what is the best approach here. The problem I see here is that the behavior for users and session will be different for the same option. What do you think about introducing a new idSessionProvider instead?

[mrcleanandfresh]

What do you think about introducing a new idSessionProvider instead?

1. Use both the boolean and the provider functions (recommended) - It seems redundant, but I think the convertSessionIdToMongoObjectId and convertUserIdToMongoObjectId booleans need to be consulted prior to creating a new user or a new session. This is in addition to checking whether the id*Provider function is present. This provides more guarantees about the data that makes it into the Database.
2. Remove the booleans that configure the override - it is confusing that they are configured in order to use a regular ObjectId, but are not consulted when the object id is being created. I don't recommend this option because of the check that is in place to convert the id to an ObjectId when querying.
3. Other options?

What are your thoughts?

[pradel]

To avoid a maximum the breaking changes and impact the users when they will upgrade to the new version option 1 seems good :)

but I think the convertSessionIdToMongoObjectId and convertUserIdToMongoObjectId booleans need to be consulted prior to creating a new user or a new session

I am just not sure about this, this would mean that idProvider is actually not used if the value is false right? That can be really confusing.

[mrcleanandfresh]

I am just not sure about this, this would mean that idProvider is actually not used if the value is false right? That can be really confusing.

Yeah... Hm... I think they're pretty even options, but I think your option of just including an idSessionProvider should work the best. Least impactful.

Alternatively, we could log a developer warning in the console when in development mode under the following conditions:

1. convertSessionIdToMongoObjectId/convertUserIdToMongoObjectId is set to true
2. idSessionProvider/idProvider is included

This would let the developer know they have things misconfigured.

[pradel]

The warning approach sounds like a good idea 👍

[mrcleanandfresh]

The warning approach sounds like a good idea 👍

The approach I'm thinking about will be very simple:

1. Add documentation about these two configuration variables by modifying Mongo Options in both the docs and website. If it should be updated elsewhere, can you point me to a good location for where it will live?
2. Update Mongo Sessions:
   1. Update the interface
   2. Add an if-statement in the constructor that checks idSessionProvider and convertSessionIdToMongoObjectId. Exact warning text open for discussion:

if (typeof this.options.idSessionProvider === 'function' && this.options.convertSessionIdToMongoObjectId) {
  console.warn(`You have set both "idSessionProvider" and "convertSessionIdToMongoObjectId = false" which will cause your "idSessionProvider" to be ignored. 
In order to fix this warning change "convertSessionIdToMongoObjectId" to "true" or remove your "idSessionProvider" from the configuration.
`);
}

3. Add a test that meets these conditions and spies on console.warn to be called with idProvider and convertUserIdToMongoObjectId.

What do you think of that approach, anything I'm missing? It seems like I can contain all those changes within this PR, but let me know if I need to open a separate one for docs or for MongoDB Password (same changes).

Thanks for any feedback!

(Addressed in 3c718c1)

[mrcleanandfresh]

I also couldn't help but notice that the Mongo Options contains configuration that assumes you will use Accounts Password as your strategy, see convertUserIdToMongoObjectId, idProvider, and others. Should the docs be updated to let the developer know which strategy combination this applies for, or updated to show all possible configurations for each strategy using the Mongo Database?

[pradel]

I was trying the change and noticed something that was possible before but not possible with this change:

const userStorage = new MongoDbDriver(mongoose.connection, {
  convertUserIdToMongoObjectId: true,
  // Date.now() can be replaced by any value here
  idProvider: () => new mongoose.Types.ObjectId(Date.now()),
});

[mrcleanandfresh]

@pradel I see that the "Test Packages" Github workflow is failing. Since this is a breaking change, I feel like that is expected, but could use your guidance on how to proceed.

[pradel]

The tests failing are coming from here, https://github.com/accounts-js/accounts/blob/master/packages/database-tests/src/index.ts
To run it on your computer, you can run yarn test in the mongo folder :)

[mrcleanandfresh]

The tests failing are coming from here, https://github.com/accounts-js/accounts/blob/master/packages/database-tests/src/index.ts
To run it on your computer, you can run yarn test in the mongo folder :)

@pradel Thanks! So it looks like I need to add the new idSessionProvider and convertSessionIdToMongoObjectId as false to the database-tests.ts file in database-mongo code. I'll try this out when I get some time.

There's other code that uses that dependency injection for running the tests, but it's for Redis.

[pradel]

Yes, you might have to edit the mongo package to pass down the options for the tests to run properly :)

[mrcleanandfresh]

@pradel tests are passing. Let me know once you've tested some things. I had to add a type definition to the AccountsMongoOptions interface, so just want to be sure I don't need to add the idSessionProvider to any other places to ensure no breakage with TypeScript. I've checked what I know about on my end.

Also, please refer to a previous question about the strategy for updating the docs. If the latter is preferred, and you can offer clear enough directions on how I can find all combinations I can take care of this now. Otherwise, it might be good for you or someone else to tackle that in a separate PR.

[mrcleanandfresh]

I get this error when I try to run all tests:

~/Code/forks/accounts$ pnpm run testonly

> @0.22.0 testonly /home/kevin/Code/forks/accounts
> lerna run testonly

lerna notice cli v4.0.0
lerna info Executing command in 23 packages: "pnpm run testonly"
lerna ERR! pnpm run testonly exited 1 in '@accounts/oauth-twitter'
lerna ERR! pnpm run testonly stdout:

> @accounts/oauth-twitter@0.32.1 testonly /home/kevin/Code/forks/accounts/packages/oauth-twitter
> jest

 ELIFECYCLE  Command failed with exit code 1.
lerna ERR! pnpm run testonly stderr:
FAIL __tests__/index.ts
  ● Test suite failed to run

    TypeError: Cannot read property 'bind' of undefined

      at Runtime._createJestObjectFor (../../node_modules/.pnpm/jest@27.3.1/node_modules/jest/node_modules/jest-runtime/build/index.js:2193:46)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.619 s
Ran all test suites.
lerna ERR! pnpm run testonly exited 1 in '@accounts/oauth-twitter'
lerna WARN complete Waiting for 3 child processes to exit. CTRL-C to exit immediately.
 ELIFECYCLE  Command failed with exit code 1.

Do you know what I'm missing, or should I not be trying to run the tests at this level?

[pradel]

I never run the tests from the root of the monorepo so don't know about this error, you can run the testonly command in the subfolders too :)

[pradel]

Also, please refer to a previous question about the strategy for updating the docs. If the latter is preferred, and you can offer clear enough directions on how I can find all combinations I can take care of this now. Otherwise, it might be good for you or someone else to tackle that in a separate PR.

We can leave this to a separate pr, let's merge and release the work you did first :)

[pradel]

Thanks for the work on this one and sorry it took so long! Will create a new release soon.

[mrcleanandfresh]

Thanks for the work on this one and sorry it took so long! Will create a new release soon.

Absolutely, happy to contribute! No issues with how long it took, it was over the year's end which is a busy time.