Re: LiveUser_Auth_Propel Container 0.1

Georg Gell wrote:

>>if you insist to overwrite login() please not the following: your
>>login() implementation takes a blacklist approach. i think its better to
>>check if the person is active (or if active is not enabled), instead of
>>checking the user is not active.
> 
> 
> Both are the same: I check if the is_active check is enabled _and_ user
> is not active, then i _don't_ log in, you check if if the is_active
> check is disabled _or_ the user is active, then you _do_ log in. The
> result is the same.
> 
> 					me		you
> is_active not enabled:			login		login
> is_active enabled, is_active=true	login		login
> is_active enabled, is_active=false	no login	no login
> 
> I think my comment is misleading

I know its the same .. more or less .. I just think its better to 
whitelist .. as in .. is it ok then pass and not the other way around .. 
seems more sensible in security relevant code ..

> For me the big advantage of Propel is that the database object itself is
> private, and only accessible by get/set methods. So I can never forget
> to escape some input or things like that. This is a feature, not a bug ;)

Sure. I see your point. Not that we forget to escape things in our code, 
but I guess this is getting into philosophic arguments :-)

> I am already happy with the getProperty method. When I overload this, I
> can translate it to a get to the user object. And a quick grep in the
> LiveUser code shows that all accesses to $propertyValues for the auth
> object use getProperty, so for the auth object I think that I can leave
> the propertyValues array to what auth_common sets it. And to un/freeze
> the user data:
> $propertyValues['Propel']['userObject']=$this->user;
> and I keep my Propel user object.

Yeah. We really try to prefix anything with an underscore that is 
internal, but I think we havent done so with the properties which we 
might need to do. Should be just a bit of tweaking in the init() method 
to get it to work.

> The biggest problem I have is this:
> the access to $propertyValues is distributed over so many files that it
> is very difficult to find out, what keys/values are needed, and where
> they are read/written.
> For me the perfect solution would be to use getProperty/setProperty
> methods everywhere, because then I would only have to overload one
> method, and get the values from $propertyValues or the related Propel
> objects.

And that would drive me nuts :-)

>>hmm maybe we can remove propertyValues from the common implementation,
>>so that you can use overloading?

But if I do remove the definition of the property, then you could use 
__get() and __set() to overload things as well.

> The problem with the common class is that it expects all database
> results as arrays. But Propel results are objects.
> This will not be a problem for the permission container, because I can
> use the interface ArrayAccess for those classes, because i can set my
> own base class for those objects, so I think this will be easier.
> But for the auth container, the users might need their own base class,
> so I think I will just overload login and getProperty. And everything
> should work well?

and add readUserData() :-)

> btw what are the setExternalValues(), externalValuesMatch() used for?
> and un/freeze() only to store the properties in a session?

those are there to be able to check values in arrays that need to match 
on the next request in order to allow unfreezing. this is useful to add 
an ip or user agent check

for example:
      *           'externalValues' => array(
      *                  'values'      => &$_SERVER,
      *                  'keysToCheck' => array('HTTP_USER_AGENT')
      *           ),

regards,
Lukas