Solinx - 2009-07-07 13:01:14
When I read the description of your Class, I was curious to see how you handled the balance between functionality and efficiency.
Overall it is a nifty class. As it says in the description, it has all the basic things required of a simple site.
There are some points for improvement though. Most importantly, the password. Or rather the use of it. When updating the site content, you state that the user needs to re-enter the password for security reasons.
The way it is set up to work now, it is only an annoyance and actually creates a minor security hole.
First off, the hash value shouldn't be entered in the password field, even if it is hidden. If you forget to re-enter the password, the hash is accepted as new password and you're locked out of the panel.
Second, the user can enter anything he likes and it will be accepted. This includes leaving the field empty. It can also occur that the user makes a typo. In this case, the user is again locked out of the panel.
The lack of validation against the current password is also the security hole. Anybody with access to the pc showing the admin panel can quickly enter a new pass, locking out the real administrator.
Basically the only reason a password needs to be entered is when the admin wants to change it. Otherwise the active hash definition can just be restored.
So there is a choice. Either split up the password field in two fields (Password confirmation & New password) or make it optional, writing the code so that it re-uses the active hash definition if the field is left empty. Less secure, but at least it removes the annoyance. A third option is to show a warning if the field is left empty, similar to the warning shown when an attempt is made to log in with an invalid password.
Validation is actually lacking with about every field but with the e-mail adress and login check. The e-mail message content however wasn't validated either. It can even be empty. There is no database involved, but at least some validation still seems like a good idea to me.
One of the first things I noticed was that you repeatedly created a new object to perform one function. Since you don't appear to have a constructor, it seemed to me you'd be better off making the functions static and using autocms::<functionname>. I just read you sumbitted this as php 4 compatible, never having worked with php 4 classes, I guess that's the reason for not doing this.
If I ever need a simple solution, I'll certainly consider using this class. Although I'll make some adaptions, if you don't mind.
The class description says it needs an additional class for the contact form to work, but I didn't see any need for it in the code.