Add a new dropdown 'token type' for numeric tokens. #1104

pefeigl · 2018-07-30T11:53:35Z

This commit adds a new selection to let you choose between the normal tokens and numeric-only tokens.

New feature # :
Dev:
Dev:

This commit adds a new selection to let you choose between the normal tokens and numeric-only tokens.

Shnoulle · 2018-07-30T12:37:28Z

Such feature mus be in plugin

pefeigl · 2018-07-30T13:36:46Z

Such feature must be in plugin

Can this be done in a plugin?

Shnoulle · 2018-07-30T14:08:18Z

Except with some hack, i don't think currently.

lacrioque · 2018-07-30T14:45:19Z

Hey, thanks a lot.
We like this change and we will merge it, but not just now, we will wait for after the next release.
We get asked for numerical tokens often, so thank you very much for the effort made!

pefeigl · 2018-07-31T05:24:40Z

Great to hear that! Please just ping me when you are considering merging this, then I'll rebase it and make sure it still works. One caveat: The random generator for the numbers is just randomChars, which might be unsafer than the default one. If necessary, I can look into changing that.

dominikvitt · 2019-04-30T15:18:57Z

This is a great feature.
Can you close this one and create a new pull request in develop branch?
All new features go there.
As soon as you are finished, we'll merge it.

TonisOrmisson · 2019-05-01T13:22:09Z

I would like to have a "startFrom' value there. Typically I like having similar tokens with the same length so I would like to start tokens eg from 10000,

Shnoulle · 2019-05-02T08:18:48Z

@TonisOrmisson it's exacly why i ask it to be in plugin … Create an event + a core plugin to set it as number allow creating another plugin to have

start from plugin
fixed part plugin (for example test{number})
only capitals plugin
… … lot of possibility.

pefeigl · 2019-05-02T09:01:45Z

I'm willing to port the patch to the current dev branch, but could you please first discuss whether it will be accepted or not? [not as a plugin, but as a part of core, as it is still impossible to write as a plugin at this point, I think].

Shnoulle · 2019-05-02T09:03:30Z

it is still impossible to write as a plugin at this point

I confirm, personnaly i ask for a new event in core to allow adding this.

TonisOrmisson · 2019-05-02T09:06:11Z

application/models/Survey.php

@@ -472,6 +473,7 @@ public function rules()
            array('format', 'in', 'range'=>array('G', 'S', 'A'), 'allowEmpty'=>true),
            array('googleanalyticsstyle', 'numerical', 'integerOnly'=>true, 'min'=>'0', 'max'=>'2', 'allowEmpty'=>true),
            array('autonumber_start', 'numerical', 'integerOnly'=>true, 'allowEmpty'=>true),
+            array('tokentype', 'in', 'range'=>array('default','numeric'), 'allowEmpty'=>true),


I would suggest using constants for the types. Much easier to refer to later and refactor if needed

TonisOrmisson · 2019-05-02T09:06:56Z

application/models/Token.php

