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

Allow configuration of fields during register #16333

Closed
wants to merge 5 commits into from

Conversation

derrickmehaffy
Copy link
Member

@derrickmehaffy derrickmehaffy commented Apr 10, 2023

What it does

Modifies users-permissions to pick fields from registration body instead of omitting them and allows for plugin configuration to let users select what fields are allowed during the registration phase.

How to test

Configure the plugin like the following:

// path: ./config/plugins.js

module.exports = {
  'users-permissions': {
    config: {
      register: {
        allowedFields: [ 'username', 'password', 'email', 'role' ]
      }
    }
  }
}

This should allow you to specify the role ID during the registration phase (not that I'd recommend that personally)

Documentation PR

@derrickmehaffy derrickmehaffy self-assigned this Apr 10, 2023
@derrickmehaffy derrickmehaffy added source: plugin:users-permissions Source is plugin/users-permissions package pr: enhancement This PR adds or updates some part of the codebase or features pr: security This PR is security issue flag: documentation This PR requires a documentation update labels Apr 10, 2023
Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

This could be considered a breaking change, since users who have modified their user schema may be expecting those additional fields to be set on register.

I think it would be acceptable in this case though if we hold this until the next minor release and add a note in the release log about it so anyone doing so can add this setting.

@derrickmehaffy derrickmehaffy added the flag: don't merge This PR should not be merged at the moment label May 2, 2023
Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

Before merging, we need to have documentation written and then we need to put a "breaking change" in the patch notes instructing anyone with custom user fields they expect to be set on registration to be added to the config.

But I think the breaking change is worth it in this case, it only breaks code for users who are relying on a vulnerability in their code. We need to wait for at least a minor version though, not the next patch version.

@innerdvations innerdvations added flag: 💥 Breaking change This PR contains breaking changes and should not be merged and removed flag: documentation This PR requires a documentation update labels May 15, 2023
@innerdvations innerdvations added this to the 4.11.0 milestone May 15, 2023
@innerdvations
Copy link
Contributor

innerdvations commented May 15, 2023

Documentation has been written

We need to link to it when we released and flag the breaking change in 4.11

@Convly Convly modified the milestones: 4.11.0, 4.11.1 Jun 7, 2023
@Marc-Roig Marc-Roig modified the milestones: 4.11.1, 4.11.2 Jun 12, 2023
@Convly Convly modified the milestones: 4.11.2, 4.11.3 Jun 21, 2023
@alexandrebodin alexandrebodin removed this from the 4.11.3 milestone Jun 26, 2023
@alexandrebodin
Copy link
Member

alexandrebodin commented Jun 26, 2023

We can make the breaking change for v5. In the meantime we can make the omit configurable so users can still make this better without a breaking change

@NicoHof
Copy link
Contributor

NicoHof commented Jun 27, 2023

Hello!
I got here because I am a bit confused by the way this breaking change is communicated.
I noticed this migration guide https://docs.strapi.io/dev-docs/migration-guides --> https://docs.strapi.io/dev-docs/migration/v4/migration-guide-4.10.1-to-4.11.0

It says this breaking change is already introduced in 4.11.0 but this PR is still open.

I think it is confusing for people to upgrade to 4.11.0 because custom fields still work on registration in 4.11.0 without the suggested configuration changes.

Best, Nico

@innerdvations
Copy link
Contributor

innerdvations commented Jun 27, 2023

It says this breaking change is already introduced in 4.11.0 but this PR is still open.

You're right, I'll make sure the documentation gets reverted, thank you for pointing that out! @pwizla

As far as how to proceed, here is what we've decided to do, which will require updating this PR:

  • Continue to use omit when allowedFields is not present in config
    • Add pseudo-deprecation warning, on strapi startup U&P will emit a warning when allowedFields is not set saying that in a future version custom user fields will not be accepted for user registration unless added to allowedFields
  • When allowedFields is present in config, we will switch to pick merging in the user's array
  • New projects will be generated with allowedFields set to an empty array
  • the change in functionality will be documented
  • in v5, we will remove the omit option entirely (and then we don't need to require allowedFields or emit a warning, we just treat undefined as an empty array like the current PR)

@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/new-content-type-fields-for-users-permissions-user-arent-preserved/29604/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: 💥 Breaking change This PR contains breaking changes and should not be merged pr: enhancement This PR adds or updates some part of the codebase or features pr: security This PR is security issue source: plugin:users-permissions Source is plugin/users-permissions package
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

10 participants