[TYPO3-core] RFC: #11019: User Setup Rewrite #4

[Rupert Germann]

hi Steffen,

review continued:

1. I have a problem with these lines (148)
...
// save all submitted values if they are no array (arrays are with 
// table=be_users)
foreach($d as $key => $value) {
  if (!is_array($value)) {
    $BE_USER->uc[$key] = $value;
  }
}

as you already wrote in the comment you write ALL non-array values from
$_POST[data] to be_user->uc. 
That breaks the most important security rule in web application development:
DONT TRUST ANY INPUT!

please validate the inserted values against TUS->columns.

2. I don't like the idea of $GLOBALS['TUS']['ctrl']['hideFields'] - not so
much because it doesn't work in your patch (wrong var name used in line
403: should be $fieldname instead of $field)
The more important reason is that in TCA all lists are whitelists (showitem,
showRecordFieldList) and you also use TUS->groups->fields so I don't see a
need to change this principle by introducing the first blacklist.
Why don't you use a property like 'exclude' for each field in columns? 
Hey, it was you who said that TUS is "TCA-like" so don't call
me "Korintenkacker" :-)

3. TCA uses "itemsProcFunc" TUS uses "itemProcFunc", should be both the
same.

4. simulateUser select is not shown (wrong itemProcFunc in TUS) 

5. simulateUser doesn't work anymore because you removed one
$GLOBALS['BE_USER'] to much (line 633)

6. when simulating a user the access check doesn't work correctly. if a non
admin user is simulated the simulateUser field disappears (didn't so in
TYPO3 4.2.6)

7. Do you test anything before you upload patches?

8. calling $GLOBALS['BE_USER']->isAdmin() again for each column and group in
TUS is not needed. Should be read once and stored in the class.

9. line 577: $subKey appears only once

10. please rename the methods startModuleSelect,simulateUserSelect and
languageSelect to something like getStartModuleSelect or render...

greets
rupert










Steffen Kamper wrote:

> Hi Rupi,
> 
> Rupert Germann schrieb:
>> hi Steffen,
>> 
>> I had a look to your v2 patch and have some remarks:
>> 
>> +       protected function checkAccess($level) {
>> +               if ($level = 'admin') {
>> +                       return $GLOBALS['BE_USER']->isAdmin();
>> +               }
>> +       }
>> 
>> this will always be TRUE ;-)
>>
> 
> sure, was a typo
> 
>> and as Olly said: please move the $GLOBALS[...]-> changes to another
>> patch. Or do them at least consequently - and not like in this line:
>> 
>> <input type="submit" name="data[setValuesToDefault]" value="'
>> $LANG->getLL('setToStandard').'" onclick="return confirm(\''
>> $GLOBALS['LANG']->getLL('setToStandardQuestion').'\');" />'
>>  
> 
> I reverted now all $GLOBALS in functions having globals defined. Will do
> in seperate cleanup.
> 
> vg Steffen