? ?

Pending

User tests: Successful: Unsuccessful:

Pull Request for Issue #17825 .

Summary of Changes

new method 'onUserBeforeSave'
deletes all #__user_keys database records for current user, if he changed his password (and this option is activated in the plugin configuration.

Testing Instructions

  1. Login on one device with activated "remember-me" checkbox and close the browser again.
  2. Login on another device and change your password here.
  3. Go back to your first device and open the website again.

Expected result

Website should not allow you to login without providing new password

Actual result

Documentation Changes Required

additional changes in other files:

staging...schultz-it-solutions:patch-1 (Plugin XML definition)
staging...schultz-it-solutions:patch-3 (Language File)
staging...schultz-it-solutions:patch-4 (Language File)

avatar joomla-cms-bot joomla-cms-bot - change - 1 Sep 2017
Category Front End Plugins
avatar schultz-it-solutions schultz-it-solutions - open - 1 Sep 2017
avatar schultz-it-solutions schultz-it-solutions - change - 1 Sep 2017
Status New Pending
avatar dgt41
dgt41 - comment - 1 Sep 2017

The code has some code styling issues:

--------------------------------------------------------------------------------
26s
57
FOUND 10 ERROR(S) AND 1 WARNING(S) AFFECTING 8 LINE(S)
26s
58
--------------------------------------------------------------------------------
26s
59
 100 | WARNING | Line exceeds 150 characters; contains 161 characters
26s
60
 115 | ERROR   | Please start your comment with a capital letter; found "//
26s
61
     |         | irelevant on new users"
26s
62
 116 | ERROR   | Expected "if (...)\n"; found "if (...) "
26s
63
 116 | ERROR   | Closing brace must be on a line by itself
26s
64
 117 | ERROR   | Please start your comment with a capital letter; found "//
26s
65
     |         | irelevant, because password was not changed by user"
26s
66
 118 | ERROR   | Expected "if (...)\n"; found "if (...) "
26s
67
 118 | ERROR   | Closing brace must be on a line by itself
26s
68
 119 | ERROR   | Please start your comment with a capital letter; found "//
26s
69
     |         | irelevant, because "resetting on pw change" is not activated"
26s
70
 120 | ERROR   | Expected "if (...)\n"; found "if (...) "
26s
71
 120 | ERROR   | Closing brace must be on a line by itself
26s
72
 149 | ERROR   | Please put a space between the // and the start of comment
26s
73
     |         | text; found "//$this->app->input->cookie->set($cookieName, '',
26s
74
     |         | 1, $this->app->get('cookie_path', '/'),
26s
75
     |         | $this->app->get('cookie_domain', ''));"
26s
76
--------------------------------------------------------------------------------
26s
77
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
26s
78
--------------------------------------------------------------------------------
avatar brianteeman
brianteeman - comment - 1 Sep 2017

please add your other changes to this pr. you can do that by going to this branch in your fork and editing the files there. they will then be added automatically to this pull request

avatar schultz-it-solutions schultz-it-solutions - change - 1 Sep 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 1 Sep 2017
Category Front End Plugins Administration Language & Strings Front End Plugins
avatar schultz-it-solutions schultz-it-solutions - change - 1 Sep 2017
Labels Added: ?
avatar Quy
Quy - comment - 1 Sep 2017
avatar Bakual
Bakual - comment - 1 Sep 2017

Do we really need an option for that? I would just go without it.
If you really want that parameter, the default values in code and in XML will have to match to prevent unexpected behavior.

avatar Bakual
Bakual - comment - 1 Sep 2017

Another thing to consider: You put the code into the "System - Remember" plugin, but that is actually just a proxy plugin so the real "Authentication - Cookie" plugin can do its work. So this code should go into that authentication plugin instead.

avatar mbabker
mbabker - comment - 1 Sep 2017

If we aren't importing authentication plugins in the MVC for CRUD actions then this is the right plugin.

