Address multiple users & permissions issues

[alexandrebodin]

What does it do?

Addresses bug mentioned in #12322 and #12301

Why is it needed?

See history in linked PR and issue

How to test it?

• Try to register several users with same username or email and mixing them together with different providers

Related issue(s)/PR(s)

• Throw error if users try to sign up with a username that is already taken #12322
• Users can register using a taken email as username #12301)
• fix The populate=* query param not working for /me endpoint #13296
• fix Twitter Provider doesn't work #13217
• fix Advanced Settings => Redirection URL => placeholder should have different example [minor] #12570
• fix Login flow leaks user status #12362
• fix Multiple users can sign up with the same username #12319
• fix It says Email already taken, but it's actually not taken when you register user. #11806
• fix Users-Permissions plugin endpoints not showing in documentation #12555
• fix Users-Permissions Roles Route Config does not include prefix config #13172

[alexandrebodin]

@WalkingPizza If you can have a look here that would be awesome !

[codecov]

Codecov Report

Merging #13435 (8da88a7) into master (87abe1b) will not change coverage.
The diff coverage is n/a.

❗ Current head 8da88a7 differs from pull request most recent head 3b6e629. Consider uploading reports for the commit 3b6e629 to get more accurate results

@@           Coverage Diff           @@
##           master   #13435   +/-   ##
=======================================
  Coverage   54.25%   54.25%           
=======================================
  Files        1198     1198           
  Lines       30601    30601           
  Branches     5565     5565           
=======================================
  Hits        16603    16603           
  Misses      12178    12178           
  Partials     1820     1820           

Flag	Coverage Δ
front	56.60% <ø> (ø)
unit	48.55% <ø> (ø)

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

Impacted Files	Coverage Δ
...s/admin/src/pages/AdvancedSettings/utils/layout.js	100.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 87abe1b...3b6e629. Read the comment docs.

[derrickmehaffy]

Patch files for patch-package based on f7f9ecc
patches.zip

[derrickmehaffy]

Testing the following based on f7f9ecc

• Throw error if users try to sign up with a username that is already taken #12322
  • Confirmed, returns proper error
• Users can register using a taken email as username #12301
  • Confirmed, can't register using a taken email as username 👍
• The populate=* query param not working for /me endpoint #13296
  • Confirmed, need to give the user's role access to find permission on roles but that's to be expected.
  • Also looks like by default we populate role in the ctx.state.user 👍 🥳
• Twitter Provider doesn't work #13217
  • Not tested but we were on Discord as you were working on this, trust it's fixed 😛
• Advanced Settings => Redirection URL => placeholder should have different example [minor] #12570
  • Fixed as the example changed
• Login flow leaks user status #12362
  • Confirmed fixed, both for blocked/email conf we always check password first and return the proper error
• Multiple users can sign up with the same username #12319
  • Confirmed fixed, both username and email must be unique
• It says Email already taken, but it's actually not taken when you register user. #11806
  • Confirmed fixed

[strapi-bot]

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

https://forum.strapi.io/t/users-me-is-not-returning-relation-field-of-type-belong-to-many/3911/5

[derrickmehaffy]

Sorry @alexandrebodin didn't get a chance to test this before, I'll try testing tmrw when I'm at coworking space for the docs side

[derrickmehaffy]

@WalkingPizza / @Convly if both of you could test before next wednesday we might be able to squeeze this into 4.2 release but Alex wants at least one Strapi engineer to review.

Maybe if @petersg83 gets some time if JS doesn't?

[WalkingPizza]

Haven't had the chance to test anything yet, but I noticed we are still relying on the Query Engine API for most operations in the U&P controllers.

Since quite a lot of things have been changed, should we also switch to the Entity Service API while we are at it?

[derrickmehaffy]

Haven't had the chance to test anything yet, but I noticed we are still relying on the Query Engine API for most operations in the U&P controllers.

Since quite a lot of things have been changed, should we also switch to the Entity Service API while we are at it?

😅 This is something I brought up internally and with @iicdii too and I think we decided to do as little modification as possible to just make sure stuff works without fundamentally rewriting/refactoring the plugin.

Soon (tm), and with a TBD date we plan to replace this plugin with a new one that will allow us to deprecate this one as we don't implement breaking changes that require us to bump this package to v5.x.x.

[alexandrebodin]

@WalkingPizza anywhere specific you think we would benefit from the entityService? I'll have a look

[fingeg]

While this fixes the population problem of the /me request, it doesn't address it in the login response:

strapi/packages/plugins/users-permissions/server/controllers/auth.js

Line 53 in f925e93

const user = await strapi.query('plugin::users-permissions.user').findOne({

has no option to populate a user, was using nested collections in the old version with MongoDB, and would be great if I could use it in the new version, too.

Any thoughts on this?

[derrickmehaffy]

LGTM

[strapi-bot]

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

https://forum.strapi.io/t/new-release-strapi-v4-2-2/19927/1