Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from Jan 25, 2022

Conversation

petersg83
Copy link
Contributor

@petersg83 petersg83 commented Dec 6, 2021

It enables session middleware by default (which fixes U&P provider authentication)
It removes the possibility to authenticate requests using access_token url query param
It fixes a wording where the displayed provider name was wrong

Based on a previous PR created by @rgoupil #11777

also fix #7405 (thank you @borm for showing a fix)

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #11825 (56407e8) into master (644624d) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11825      +/-   ##
==========================================
+ Coverage   47.68%   47.75%   +0.07%     
==========================================
  Files         212      210       -2     
  Lines        8223     8139      -84     
  Branches     1863     1835      -28     
==========================================
- Hits         3921     3887      -34     
+ Misses       3547     3503      -44     
+ Partials      755      749       -6     
Flag Coverage Δ
front ?
unit 47.75% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/admin/server/strategies/api-token.js 100.00% <ø> (+3.03%) ⬆️
packages/core/strapi/lib/services/fs.js 71.42% <100.00%> (ø)
packages/core/strapi/lib/core/registries/config.js

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 644624d...56407e8. Read the comment docs.

@petersg83 petersg83 changed the title Enable session middleware by default + make auth U&P routes public + fix provider name Enable session middleware by default + remove access_token url auth + fix provider name Dec 20, 2021
@petersg83 petersg83 added this to the 4.0.1 milestone Dec 20, 2021
@petersg83 petersg83 added issue: bug Issue reporting a bug source: plugin:users-permissions Source is plugin/users-permissions package labels Dec 20, 2021
@petersg83 petersg83 changed the title Enable session middleware by default + remove access_token url auth + fix provider name Enable session middleware by default Dec 20, 2021
@petersg83 petersg83 changed the title Enable session middleware by default Enable session middleware by default (fixes U&P provider authentication) Dec 20, 2021
@derrickmehaffy
Copy link
Member

Will test it after the monday meeting @petersg83 👍

packages/core/strapi/lib/middlewares/session.js Outdated Show resolved Hide resolved
packages/core/strapi/lib/middlewares/session.js Outdated Show resolved Hide resolved
packages/core/strapi/lib/middlewares/session.js Outdated Show resolved Hide resolved
@alexandrebodin alexandrebodin modified the milestones: 4.0.1, 4.0.2 Dec 22, 2021
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need all the env_path manipulations for the tests. If we use the generator we should just have a valid .env in the testApp 🤔

Also I would keep the .env.example as when the dev will push their app they won't get a .env we should generate one with the missing env vars

examples/getstarted/config/middlewares.js Outdated Show resolved Hide resolved
@petersg83
Copy link
Contributor Author

env_path : The .env is not detected because the script run is not located next to the .env file, so we need to specify the correct path. Before that the .env file of the testApp was full of generated token (each time the app was restarted they were generated again). Do you think we can do something simpler?

@petersg83 petersg83 force-pushed the fix/enable-session-middleware branch from 1427fb7 to 1dc74b0 Compare January 19, 2022 16:13
@alexandrebodin alexandrebodin modified the milestones: 4.0.5, 4.0.6 Jan 19, 2022
test/e2e.js Outdated Show resolved Hide resolved
@@ -1,4 +1,7 @@
module.exports = ({ env }) => ({
host: env('HOST', '0.0.0.0'),
port: env.int('PORT', 1337),
app: {
keys: env.array('APP_SECRETS'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we call that APP_KEYS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes let's do that

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It's a lot cleaner than previously

@alexandrebodin alexandrebodin merged commit d796cf9 into master Jan 25, 2022
@alexandrebodin alexandrebodin deleted the fix/enable-session-middleware branch January 25, 2022 12:24
@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/user-authentication-with-next-js-and-strapi/6289/19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug source: plugin:users-permissions Source is plugin/users-permissions package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[V4] Auth Providers issues Plugin users-permissions providers Custom callback url doesn't work
6 participants