User tests: Successful: Unsuccessful:
Pull Request for Issue #9943
I continued the great work of @framontb at @icampus
Category | ⇒ | Administration Components Language & Strings Templates (admin) Front End |
Status | New | ⇒ | Pending |
Labels |
Added:
?
?
|
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-08 13:17:05 |
Closed_By | ⇒ | brianteeman | |
Labels |
Closing for now until the branch is fixed
Labels |
Status | Closed | ⇒ | New |
Closed_Date | 2016-08-08 13:17:05 | ⇒ | |
Closed_By | brianteeman | ⇒ |
Status | New | ⇒ | Pending |
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-09 08:20:54 |
Closed_By | ⇒ | mxkmp29 |
Status | Closed | ⇒ | Pending |
I have tested this item
Before this PR there is ONE tab called password options
After this PR there are TWO tabs
I have tested this item
@icampus
I have tested this Pull Request Successfully. I installed the Pull Request and followed the Test Instructions.
All domains i set on the Blacklist were blocked.
Everything work well.
I have tested this item
The default setting for the number of characters in a password is 0 which means there is no minimum or maximum characters
BUT
Try to save a username with just one character and you will get the following error
Save failed with the following error: Please enter a valid username. No space at beginning or end, at least 2 characters and must not contain the following characters: < > \ " ' % ; ( ) &.
Entering an email from the blocked list gives a different error in the admin and the frontend
Warning
Invalid field: Email
Warning
The email address you entered is already in use or invalid. Please enter another email address.
It would be preferable if they were the same and then we can work on some more appropriate text for the message
####AJAX
Maybe its me misunderstanding but i am not seeing any ajax validation. Can you please record a small screen capture of what I am supposed to be seeing
Status | Pending | ⇒ | Closed |
Closed_Date | 2016-08-09 08:20:54 | ⇒ | 2016-08-12 09:25:12 |
Status | Closed | ⇒ | New |
Closed_Date | 2016-08-12 09:25:12 | ⇒ | |
Closed_By | mxkmp29 | ⇒ |
Status | New | ⇒ | Pending |
sorry, I clicked the wrong button
Ah I see now that the AJAX stuff only works on the frontend - it doesnt work on the admin at all
@brianteeman The reason you see 2 different messages is because the back-end form uses Javascript validation and the front-end does not. The Javascript validation has no option for using language strings at this moment. It simply reads the invalid field label and concatenates that to the Invalid field: message.
To make this consistent Max would have to rewrite the whole back-end form validation. Even though I agree the messages should be the same, this may be out of scope for this pull request and would warrant another pull request.
In general I believe we should have a more flexible and consistent validation system for forms without duplicating checks.
Your thoughts?
yes I see now that the ajax stuff is not present on the admin side at all.
On 12 August 2016 at 11:01, RolandD notifications@github.com wrote:
@brianteeman https://github.com/brianteeman The reason you see 2
different messages is because the back-end form uses Javascript validation
and the front-end does not. The Javascript validation has no option for
using language strings at this moment. It simply reads the invalid field
label and concatenates that to the *Invalid field: * message.To make this consistent Max would have to rewrite the whole back-end form
validation. Even though I agree the messages should be the same, this may
be out of scope for this pull request and would warrant another pull
request.In general I believe we should have a more flexible and consistent
validation system for forms without duplicating checks.Your thoughts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11516 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8fNngwWlGZnPeG6IevptfpH8Q0Kpks5qfERxgaJpZM4Je_OM
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
Thanks for the input, we will keep it to the front-end for this PR. Max already fixed the username length, so it's ready for testing again.
thanks for fixing the username length - that works
On 12 August 2016 at 11:10, RolandD notifications@github.com wrote:
Thanks for the input, we will keep it to the front-end for this PR. Max
already fixed the username length, so it's ready for testing again.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11516 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8dmov8RTArR4-N36STvEosjLB_aeks5qfEaggaJpZM4Je_OM
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
Is there supposed to be an ajax check on the email?
This PR needs a "New feature" label ?
@brianteeman yes
Max already fixed the username length, so it's ready for testing again.
The language string needs updating now as there isnt a Zero indicates no limit
Labels |
Added:
?
|
Its possible to set the maximum number of characters in the username to less than the minimum
Set the minimum number of characters to 3 and you can still create a 2 character username in the admin.
In the front end you are restricted but the error messages are wrong
You get these two
Warning
The username field should contain a minimum of 3 characters. You only give 2.
The username you entered is not available. Please pick another username.
You dont need the two messages - and I would be frustrated if I then chose a username that was longer than the maximum
The username should contain a minimum of 3 characters and a maximum of 5. You only gave 2. Please enter another username.
Some comments,
Using a comparison tool, i see that : media/system/js/validate-uncompressed.js
only has fixed indentationand some new lines, i guess it is ok
Also i see that this can be usable in 3rd party code too
A question should the number of requests to:
$ajaxUri = JRoute::_('index.php?option=com_users&task=registration.validate&format=json&' . JSession::getFormToken() . '=1');
be limited to somehow ?
or is it ok to allow unlimited number ?
someone can grab the above url, from the HTML and make unlimited requests ?
@brianteeman We looked at this and it actually works as designed at the moment. I agree with you that we should just show one error message. The problem is the following. To be able to make a customized error message based on the settings of the site, we need to pass it the configuration values. This is impossible with the current way Joomla works. By adding the message to the rule which contains these settings we can customize the message.
Now the XML definition contains an element called message which holds the language string to show when there is an error. This is why we get two messages, the customized one and the default one. Now the idea was to remove the default one but that resulted in Invalid field: Username. So that is not the way to go either.
The only solution I see for one message is to modify the existing default ones and forget about the customizing of the message. The default one should be very generic in that case.
@ggppdk The requests are limited by the session token which I believe is good enough. Yes, you can grab that URL and make unlimited requests just like you can take the homepage URL and make unlimited requests. I guess I don't see this as an issue.
If it tells me that I have failed the minimum and try again
and then I try again and it tells me the maximum I will not be a happy user
The current default message is:
The username you entered is not available. Please pick another username.
This we can change to:
The username does not meet the requirements.
It just won't tell you which requirements because we can't specify them.
The requests are limited by the session token which I believe is good enough. Yes, you can grab that URL and make unlimited requests just like you can take the homepage URL and make unlimited requests. I guess I don't see this as an issue.
ok, i see
i was just wondering if this makes much easier, to find a sub-set of usernames and emails that are used in a website,
JSON requests will also run much faster and also it is much easier to check the response
i guess, related plugins at JED will be updated to handle this case too,
@ggppdk As for finding out existing usernames/emails that will depend on the message given I guess. You can also write a script that mimics sending the user registration and parse the output of that. Indeed JSON responses are easier but does it make a real difference with todays computing powers?
Just wondering why the maximum number of characters for a username is 99 when the field is varchar(150)
because the minimum length have to be 3 characters
Maybe I am being thick but why does an absolute minimum of three prevent you from setting a minimum of 150
sorry I was a bit confused. Should the maximum 149 because of the 0 it would be 150 Characters or I'm wrong ? @brianteeman @roland-d
Yes you are confused. You are confusing the value of an integer with the nu!best of characters in a field
Milestone |
Added: |
Since the DB size of the field is 255 characters, we can set the maximum to 255. The minimum is 3 and up as shown in the message. Those are the correct numbers I think.
What is the status of this?
its ready to test @brianteeman
I have tested this item
The checks seem to work pretty well, however I do suggest making the warning message go away once you pass validation. For example if the name "Josh" was taken and I change it to "JoshLewis", the message above remains which gives the vibe that both are taken.
Happy to see this almost done, @mxkmp29 . An impressive work by the whole team !
It was harder than it seemed at first sight.
Perhaps it should be tested specifically the UNICODE Security MECHANISMS IMPLEMENTATION of Highly Restrictive level (libraries/joomla/form/rule/username.php). I did some tests when I implemented it, but it's worthy a review. The Default Identifier Syntax code (UAX31-D1) is commented because I thought it's too restrictive. Test and insight required too.
Regards. Good job !
@JoshuaLewis thanks for the feedback, I changed it.
I have tested this item
Awesome, love the new addition! :-D Tested successfully, works great.
I have tested this item
Awesome, love the new addition! :-D Tested successfully, works great.
Looking for more testers. @brianteeman @wilsonge Can you test this as well?
Category | Administration Components Language & Strings Templates (admin) Front End | ⇒ | Administration Components Feature Request Front End Language & Strings Templates (admin) |
Labels |
@brianteeman the comments from @roland-d & @mbabker are corrected. The comment was on the wrong fields in the code. Under "Username Options" the changes are visible and working, which were discussed in this comment.
I'm willing to retest once the comment zero-24 mentioned is either addressed or explained.
I have tested this item
Works as expected. :-)
@brianteeman How does it look now (in terms of the requirements you've mentioned)? After the recent change it's working great on my end.
@brianteeman the comments from @roland-d & @mbabker are corrected. The comment was on the wrong fields in the code. Under "Username Options" the changes are visible and working, which were discussed in this comment.
I dont see the correction that @mbabker says to make in https://github.com/joomla/joomla-cms/pull/11516/files#r75477191 are you saying thats not correct?
Surely it is not correct that I can set the minimum number of characters to be greater than the maximum number of characters
Summary of Changes
- Added ajax username and email checks, which compares the entries of the database (only in Frontend)
This happens because the ajax calls only checks usernames or emails in the database, not the validation. The validation was implemented by @framontb in the username.php and contains only checks via php. There is no ajax validation, which checks the the rules from the admin-backend.
Seems daft to me to only do ajax on "if exists" and not on "if valid" kind of defeats the object of the code to me
Also if this is the case that it only checks "if exists" and not "if valid" then the error message is incorrect
The email address you entered is already in use or invalid.
@brianteeman
what you suggest is translate all the validation to ajax?
Well I dont see any point in only having half the job as ajax
There is a possible issue that needs to be considered in that with the ajax checking of existing usernames and email addresses i can quite quickly gather a list of all the users on the site
The username and email usage check is a user enumeration attack vector from a security perspective. Yes, we have this check during the current registration process too, but this one can at least be protected using a captcha, that's not possible for the AJAX request.
So, I would suggest to remove the "is username or email" already used check in this PR.
You can enter and save a lower number of "Max. number of characters" then "Min. number of characters"
This can' be correct. As you can see in my screenshot I can configure to have a username with at least 3 characters but maximum 1 character
This is not ready for a review, it still has outstanding change requests from several months ago and just through scanning a couple of files there are miscellaneous whitespace changes littered in the PR.
From a security perspective, the concept is "controversial" and this is introducing a "convenience over security" workflow by making it easier to ascertain whether a username exists on a website. Considering this is triggered by JavaScript based on typing into a field, the implementation needs to ensure that there isn't a way to turn this into a DoS attack on a site.
Hey,
Thanks for the big discussion under this PR, but I don't have the time and maybe not the knowledge about joomla and how to fix the security issues to finish this PR. Maybe someone else can continue this work, if some of these features are needed in a future version of joomla.
best regards
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-01-16 13:53:45 |
Closed_By | ⇒ | mxkmp29 |
Category | Administration Components Language & Strings Templates (admin) Front End Feature Request | ⇒ | Administration com_users Language & Strings Front End Libraries JavaScript Components Feature Request |
@mxkmp29 there are a lot of unrelated changes in this PR