Add support for custom auth token expire date in UI and API #23340
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
| if (!empty($_POST['token_expire_date'])) { | ||
| $tokenExpireDate = \Piwik\Request::fromRequest()->getStringParameter('token_expire_date'); |
There was a problem hiding this comment.
What is that check supposed to do? Do you want to ensure it's sent by POST? Currently if the value would be sent by GET and POST, it would actually use the GET value. The already existing code might already haven been broken 🤔
You could simply use \Piwik\Request::fromPost() for that.
And it might be beneficial to only call \Piwik\Request::fromPost() once and assign it to a variable that can be reused.
| bool $hasTokenExpiryDate = false, | ||
| string $tokenExpiryDate = null |
There was a problem hiding this comment.
What are those new parameters for? They seem to be unused.
| name="has_expiration" | ||
| :title="translate('UsersManager_TokenExpireDateCheckboxLabel')" | ||
| :required="false" | ||
| :inline-help="translate('UsersManager_TokenExpireDateCheckboxHelp')" |
There was a problem hiding this comment.
That one has a placehoder, which currently has no provided value.
| <input | ||
| type="text" | ||
| name="token_expire_date" | ||
| :value="tokenExpireDate" |
There was a problem hiding this comment.
wondering if it could make sense to have a required attribute here, that is set based on the value of tokenHasExpiration
|
|
||
| $noDescription = null; | ||
| $description = ''; | ||
| if (!empty($_POST['description'])) { |
There was a problem hiding this comment.
As mentioned above: we should never directly access $_POST. Better to use Request::fromPost.
| } elseif (isset($_POST['description'])) { | ||
| $noDescription = true; | ||
| } |
There was a problem hiding this comment.
Can't that be simplified in some way? If I see that correctly the variable is used to be passed through the template to vue.js. The vue component assumes the value to be passed as boolean, so passing the default null value, that is set, if the POST parameter is missing, might actually result in unexpected behavior.
| try { | ||
| $invalidExpireDate = true; | ||
| if (preg_match('/^(\d{4})-(\d{2})-(\d{2})$/', $tokenExpireDate)) { | ||
| Date::factory($tokenExpireDate); |
There was a problem hiding this comment.
Maybe some check if the date is actually in the future might be helpful. Otherwise someone might by accident create a token that is already expired and would never show up in the list of tokens afterwards.
Description:
Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.
Review