[MGNLPUR-93] UserProfile is instantiated directly, ignoring user configuration Created: 22/Apr/13  Updated: 07/Oct/13  Resolved: 31/Jul/13

Status: Closed
Project: Magnolia Public User Registration
Component/s: None
Affects Version/s: 1.4.3, 2.0
Fix Version/s: 1.4.4, 2.1

Type: Bug Priority: Neutral
Reporter: Jan Haderka Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLPUR-82 Add in a version handler the delta be... Closed
relates to MGNLMIGRATION-249 Fix integration tests of migration Closed
causality
is causing MGNLSTK-1226 Separate configuration of demo-projec... Closed
is causing MGNLPUR-108 Wrong default configuration of PUR Closed
is causing MGNLPUR-107 PUR should not install configuration ... Closed
documentation
to be documented by DOCU-437 New Public User Configuration Closed
relation
is related to MGNLPUR-89 Allow configuration for multiple sites Closed
is related to MGNLPUR-99 PUR does not provide possibility to m... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

For details see http://forum.magnolia-cms.com/forum/thread.html?threadId=46aafe06-f884-488a-a42f-6aecdae17def



 Comments   
Comment by Roman Kovařík [ 20/Jun/13 ]

Methods:

protected void setExtraUserProperties(User user, UserProfile userProfile)
protected void updateExtraUserProperties(User user, UserProfile userProfile)

in DefaultUserRegistrar were removed in favour of

protected void setExtraUserProperties(User user, UserProfile userProfile, UserManager userManager)
protected void updateExtraUserProperties(User user, UserProfile userProfile, UserManager userManager)

without deprecating old ones, because:

1. It's not possible to change user properties without UserManager anymore.
2. They are not part of UserRegistrar interface.
3. It was not possible to use custom UserProfile class before, so I believe nobody had override them.

Comment by Jan Haderka [ 22/Jun/13 ]

DefaultUserRegistrar

  • don't use field names to iterate over props. While it is a good practice to keep getters in sync with props, it's not guaranteed. Also private props would not be accessible over reflection if running with security manager enabled.
  • The log for catch-all blog should be an error and should print the error stacktrace as well.
  • calling setExtraUserProperties from within updateExtraUSerProperties is incorrect. Missing (unset) properties should be removed on update instead of being set to null.
  • As for deprecation of method w/o user manager, someone could have still overriden them and retried user manager directly from SecuritySupport. You should put that method back call it and leave it empty as it was.
  • error message in RegistrationProcessor should be internationalized.
Comment by Jan Haderka [ 22/Jun/13 ]

btw signature of setExtra... was changed o also return user object. Update method should behave the same and descr above should be updated to reflect this change.

Comment by Jan Haderka [ 22/Jun/13 ]

One more thing - any reason for setting user profile class directly and configuring it under module config rather then requesting it via IoC and configuring it in module descriptor?

Comment by Jan Haderka [ 23/Jun/13 ]

or to move it one step further, we should group realm and profile class props in module config and allow someone to have different profile per realm.

Comment by Jan Haderka [ 23/Jun/13 ]

UpdateProcessor seems to suffer from same issue. You need to check them all. Further more class property in pur must be string, not class otherwise m2b chant pick it up.

Comment by Roman Kovařík [ 24/Jun/13 ]
  • As for deprecation of method w/o user manager, someone could have still overriden them...You should put that method back call it and leave it empty as it was...
  • btw signature of setExtra... was changed o also return user object. Update method should behave the same...

https://git.magnolia-cms.com/gitweb/?p=modules/public-user-registration.git;a=commitdiff;h=e7acabf2b5f5db0ee7af3ec51681ac67d8c73ab4

UpdateProcessor seems to suffer from same issue.

https://git.magnolia-cms.com/gitweb/?p=modules/public-user-registration.git;a=commitdiff;h=caa16f33483cbb7d687e106c47c922d66b920363

error message in RegistrationProcessor should be internationalized.

https://git.magnolia-cms.com/gitweb/?p=modules/public-user-registration.git;a=commitdiff;h=9c8ad7b1f64acfa83259447c004735446e9a42fb

...Missing (unset) properties should be removed on update instead of being set to null.

Is it possible to do it via UserManager? Currently they are set to empty String.

any reason for setting user profile class directly and configuring it under module config rather then requesting it via IoC?

This was already implemented.

...we should group realm and profile class props in module config

I can change this configuration, but can I change the configuration which was already there?
...And also how can I use more realms?

Comment by Roman Kovařík [ 28/Jun/13 ]

Autopopulating with user properties was removed due to security reasons.

Comment by Jan Haderka [ 09/Jul/13 ]

just wondering if we could go a middle ground here. Not to automatically populate everything, but allow dev/admin to configure list of fields that would be populated if present. WDYT?

Comment by Roman Kovařík [ 16/Jul/13 ]

Allow to specify user profile properties which will be automatically populated.

Comment by Jan Haderka [ 24/Jul/13 ]
  • src/main/java/info/magnolia/module/publicuserregistration/CustomUserProfile2.java should be in src/test/java
  • AbstractPURProcessor
             userProfile.setPassword((String) parameters.get(UserProfile.PASSWORD));
             userProfile.setFullName((String) parameters.get(UserProfile.FULLNAME));
             userProfile.setEmail((String) parameters.get(UserProfile.EMAIL));
    +
    +        Map<String, Object> properties = new HashMap<String, Object>();
    +        for (String property : userProfile.getExtraUserProperties()) {
    +            properties.put(property, parameters.get(property));
    

    since we are allowing admins to configure autopopulated props, they should be able to configure all of them incl password, fullname and e-mail.

  • UserProfileConfiguration - configuration node for autopopulated props should not be called extraProperties
Comment by Milan Divilek [ 30/Jul/13 ]

Reopen: extraUserProperties was not change to autopopulatedProperties in /magnolia-module-public-user-registration/src/main/resources/mgnl-bootstrap/public-user-registration/config.modules.public-user-registration.config.xml -> so after installation you get wrong configuration

Generated at Mon Feb 12 06:43:01 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.