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

[Steffen Kamper]

Hi Rupi,

first thx for your time to make a review.
Indeed i have to apologize as last update wasn't that solid as it should 
be. i try my best in this one.

Rupert Germann schrieb:
> 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.
>

yeah - also thought about this as bad style. Validation is done now.
I now loop through the fields so the checkbox issue is solved too.


> 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?

my idea was to use something you know from TS like 
TCEFORMS.table.field.disabled = 1. I would like to implement such TS 
later anyway.
exclude for fields is for user access, i didn't looked yet for 
processing this.
But you're right, this list is not needed as there is the positive list 
groups=>fields where a simple removal of one element is possible. Only 
disadvantage is that you may not know in which group the wanted field is.


> Hey, it was you who said that TUS is "TCA-like" so don't call
> me "Korintenkacker" :-)
>

we will test this next week :-) (hope to see you there)

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

yes, i changed that.

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

> 5. simulateUser doesn't work anymore because you removed one
> $GLOBALS['BE_USER'] to much (line 633)
> 
hm - i tested and it doesn't worked even before this patch. I fixed that 
too. Anyway this function works different too the UserAdmin->simulate. 
What is the purpose of that function, shouldn't it work same as the 
mentioned one with reload BE and exit-button on top?

> 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)
> 

see above, i first want to clarify this.

> 7. Do you test anything before you upload patches?
>
sure, it's running on my local system. But the last patch i've written 
above, bit overloaded with work last days.


> 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.
>

done in init

> 9. line 577: $subKey appears only once

this code is untouched (only moved to own function), so old issue.
> 
> 10. please rename the methods startModuleSelect,simulateUserSelect and
> languageSelect to something like getStartModuleSelect or render...
> 

changed to render...

i attach a new version if the simulateUser question is clarified.

thx, vg Steffen