Quote:
Originally Posted by nobackseat
Hey,
Wow didn't realize my post had been noted here so fast.
I realize I got increasingly sarcastic throughout the post, but I was being honest on my view of it, and I had listed plenty of examples.
I also, like before, realize that some of these issues aren't your fault, but after all they are being released under your name. I'm glad to hear you're working on them for the next release.
In my strong opinion, globals often mean that code was designed 'wrong'. There's always a better way to achieve what you want without using globals. I can understand if the way the code is setup makes it hard to transition from globals, but it's still being released with them and I was asked to give an honest review.
The jab at the encryption was mostly at how dramatic it was. There's easier ways to obtain equally secure encryption. I would call that secure, but how you encrypted it is just odd, not common at all, which just might make it more secure overall anyway.
Good luck, I'll keep checking it out every few releases.
NBS
|
No worries. Actually its good to know that you had this many problems with the registration/authentification system. This makes my latest work on them more worthy than a waste of time. XD
I wont say its a problem to point out old programming flaws from rusnak script. We've tried our best to fix many of them, but it will take a while for everything to be fixed. We took care of the top priority issues, such as password encryption, insecure cookies and while loop running only once. The others will come when we are overhauling a specific script. Of course it speeds up the process if someone brings them up to me, with or without offering a possible solution.
Regarding database, yes I agree it is a waste to grab all fields of a row from database when we only need one or two. Since we are using PDO at this point, Id use fetchColumn() in circumstances when we only need one field from a row. The database class is very well designed by Fadillzzz, and it is just about time when we begin making the best use of it.
And you are right that superglobals exist because the original script was designed in a wrong way. In Mys v1.3.1 I already got rid of database superglobals such as $GLOBALS['localhost'] and so on. You still find superglobals with the current user information such as $GLOBALS['money'] in this release though. The best way to fix this is to overhaul the user system completely. A user object can store the current user information, and it will be passed into function or class method as argument when needed. Thats what I am doing right now, it will most likely have some issues when the initial design is completed. If you are around by then, lemme know what problems you find in the script and Id appreciate comments.
Like I said before, the reason why the encryption function looks messy is that we need to compensate for users upgrading from old script. This is what the script is supposed to look like in the first place:
PHP Code:
function passencr($username, $password, $salt){
$pepper = grabanysetting("peppercode");
return hash('sha512', $pepper.$username.$password.$salt);
}
Id say it looks less messy this way, and tbh thats what it was originally designed. But then there is no way for old users using old versions of this script to update their members passwords. Instead, they have to force everyone to reset passwords. Of course ideally everyone just starts over when the next major release is available so I dont have to worry about upgrading issues. Not sure if it will happen though, but its possible.