On Mar 18, 2008, at 3:20 PM, Jochem Maas wrote:
what started out as a simple little reply bloated out into an
inpromptu brain
fart ... lots of bla .. enjoy :-)
Jason Pruim schreef:
Hi everyone,
I am attempting to add a little error checking for a very simple
login system. The info is stored in a MySQL database, and I am using
mysqli to connect to it. I have it working with the solution provided
below, but I am wondering if this is the right way to do it or if
there is a better way?
at an abstract level you might consider that your function could simply
always return a boolean (true = logged in, false = not logged in) and
that the
rest of the application retrieves all the other data via the session
(as opposed to returning half the data and storing half in the session)
I think this is what I am attempting to do... Just going about it all
wrong...
return true if it matches
if not return false which could then redirect back to login page...
}
Is it that simple? Am I trying to make things so much more complicated?
if you choose to store everything and only return authentication state
you
might also consider to abstract the storage somewhat so that other
code doesn't
have to access the session data directly. we call this concept 'loose
coupling'.
for instance:
function getUserInfo($key = null)
{
if (!isset($_SESSION['user']['loggedin']))
return null;
if (!$_SESSION['loggedin'])
return null;
$key = trim((string)$key);
if ($key == '')
return $_SESSION['user'];
if (isset($_SESSION['user'][$key]))
return $_SESSION['user'][$key];
return null;
}
this example still requires that the the consumer of getUserInfo() knows
the names of the relevant columns (from multiple tables?)
login info is stored on 1 table, while the actual records in the DB are
stored on another table. After successful login it changes from the
login table to the data table.
.. this could also
be abstracted, a simple solution would be something like:
// put these in a config file, (CKEY = 'cache key' ... just a thought)
define('CKEY_USER_NAME', 'loginName');
define('CKEY_USER_LEVEL', 'adminLevel');
define('CKEY_USER_TABLE', 'tableName');
$uName = getUserInfo( CKEY_USER_NAME );
$uLevel = getUserInfo( CKEY_USER_LEVEL );
$uLevel = getUserInfo( CKEY_USER_TABLE );
And then that would hold the info in a cache until the user hit logout
and then logged back in? I'm going to try that right after sending this
message.... That may work perfectly...
Also I'm assuming if I put these into an include file it will work just
like my other variables where I can call $pass from any page that
includes the file $pass is defined in?
... you get? ... incidentally your column names seem to be
case-sensitive,
I recommend lower or upper (depending on DBMS) case only for sql
entity names
for two reasons:
1. you avoid nitpicky irritations due to SQL case-sensitivity related
bugs
in your code.
2. if you lowercase all entity names you can write stuff like so:
$sql = "SELECT foo, bar FROM qux WHERE abc = 1 AND def=2";
which is a little more readable than this:
$sql = "SELECT FOO, BAR FROM QUX WHERE ABC = 1 AND DEF=2";
of course it should be more like:
$sql = "SELECT `foo`, `bar` FROM `qux` WHERE `abc`=1 AND `def`=2";
using case to differentiate between SQL and entity names becomes more
useful
as the queries become more complex. I also tend to then break then up
into lines:
$sql = "SELECT
q.`foo', q.`bar`,
na.`foo` AS nafoo, na.`bar` AS nabar,
noo.`foo` AS noofoo, noo.`bar` AS noobar,
FROM
`qux` AS q
LEFT JOIN
`na` AS na ON na.`qux_id` = q.`id`
LEFT JOIN
`noo` AS noo ON noo.`qux_id` = q.`id`
WHERE
(`abc`=? AND `def`=?)
AND
q.`id` IN (SELECT `qux_id` FROM `quxnobbins` WHERE
`nobbin_id`=?)
AND (
(`start_date` BETWEEN ? AND ?) OR
(`start_date` BETWEEN ? AND ?)
)";
My thinking with this is if more then 1 record is returned from the
database, then there is a issue... If only is returned then the
username/password matched and I can safely show them the info...
$rowcnt = mysqli_num_rows($loginResult);
we'll assume the original sql was suitably prepared (i.e. user values
escaped, etc).
but why not 'fix' the query and/or table so that it will only ever
return one row?
if($rowcnt !="1"){
avoid auto-casting!
if ($rowcnt !== 1) { /*...*/ }
echo "Auth failed";
die("Auth failed... Sorry");
}else{
while($row1 = mysqli_fetch_array($loginResult)) {
this 'while' is completely pointless, you know there is just one row,
no point in looping for a single iteration.
Will make that change now :)
just do:
$row = mysqli_fetch_array($loginResult);
$_SESSION['user'] = $row['loginName'];
// ... etc
$_SESSION['user'] = $row1['loginName'];
$_SESSION['loggedin'] = "YES";
"YES" is not a boolean value, I think $_SESSION['loggedin'] should be
boolean (you got deja vu here also?).
Just to double check: $_SESSION['loggedin'] = TRUE; //Is a boolean while:
$_SESSION['loggedin'] = "TRUE"; // is not correct?
check the following code to see why:
$_SESSION['loggedin'] = "FALSE";
if ($_SESSION['loggedin'])
echo "your logged in!";
$table = $row1['tableName'];
$adminLevel = $row1['adminLevel'];
$authenticated = "TRUE";
again the boolean should be boolean!
echo "<BR>authentication complete";
with regard to seperation of responsibilities: the function should
really be either attempting an authentication *or* outputting some
message
regarding the result of the authentication attempt but *not* both.
That was added for debugging, helping me track down where the error was.
in practice this means my recommendation would be to remove the echo
statements
from the function and have the code that calls this function be
responsible for
outputting feedback ... imagine if you need to, someday, perform an
authentication
without [direct] output? or you need to change the outputted message
under certain
conditions (conditions which are outside the scope of this function)?
a function should, as much as is possible, do one thing only (and do
it well), otherwise,
I guess, it would be called a functions. ;-)
}
return Array($table, $authenticated, $adminLevel);
pretty much the rest of the world writes 'Array()' as 'array()' .. the
convention
being that built in functions and lang constructs are always typed
lowercase. some
people write things like isSet($foo); ... but they are 'wrong' :-)
I thought I saw on the php.net page that it was Array() :)
I generally try to distinguish between userland and php functions by
using lowercase
for php funcs and CamelCase naming schemes for userland functions.
I see what you're getting at though... And I need to do that more
through my applications..
--
"ok, porky pig say your line."
--
Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@xxxxxxxxxx
