Fix session middleware not migrated with v4 #11777

rgoupil · 2021-12-03T08:16:56Z

What does it do?

Fix unable to access the /api/connect/* routes due to the session middleware not being available
Migrate the session middleware to v4

Why is it needed?

The auth provider system rely on the session middleware to create and maintain user sessions.
In v4.0.0, the auth provider is broken as reported in #11337.

How to test it?

Start a strapi server
Enable any login provider (I initially had the issue with auth0, [V4] Auth Providers issues #11337 mention other login provider being broken too).
Access /api/connect/ (e.g. http://localhost:1337/api/connect/google)

If the issue persists, then you will be met by 400 error and the message "Grant: mount session middleware first".

Following this PR, the login provider should behave correctly and create an end-user session.

Related issue(s)/PR(s)

Related to #11337 by fixing the task "Error: Grant: mount session middleware first". Other tasks are left untouched.
Related to PR strapi/documentation#550.

strapi-cla · 2021-12-03T08:17:00Z

All committers have signed the CLA.

rgoupil · 2021-12-03T08:23:19Z

As mentioned in #11337 the issue might've already been fixed by the Strapi team in the next release (v4.0.1).
I don't mind if this PR is declined once v4.0.1 is out, however I cannot wait for a release without an ETA for my current tasks 😃

derrickmehaffy · 2021-12-03T17:16:04Z

packages/core/strapi/lib/middlewares/session/index.js

+  prefix: 'strapi:sess:',
+  ttl: 864000000,
+  rolling: false,
+  secretKeys: ['mySecretKey1', 'mySecretKey2'],


These really need to be auto-generated, we never did it in v3 but it's a security risk

derrickmehaffy · 2021-12-03T17:18:44Z

packages/core/strapi/lib/middlewares/index.js

 const publicStatic = require('./public');

 module.exports = {
  errors,
  ip,
  security,
+  session,


This would be a breaking change correct @alexandrebodin ? It would need to be added to the default ./config/middlewares.js?

petersg83 · 2021-12-03T17:49:40Z

Thank you for this PR!
I will take over if it's ok for you ? I see many things to refactor too :p

Also, it seems the session middleware was never used with another store than the easy and default one cookie. Nothing on the doc shows how to do it otherwise and it seems hard to guess too (need to install a specific package + provide the config in a certain way). I propose to remove support for external stores for now in order to fix the issue and maybe add the feature later.

What do you think @rgoupil @derrickmehaffy @alexandrebodin ?

derrickmehaffy · 2021-12-03T17:51:40Z

I can confirm that alternative stores other than node-memory were not working in v3, we never properly updated that package to use the database or redis

rgoupil · 2021-12-03T23:02:48Z

I didn't want to be rude but I agree that the whole session middleware could use a good refactoring. I stopped at making it work again 😅 @petersg83 please be my guest and make it pretty 😎

Figuring the other stores was educated guess work indeed. I don't think installing extra packages is bad as long as it is properly documented, that's how plugins work after all.

I'm happy to have a fix for now and even happier that you guys are going to make it better!
Please let me know if I can be of any help, we only recently stumbled on Strapi and I love it so far.

petersg83 · 2021-12-06T15:48:28Z

Ahah I understand :p Thank you!

Here is my PR : #11825
Would you like to review it and maybe check on your side if that works for you? I tried with the github provider on my side.

rgoupil · 2021-12-07T09:29:20Z

Ahah I understand :p Thank you!

Here is my PR : #11825 Would you like to review it and maybe check on your side if that works for you? I tried with the github provider on my side.

Sure thing! Although it will probably have to wait for tomorrow for me.

edit: found some time under a rock and left a few comments! I can't test it yet though.

rgoupil · 2021-12-09T19:19:03Z

@petersg83 I got to test your version and it seem to be working fine with auth0 🎉🎉
I noticed however that the error message is not very explicit when the privateKeys are not set in the session middleware config (which is also not documented yet):

[2021-12-09 18:36:07.605] error: Keys must be provided.
Error: Keys must be provided.
    at new Keygrip (<project>/node_modules/keygrip/index.js:18:11)
    at new Cookies (<project>/node_modules/cookies/index.js:48:49)
    at Object.get cookies [as cookies] (<project>/node_modules/koa/lib/context.js:170:23)
    at ContextSession.initFromCookie (<project>/node_modules/koa-session/lib/context.js:112:24)
    at ContextSession.get (<project>/node_modules/koa-session/lib/context.js:38:39)
    at Object.get (<project>/node_modules/koa-session/index.js:138:38)
    at app (<project>/node_modules/grant/lib/handler/koa-2.js:24:14)
    at Object.connect (<project>/node_modules/@strapi/plugin-users-permissions/server/controllers/auth.js:194:30)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    [...]

I suggest adding a check with a better error message in the session middleware to avoid this unclear trace and error.

cloakedninjas · 2021-12-14T13:11:03Z

Is there any workaround for this? Or do we need to wait for a fix?

alexandrebodin · 2021-12-14T14:57:15Z

@cloakedninjas you can fix it by adding the session middleware yourself for now. The PR is being worked on hopefully we will release a fix very soon

cloakedninjas · 2021-12-14T15:24:33Z

@cloakedninjas you can fix it by adding the session middleware yourself for now.

Adding 'strapi::session' into config/middlewares.js results in

error: Middleware strapi::session not found.

Or am I misunderstanding ?

alexandrebodin · 2021-12-14T16:33:39Z

You have to create a custom middleware that loads koa-session as the session middleware is not available yet (the goal of the PR)

petersg83 · 2021-12-20T16:05:06Z

@rgoupil I updated my PR if you want to check it :)
#11825

I'm closing this one in favor of #11825

fix: session middleware not migrated with v4

f73f23d

rgoupil mentioned this pull request Dec 3, 2021

[V4] Auth Providers issues #11337

Closed

4 tasks

rgoupil mentioned this pull request Dec 3, 2021

Fix user-permissions v4 api breaking changes strapi/documentation#550

Merged

rgoupil changed the title ~~fix: session middleware not migrated with v4~~ Fix session middleware not migrated with v4 Dec 3, 2021

derrickmehaffy requested changes Dec 3, 2021

View reviewed changes

petersg83 self-requested a review December 3, 2021 17:49

petersg83 closed this Dec 20, 2021

petersg83 mentioned this pull request Dec 20, 2021

Enable session middleware by default (fixes U&P provider authentication) #11825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix session middleware not migrated with v4 #11777

Fix session middleware not migrated with v4 #11777

rgoupil commented Dec 3, 2021 •

edited

strapi-cla commented Dec 3, 2021 •

edited

rgoupil commented Dec 3, 2021 •

edited

derrickmehaffy Dec 3, 2021

derrickmehaffy Dec 3, 2021

petersg83 commented Dec 3, 2021

derrickmehaffy commented Dec 3, 2021

rgoupil commented Dec 3, 2021 •

edited

petersg83 commented Dec 6, 2021 •

edited

rgoupil commented Dec 7, 2021 •

edited

rgoupil commented Dec 9, 2021 •

edited

cloakedninjas commented Dec 14, 2021

alexandrebodin commented Dec 14, 2021

cloakedninjas commented Dec 14, 2021

alexandrebodin commented Dec 14, 2021

petersg83 commented Dec 20, 2021

Fix session middleware not migrated with v4 #11777

Fix session middleware not migrated with v4 #11777

Conversation

rgoupil commented Dec 3, 2021 • edited

What does it do?

Why is it needed?

How to test it?

Related issue(s)/PR(s)

strapi-cla commented Dec 3, 2021 • edited

rgoupil commented Dec 3, 2021 • edited

derrickmehaffy Dec 3, 2021

Choose a reason for hiding this comment

derrickmehaffy Dec 3, 2021

Choose a reason for hiding this comment

petersg83 commented Dec 3, 2021

derrickmehaffy commented Dec 3, 2021

rgoupil commented Dec 3, 2021 • edited

petersg83 commented Dec 6, 2021 • edited

rgoupil commented Dec 7, 2021 • edited

rgoupil commented Dec 9, 2021 • edited

cloakedninjas commented Dec 14, 2021

alexandrebodin commented Dec 14, 2021

cloakedninjas commented Dec 14, 2021

alexandrebodin commented Dec 14, 2021

petersg83 commented Dec 20, 2021

rgoupil commented Dec 3, 2021 •

edited

strapi-cla commented Dec 3, 2021 •

edited

rgoupil commented Dec 3, 2021 •

edited

rgoupil commented Dec 3, 2021 •

edited

petersg83 commented Dec 6, 2021 •

edited

rgoupil commented Dec 7, 2021 •

edited

rgoupil commented Dec 9, 2021 •

edited