websavers Posted October 4, 2019 Share Posted October 4, 2019 (edited) Six years ago someone opened a feature request on the appropriate community here to have WHMCS remove case sensitivity from answer checking. This is because case sensitivity causes unnecessary friction with customers who often spell things with differing cases in security question answers. They're *not* the same as passwords. Sadly the request was archived by WHMCS. Even more sad: it would take WHMCS devs about 5 minutes to change from strcmp to stricmp, yet we've got to invest quite a bit longer into the code below just to have it be less effective. Here's my incomplete code to resolve this via hooks. It inserts javascript into the necessary pages that forces everything to lowercase. add_hook('AdminAreaFooterOutput', 1, 'ws_security_question_ans_lower_case'); add_hook('ClientAreaFooterOutput', 1, 'ws_security_question_ans_lower_case'); function ws_security_question_ans_lower_case($vars){ if ($vars['filename'] == 'register' || //client area create acct $vars['filename'] == 'clientsprofile' || //admin edit client $vars['filename'] == 'pwreset' || //password reset process ($vars['filename'] == 'cart' && $_REQUEST['a'] == 'checkout') || //client area cart $_REQUEST['action'] == 'changesq'): return <<<HTML <script> jQuery(document).ready(function($){ targets = $('input[name="securityqans"],input[name="currentsecurityqans"],input[name="securityqans2"],input[name="answer"]#inputAnswer'); if (targets.length > 0){ $(targets).each(function(){ $(this).keyup(function(){ $(this).val($(this).val().toLowerCase()); }); }); } }); </script> HTML; endif; } This code has one problem: it will break the ability for clients to change their own security question if their existing question has upper case letters in it. Anyone know how to overcome this easily? Clearly the better solution is for WHMCS to eitheruse stricmp for all security question answers or provide an option to do so, but in the absence of them helping improve usability of their software, this is what we've got to work with. Edited October 4, 2019 by websavers 0 Quote Link to comment Share on other sites More sharing options...
websavers Posted October 4, 2019 Author Share Posted October 4, 2019 In the interest of improving security question management, we've also written this hook to prompt the user to change their security question after changing other profile details: add_hook('ClientAreaFooterOutput', 1, function($vars){ if ($vars['filename'] == 'clientarea' && $_REQUEST['action'] == 'details' && $_REQUEST['success'] == 1): return <<<HTML <script> jQuery(document).ready(function($){ $('.alert-success').after('<div class="alert alert-error text-center">Reminder: when updating your profile details it is also recommended to <a href="clientarea.php?action=changesq">change your Security Questions and Answers</a>.</div>'); }); </script> HTML; endif; }); 0 Quote Link to comment Share on other sites More sharing options...
brian! Posted October 4, 2019 Share Posted October 4, 2019 13 hours ago, websavers said: This code has one problem: it will break the ability for clients to change their own security question if their existing question has upper case letters in it. Anyone know how to overcome this easily? easily? not ideal, but you could not enable the JS if the current security answer contains an uppercase letter - and probably the simplest test for that would be to compare the security answer with its lowercased equivalent and only if they match (e.g they contain no uppercase characters), is the JS added... so if ($vars['filename'] == 'register' || //client area create acct $vars['filename'] == 'clientsprofile' || //admin edit client $vars['filename'] == 'pwreset' || //password reset process ($vars['filename'] == 'cart' && $_REQUEST['a'] == 'checkout') || //client area cart $_REQUEST['action'] == 'changesq'): return <<<HTML becomes... if ($vars['filename'] == 'register' || //client area create acct $vars['filename'] == 'clientsprofile' || //admin edit client $vars['filename'] == 'pwreset' || //password reset process ($vars['filename'] == 'cart' && $_REQUEST['a'] == 'checkout') || //client area cart $_REQUEST['action'] == 'changesq' && $vars['clientsdetails']['securityqans'] == strtolower($vars['clientsdetails']['securityqans'])) : return <<<HTML so if the user changes their answer to all lowercase, then the JS will be active in the future... but whilst the answer contains an uppercase character, the JS isn't added and they can continue to use uppercase. 13 hours ago, websavers said: Sadly the request was archived by WHMCS. Even more sad: it would take WHMCS devs about 5 minutes to change from strcmp to stricmp, yet we've got to invest quite a bit longer into the code below just to have it be less effective. absolutely. as you did with your second hook, it might be an idea to post a link to this thread in that feature request thread - even if this isn't a perfect solution, it's better than nothing... or waiting for an archived request to become completed. 13 hours ago, websavers said: In the interest of improving security question management, we've also written this hook to prompt the user to change their security question after changing other profile details: the only thing I would possibly change with that hook code would be to use a Language Override string rather than hardcoding it in one language... though that is by no means a criticism of the hook as is because using a LO would make the answer require a longer explanation and would either require the hook to check if a LO existed and if so use it, or else fall back on hardcoded text. 0 Quote Link to comment Share on other sites More sharing options...
websavers Posted October 4, 2019 Author Share Posted October 4, 2019 Hey Brian, 1 minute ago, brian! said: so if the user changes their answer to all lowercase, then the JS will be active in the future... but whilst the answer contains an uppercase character, the JS isn't added and they can continue to use uppercase. I was considering that! The tricky part is tied to this statement: "but whilst the answer contains an uppercase character," That has to apply to *existing* answers already stored, no? The front-end/JS of the page can't see that value. I believe what you're identifying in the code is actually the 'new value' to be entered when *changing* the security question in the logged in Client Area. Though perhaps I'm misunderstanding the logic of your suggested changes? 6 minutes ago, brian! said: as you did with your second hook, it might be an idea to post a link to this thread in that feature request thread - even if this isn't a perfect solution, it's better than nothing... or waiting for an archived request to become completed. Yes! I tried; but it refused to allow me to update the thread. I assumed because it was archived it no longer allows updates. 7 minutes ago, brian! said: the only thing I would possibly change with that hook code would be to use a Language Override string rather than hardcoding it in one language... though that is by no means a criticism of the hook as is because using a LO would make the answer require a longer explanation and would either require the hook to check if a LO existed and if so use it, or else fall back on hardcoded text. Thanks for the suggestion! We've got a lot of hard-coded english due to very heavy javascript modifications to WHMCS (often in separate JS files, granted this is not). I figure anyone using language translations should know how to swap that out fairly easily with an LO string var anyway. 0 Quote Link to comment Share on other sites More sharing options...
websavers Posted October 4, 2019 Author Share Posted October 4, 2019 (edited) Oh! I see what you're saying with that. You're positive that $vars['clientsdetails'] ['securityqans'] exists unencrypted in the $vars array for comparison like that? I guess I'll have to test it 🙂 One remaining problem: we also target the password reset process: $vars['filename'] == 'pwreset' || //password reset process If an existing SQ doesn't have lowercase it'll break the pw reset process, which is, perhaps, the most common point of failure for the SQ system as a whole. And I don't think we have access to $vars['clientsdetails'] ['securityqans'] in that template at all. I guess excluding that page from forcing lowercase will still help us out in a number of other locations, just not during a password reset. Thoughts? Edited October 4, 2019 by websavers New details. 0 Quote Link to comment Share on other sites More sharing options...
websavers Posted October 4, 2019 Author Share Posted October 4, 2019 (edited) We could do this: if (empty($vars['clientsdetails']['securityqans'])){ $results = localAPI('GetClientsDetails', array( 'clientid' => $_SESSION['uid'], )); $existing_sqa = $results['client']['securityqans']; } else{ $existing_sqa = $vars['clientsdetails']['securityqans']; } $existing_sq_is_lowercase = ($existing_sqa == strtolower($existing_sqa))? true:false; With this in the existing conditional: ($_REQUEST['action'] == 'changesq' && $existing_sq_is_lowercase ) However I suspect the password reset process will still not have access to the securityqans variable and $_SESSION['uid'] is surely not going to be set yet at that stage, so also making it look like this probably will fail: ($vars['filename'] == 'pwreset' && $existing_sq_is_lowercase) Edited October 4, 2019 by websavers 0 Quote Link to comment Share on other sites More sharing options...
brian! Posted October 4, 2019 Share Posted October 4, 2019 14 minutes ago, websavers said: That has to apply to *existing* answers already stored, no? The front-end/JS of the page can't see that value. I believe what you're identifying in the code is actually the 'new value' to be entered when *changing* the security question in the logged in Client Area. Though perhaps I'm misunderstanding the logic of your suggested changes? no that variable is the existing value already stored in the database and passed to the template in the $clientsdetails array - if they're logged in and they've already entered an answer previously, then it should be in the array. 14 minutes ago, websavers said: Oh! I see what you're saying with that. You're positive that $vars['clientsdetails'] ['securityqans'] exists unencrypted in the $vars array for comparison like that? I guess I'll have to test it 🙂 yes - that's what i'm saying... I doubled checked that before posting! 🙂 17 minutes ago, websavers said: One remaining problem: we also target the password reset process: $vars['filename'] == 'pwreset' || //password reset process If an existing SQ doesn't have lowercase it'll break the pw reset process, which is, perhaps, the most common point of failure for the SQ system as a whole. And I don't think we have access to $vars['clientsdetails'] ['securityqans'] in that template at all. this bit might be release dependent.... in that, I don't think v7.8 uses pwreset as a filename (or it's FU dependent)... are you testing this on v7.7.1 ?? 34 minutes ago, websavers said: I guess excluding that page from forcing lowercase will still help us out in a number of other locations, just not during a password reset. Thoughts? once I know which version you're trying this on, I can quickly test it at the weekend... 0 Quote Link to comment Share on other sites More sharing options...
websavers Posted October 7, 2019 Author Share Posted October 7, 2019 On 10/4/2019 at 1:31 PM, brian! said: this bit might be release dependent.... in that, I don't think v7.8 uses pwreset as a filename (or it's FU dependent)... are you testing this on v7.7.1 ?? I was; just completed the upgrade to 7.8.3 and tested further and updated the code (also improved readability). Notes: Thanks to your suggested changes above, changing the security question in the Client Area it will successfully accommodate for the case where the existing password is not yet forced lowercase. Since we can't access the existing security question answer programmatically and securely during the password reset process (as the user shouldn't yet be authenticated), the same trick will not work during the password reset process (as $_SESSION['uid'] should not be set). This could be a problem if the client had originally entered a security question answer with caps, doesn't realize the system auto-forces lowercase, then attempts resetting their password. Upon entering their correctly cased SQ answer, it will fail to validate. add_hook('AdminAreaFooterOutput', 1, 'ws_security_question_ans_lower_case'); add_hook('ClientAreaFooterOutput', 1, 'ws_security_question_ans_lower_case'); function ws_security_question_ans_lower_case($vars){ $changesq_page_new_fields = false; $changesq_page_existing_target = ''; $checkout_page = false; if ($vars['filename'] == 'clientarea' && $_REQUEST['action'] == 'changesq'){ $changesq_page_new_fields = true; if ( is_int($_SESSION['uid']) ){ $results = localAPI('GetClientsDetails', array('clientid' => $_SESSION['uid'])); $existing_sqa = $results['client']['securityqans']; if ($existing_sqa === mb_strtolower($existing_sqa)){ $changesq_page_existing_target = ',input[name="currentsecurityqans"]'; } } } if ($vars['filename'] == 'cart' && $_REQUEST['a'] == 'checkout') $checkout_page = true; if ($vars['filename'] == 'register') $registration_page = true; if ($vars['filename'] == 'clientsprofile') $admin_client_edit_page = true; if ( $admin_client_edit_page || $registration_page || $checkout_page || $changesq_page_new_fields ): return <<<HTML <script> jQuery(document).ready(function($){ targets = $('input[name="securityqans"],input[name="securityqans2"]$changesq_page_existing_target'); if (targets.length > 0){ $(targets).each(function(){ $(this).keyup(function(){ $(this).val($(this).val().toLowerCase()); }); }); } }); </script> HTML; endif; } 0 Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.