User tests: Successful: Unsuccessful:
#7737 - enforce frontend construction for user activation URL
#7737
when the register method constructs the user activation URL, it doesn't explicitly enforce a frontend URL. this means that 3rd party extensions wanting to trigger registration (and thus activation emails) from a backend tool cannot make use of the core register method.
since the activation URL is only valid from the frontend, we should always enforce the proper construction.
there are two locations to be modified:
https://github.com/joomla/joomla-cms/blob/staging/components/com_users/models/registration.php#L410
https://github.com/joomla/joomla-cms/blob/staging/components/com_users/models/registration.php#L447
In both cases, we can just include the base url inside JRoute::_() to achieve the enforcement:
$data['activate'] = JRoute::_($base . '/index.php?option=com_users&task=registration.activate&token=' . $data['activation'], false);
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Title |
|
Status | Pending | ⇒ | Information Required |
Category | ⇒ | Front End |
I wonder if we need $base here.
Maybe we would just need `JRoute::_(JUri::root() . 'index.php?option=com_users&task=registration.activate&token=' . $data['activation'], false);
This code is wrong. And I think the solution is quite simple. We just need to remove administrator/ from $data['activate'] when this method is called from administrator area with this code
if (JFactory::getApplication()->isAdmin())
{
$adminPos = strrpos($data['activate'], 'administrator/');
$data['activate'] = substr_replace($data['activate'], '', $adminPos, 14);
}
I made a small PR to your branch to make it works like that lcdservices#1. Please try to test it and if it works, please merge it so that other testers can test it
I think the suggestion from @infograf768 is much cleaner and more direct than doing a string replacement after the URL has been constructed. @zero-24 -- can you test the revised PR? I don't have a dev site set up where Joomla is installed in a subdirectory (which is the use case that created the issues you noticed).
@lcdservices While the suggestion from @infograf768 works, it always return none-sef URL. Also, I haven't seen anywhere in our core code which pass full URL into JRoute::_ method, so I don't think that the code is good enough.
hmm generaly it works but it looks like we need to fix is also here:
https://github.com/lcdservices/joomla-cms/blob/patch-1/components/com_users/models/registration.php#L85
We also get with this patch applyed:
But i have no clue why?
hmm i guess the seccond on was an error on my side. I can not reproduce this again?!
Ok. So only point on need to be fixed see:
https://github.com/lcdservices/joomla-cms/blob/patch-1/components/com_users/models/registration.php#L85
and the comment by @joomdonation
@zero-24 I think we don't have to worry at the code at activate method (which you mentioned). The reason is because usually, third party developer only uses register method to create a Joomla user in Joomla core (I use it in my extension too), but for the activate method, I think it is just handled by Joomla core (when users click on the activation link).
ok...
@zero-24 I fixed it in the additional location you identified
@joomdonation - I really don't like the string replace method -- it's too hackish. but I agree it seems the only way to do this while preserving SEF urls. So I restructured it to handle it in this way.
Status | Information Required | ⇒ | Pending |
@lcdservices Could you merge @zero-24 PR so that travis happy and we can get this PR merged?
I use this function in my extension, too, and would love to have this merged in next release of Joomla so that I don't have to change code / fix bugs for client
done. sorry, didn't notice he had done it as a PR against my patch.
Status | Pending | ⇒ | Ready to Commit |
RTC based on testing
Labels |
Added:
?
|
Any reason not to use a simpler:
$data['activate'] = str_replace('administrator/', '', $data['activate'])
instead of looking for its position?
Might have bug in case you you have a site called administrator
for example, http://localhost/administrator/administrator
Of course it will only happens in that very special case but we should avoid that.
Milestone |
Added: |
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-10-01 18:13:27 |
Closed_By | ⇒ | rdeutz |
Labels |
Removed:
?
|
Milestone |
Milestone |
Added: |
Milestone |
Added: |
Milestone |
@lcdservices I have just tested this but it don't work for me.
How to test
apply this patch
choose "self" registration
expected result
http://www.example.org/dev/e3/index.php?option=com_users&task=registration.activate&token=ab7bc2b688beb2477c103ce4ee5b318b
actual result
http://www.example.org/index.php?option=com_users&task=registration.activate&token=ab7bc2b688beb2477c103ce4ee5b318b