Re: Is this the best way?

Jason Pruim schreef:

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

start from scratch again?

I want the pages to check to see if the person is still logged in and if they are, then it's pulling live data from the database... So maybe I should edit my authentication function...

maybe. there are two different things being confused: 1. checking logged in state. 2. attempting to login. function getUserData() { if (isAuthenticatedUser()) return $_SESSION['user']['data']; return null; } function isAuthenticatedUser() { return (isset($_SESSION['user']['authenticated']) && $_SESSION['user']['authenticated']); } function authenticateUser($u, $p, $cc = false) { if (($iau = isAuthenticatedUser()) && !$cc) throw Exception('Already logged in!'); $cmd = $iau ? 'verify account' : 'login'; if (!($p = trim($p)) || !($u = trim($u))) throw Exception('Cannot '.$cmd.' without credentials!'); $p = mysql_real_escape_string($p); $u = mysql_real_escape_string($u); if (!($res = mysql_query("SELECT * FROM users WHERE 'pwd'='$p' AND usr='$u'"))) throw Exception('Cannot '.$cmd.', verification system error.'); if (mysql_num_rows($res) != 1) return false; if (!($row = mysql_fetch_assoc($res))) throw Exception('Cannot '.$cmd.', verification system error.'); if ($iau) return (int)$_SESSION['user']['data']['id'] === (int)$row['id']; unset($row['pwd']); $_SESSION['user'] = array( 'authenticated' => true, 'data' => $row, ); return true; }

function auth($loggedin) {

    query database to see if username & Password match;
    write certain variables into session (Or maybe into the cache?)

    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@raoset.com