avatar Bakual
Bakual - comment - 1 Sep 2017

How about we import the authentication plugins instead? I'd say that should be easy and could be useful for other authentication plugins as well. I don't think it's a good idea to spread the logic of the cookie plugin (again) over multiple ones. If we can, it should be kept within the cookie one.

avatar mbabker
mbabker - comment - 1 Sep 2017

The authentication plugins shouldn't be handling CRUD actions (since some people seem to be overly concerned about what tasks specific plugin groups actually listen to, apparently we should have a "User - Remember Me" plugin for this task if you buy into their thinking 😉 ).

The authentication plugins in general aren't treated the same as every other plugin group, there are actually only two places in core where they are actually loaded as "real" plugins otherwise the Authentication class pulls the plugin objects out of the dispatcher and direct calls the methods (so not using the event system). I would be hesitant to add more plugin based functionality to this plugin group since it is not treated as a correct plugin group (if we want to use it as a real plugin group the authentication system needs to be refactored to use proper events and not the crap logic it has now).

As far as where the code is, things are fine right now.

avatar schultz-it-solutions
schultz-it-solutions - comment - 1 Sep 2017

On the "user message": I may be "over sensible" in this respect, because of the underlying story why this pr exists...
If you think this user message is "clutter", I am not oposing this!

avatar Bakual
Bakual - comment - 5 Sep 2017

Do you guys think we need that option? Is there a reason you do not want to reset the cookies after a password change? Imho that "feature" should always be enabled. I don't see why someone would want to disable it. So the option could be removed to make the code simpler.

avatar brianteeman
brianteeman - comment - 5 Sep 2017

Agreed. Why would you want to enable a bad option

avatar ggppdk
ggppdk - comment - 6 Sep 2017

Do you guys think we need that option? Is there a reason you do not want to reset the cookies after a password change? Imho that "feature" should always be enabled. I don't see why someone would want to disable it. So the option could be removed to make the code simpler.

I was reading this from top, and was going to write that an option should not be added,
this behaviour should not be possible to disable !

avatar joomla-cms-bot joomla-cms-bot - change - 6 Sep 2017
Category Front End Plugins Administration Language & Strings Language & Strings Front End Plugins
avatar joomla-cms-bot joomla-cms-bot - change - 6 Sep 2017
Category Front End Plugins Language & Strings Front End Plugins
avatar schultz-it-solutions
schultz-it-solutions - comment - 24 Oct 2017

is there anything left to be done before this PR gets merged back?

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Oct 2017

@schultz-it-solutions at least it needs 2 successfully Tests.

avatar schultz-it-solutions
schultz-it-solutions - comment - 19 Dec 2017

@Quy , @dgt41 , @ggppdk
any chance one of you guys can perform another test in order to finalize this pr?

avatar schultz-it-solutions schultz-it-solutions - change - 19 Dec 2017
Labels Removed: ?
avatar Quy Quy - test_item - 21 Dec 2017 - Tested successfully
avatar Quy
Quy - comment - 21 Dec 2017

I have tested this item successfully on f226416


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17827.

avatar schultz-it-solutions
schultz-it-solutions - comment - 23 Jan 2018

@dgt41 , @ggppdk
any chance one of you guys can perform another test in order to finalize this pr?

avatar csthomas
csthomas - comment - 23 Jan 2018

I have tested this item successfully on f226416


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17827.

avatar csthomas csthomas - test_item - 23 Jan 2018 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 23 Jan 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jan 2018

Ready to Commit after two successful tests.

avatar zero-24 zero-24 - change - 7 Feb 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-02-07 00:49:52
Closed_By zero-24
Labels Added: ?
avatar zero-24 zero-24 - close - 7 Feb 2018
avatar zero-24 zero-24 - merge - 7 Feb 2018
avatar zero-24
zero-24 - comment - 7 Feb 2018

Merged thanks!

Add a Comment

Login with GitHub to post a comment