-        return str_replace(array('~', '_'), array('a', 'z'), Yii::app()->securityManager->generateRandomString($iTokenLength));
+        if($tokenType == 'numeric') {
+            return randomChars($iTokenLength, '123456789');
+        } else {


no else is needed here if true above will return

TonisOrmisson · 2019-05-02T09:10:09Z

application/views/admin/survey/subview/accordion/_tokens_panel.php

+            <div class="form-group">
+                <label class=" control-label" for='tokentype'><?php  eT("Select token type:"); ?></label>
+                <div class="">
+                    <select name='tokentype' id='tokentype' class="form-control">


please use CHtml::dropDownList here. That's why we have a framework for

maziminke · 2019-05-03T07:18:45Z

I like this feature. While using a plugin is nice to separate code, I consider this a core feature which should go into LS 4 directly.

Shnoulle · 2019-05-03T07:24:08Z

@maziminke : The reason why i think it's best by event is allowing forcing to anythging else … One user can want number only, one another uppercase only etc …

dominikvitt · 2019-05-07T11:59:14Z

@pefeigl: Please port this PR to the current develop branch.
It should be developer as core feature and all suggestions mentioned above implemented (if possible).
We hope to have this feature merged into develop before final LS4 release.

Shnoulle · 2019-07-05T09:00:30Z

@dominikvitt : then need another Pull request for Omit ambiguous characters from token

Again and agin : best solution : add an event for plugin to do anything with token …

olleharstedt · 2019-07-05T09:11:01Z

@Shnoulle Why would this be put in a plugin instead of core?

olleharstedt · 2019-07-05T09:13:55Z

application/controllers/admin/tokens.php

@@ -691,6 +691,7 @@ public function addnew($iSurveyId)

            $aData['thissurvey'] = getSurveyInfo($iSurveyId);
            $aData['surveyid'] = $iSurveyId;
+            $aData['tokenType'] = !empty(Token::model($iSurveyId)->survey->tokentype) ? Token::model($iSurveyId)->survey->tokentype : 'default';


When ported to LS4, you can use the ?? operator here, since LS4 will require PHP 7+.

tammoterhark · 2019-07-05T09:14:46Z

Just to make things more complicated.

From a user point of view: please put this in core and not in a plugin.

Example from LastPass.

I would already be very glad with the (default) omission of ambiguous charaters, for people that have to fill in by hand.

Shnoulle · 2019-07-05T09:15:54Z

@olleharstedt : because see #1104 (comment)

I would like to have a "startFrom' value there. Typically I like having similar tokens with the same length so I would like to start tokens eg from 10000,

Or Omit ambiguous characters from token (most valuable in my opinion than only number)

I ever have user with specificcode{number} only token code. And why not only alpha (not numeric), only uppercase (great to have too)

This pull request do ONE system when we can need a lot more … Adding a event + a plugin for numeric only care all of options …

Shnoulle · 2019-07-05T09:18:23Z

@tammoterhark is a solution for all too, but : more and more system to care by core dev in LimeSurvey … need a test here for combination.

olleharstedt · 2019-07-05T09:19:54Z

We can do both. Add new core features and event to allow more customization.

olleharstedt · 2019-07-05T09:21:01Z

Also note that Travis fails because the newly installed database column is different from the upgraded one:

(Upgraded from db version 258) Comparing field name "allowNull" for column "tokentype" in table "lime_surveys": upgraded value: false; fresh install value: true
Failed asserting that true matches expected false.

Shnoulle · 2019-07-05T09:39:40Z

We can do both. Add new core features and event to allow more customization.

? Why do both , in my opinion, needed

Plugin event and one or more core plugins
@tammoterhark solution to do whole in core

Jan-E · 2019-07-19T05:44:17Z

We can do both. Add new core features and event to allow more customization.

How about a change like this for adding a new event customToken:

diff --git a/application/models/Token.php b/application/models/Token.php
index e4b90ce..3005821 100644
--- a/application/models/Token.php
+++ b/application/models/Token.php
@@ -237,7 +237,19 @@ abstract class Token extends Dynamic
      */
     public static function generateRandomToken($iTokenLength)
     {
-        return str_replace(array('~', '_'), array('a', 'z'), Yii::app()->securityManager->generateRandomString($iTokenLength));
+        /**
+         * We fire the customToken event
+         */
+        $event = new PluginEvent('customToken');
+        // $surveyId = $this->dynamicId; <- fails
+        $surveyId = isset($_SESSION['LEMsid']) ? intval($_SESSION['LEMsid']) : NULL;;
+        $event->set('surveyId', $surveyId);
+        $event->set('iTokenLength', $iTokenLength);
+        $event->set('generatedToken', '');
+        App()->pluginManager->dispatchEvent($event);
+        $token = $event->get('generatedToken');
+        if ($token == '') $token = str_replace(array('~', '_'), array('a', 'z'), Yii::app()->securityManager->generateRandomString($iTokenLength));
+        return $token;
     }
 
     /**

The idea is that a plugin that subscribes to the event customToken can set a $token.
If the token is not generated by a plugin the existing function will be used.

I tested this with a plugin that used this function to generate the token:

/**
 * The custom generate function
 */
public function generateCustomToken()
{
    $event = $this->getEvent();
    $iSurveyID=$event->get('surveyId');
    if (!$this->get('enabled', 'Survey', $iSurveyID, false)) {
        //echo "<pre>pkugin not active for iSurveyID {$iSurveyID} ".$this->get('enabled', 'Survey', $iSurveyID, false)."</pre>";;
        return;
    }
    $iTokenLength = $event->get('iTokenLength');
    $event->set('generatedToken', randomChars($iTokenLength, '123456789'));
    //echo "<pre>iSurveyID = {$iSurveyID} generatedToken ".$event->get('generatedToken')." ".$this->get('enabled', 'Survey', $iSurveyID, false)."</pre>\n";
}

The token is now generated as numeric, just like the purpose of this PR: randomChars($iTokenLength, '123456789') Other functions are possible as well, for instance like the one in Omit ambiguous characters from token

Jan-E · 2019-07-19T22:31:49Z

I now have these functions:

Numeric tokens
Without ambiguous characters
CAPITALS ONLY

See the functions in the source code of our TFRcustomToken plugin

Shnoulle · 2019-07-20T10:52:49Z

@Jan-E 👍 for $event = new PluginEvent('afterGgenerateCustomToken');

With

$token = str_replace(array('~', '_'), array('a', 'z'), Yii::app()->securityManager->generateRandomString($iTokenLength));
$event = new PluginEvent('afterGgenerateCustomToken');
$event->set('surveyId', $this->getSurveyId());
$event->set('oToken', $this);
$event->set('token', $token);
App()->pluginManager->dispatchEvent($event);
$token = $event->get('token');
return $token;

Then :

sure to have current sid,
can check some value indide current token (Object),
can get the current token and update or totally replace.
Without plugin : nothings are updated

Jan-E · 2019-07-20T12:06:29Z

Comment moved to #1307 (comment)

Jan-E · 2019-07-20T12:33:06Z

Let us continue the discussions in the new PR without any code changes yet:
#1307
This way we separate the discussions about adding a new dropdown in core and adding a new event. better not get too off-topic here.

Shnoulle · 2019-07-20T14:03:44Z

Ah right … no surveyId because it's a static function,

Think it's best to have $this totally when generate token, then 2 solution:

Add the event in

LimeSurvey/application/models/Token.php

Line 223 in e5d1175

public function generateToken()

And update

LimeSurvey/application/controllers/admin/tokens.php

Line 990 in e5d1175

$token->token = Token::generateRandomToken($aData['tokenlength']);

to $token->generateToken()
Update

LimeSurvey/application/models/Token.php

Line 244 in e5d1175

public static function generateRandomToken($iTokenLength)

to public static function generateRandomToken($iTokenLength,$iSurveyId=null,$aTokenAttributes=null)

Looking at usage in develop, not a lot of place for usage

$ grep -Ri "generateRandomToken" *
application/models/Token.php:        $this->token = $this::generateRandomToken($iTokenLength);
application/models/Token.php:            $this->token = $this::generateRandomToken($iTokenLength);
application/models/Token.php:    public static function generateRandomToken($iTokenLength)
application/models/Token.php:                $newtoken = $this::generateRandomToken($iTokenLength);
application/controllers/admin/tokens.php:                    $token->token = Token::generateRandomToken($aData['tokenlength']);

But 1 seemls the really BEST solution. Maybe move generateRandomToken to private static function
$_SESSION usage must be totally avoided…

Jan-E · 2019-07-20T19:18:55Z

To all participants in and subscribers to this PR: please review Add a new event to generate custom tokens at #1307
After the review I will create a new PR to the develop branch

Jan-E · 2019-07-21T14:10:37Z

@Shnoulle approved an already outdated version of #1307
Does anyone here have additional comments?

Shnoulle · 2019-07-21T18:04:58Z

@Jan-E , i merge it in develop (and take responsability of fixing if needed).

Jan-E · 2019-07-21T21:44:14Z

@Jan-E , i merge it in develop (and take responsability of fixing if needed).

@Shnoulle Do you mean adding a dropdown 'token type' for numeric tokens?

There are several people here that want to see it in core, not in a plugin. I know you are a fan of plugins, but having this in core as well might be equally inportant.

Jan-E · 2019-07-22T16:50:54Z

New PR for afterGenerateToken plus customToken plugin is here now: #1308

If this PR is merged every LimeSurvey admin can activate the plugin and choose for the 3 token types in the plugin:

Numeric tokens
Without ambiguous characters
CAPITALS ONLY

Please review the PR.

Add a new dropbox 'token type' for numeric tokens.

fca3fec

This commit adds a new selection to let you choose between the normal tokens and numeric-only tokens.

Shnoulle requested review from Shnoulle and removed request for Shnoulle July 30, 2018 12:37

pefeigl changed the title ~~Add a new dropbox 'token type' for numeric tokens.~~ Add a new dropdown 'token type' for numeric tokens. Jul 31, 2018

TonisOrmisson reviewed May 2, 2019

View reviewed changes

olleharstedt reviewed Jul 5, 2019

View reviewed changes

Jan-E mentioned this pull request Jul 20, 2019

New Feature: Add an event to generate custom tokens #1307

Closed

Shnoulle closed this Jul 21, 2019

Add a new dropdown 'token type' for numeric tokens. #1104

Add a new dropdown 'token type' for numeric tokens. #1104

Conversation

pefeigl commented Jul 30, 2018 • edited Loading

Shnoulle commented Jul 30, 2018

pefeigl commented Jul 30, 2018 via email

Shnoulle commented Jul 30, 2018

lacrioque commented Jul 30, 2018

pefeigl commented Jul 31, 2018

dominikvitt commented Apr 30, 2019

TonisOrmisson commented May 1, 2019

Shnoulle commented May 2, 2019

pefeigl commented May 2, 2019 via email

Shnoulle commented May 2, 2019

TonisOrmisson May 2, 2019

Choose a reason for hiding this comment

TonisOrmisson May 2, 2019

Choose a reason for hiding this comment

TonisOrmisson May 2, 2019

Choose a reason for hiding this comment

maziminke commented May 3, 2019

Shnoulle commented May 3, 2019

dominikvitt commented May 7, 2019

Shnoulle commented Jul 5, 2019

olleharstedt commented Jul 5, 2019

olleharstedt Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

tammoterhark commented Jul 5, 2019

Shnoulle commented Jul 5, 2019 • edited Loading

Shnoulle commented Jul 5, 2019

olleharstedt commented Jul 5, 2019

olleharstedt commented Jul 5, 2019

Shnoulle commented Jul 5, 2019

Jan-E commented Jul 19, 2019 • edited Loading

Jan-E commented Jul 19, 2019

Shnoulle commented Jul 20, 2019 • edited Loading

Jan-E commented Jul 20, 2019 • edited Loading

Jan-E commented Jul 20, 2019

Shnoulle commented Jul 20, 2019 • edited Loading

Jan-E commented Jul 20, 2019 • edited Loading

Jan-E commented Jul 21, 2019

Shnoulle commented Jul 21, 2019

Jan-E commented Jul 21, 2019 • edited Loading

Jan-E commented Jul 22, 2019

pefeigl commented Jul 30, 2018 •

edited

Loading

olleharstedt Jul 5, 2019 •

edited

Loading

Shnoulle commented Jul 5, 2019 •

edited

Loading

Jan-E commented Jul 19, 2019 •

edited

Loading

Shnoulle commented Jul 20, 2019 •

edited

Loading

Jan-E commented Jul 20, 2019 •

edited

Loading

Shnoulle commented Jul 20, 2019 •

edited

Loading

Jan-E commented Jul 20, 2019 •

edited

Loading

Jan-E commented Jul 21, 2019 •

edited

Loading