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?

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);
if($rowcnt !="1"){
echo "Auth failed";
die("Auth failed... Sorry");



}else{
while($row1 = mysqli_fetch_array($loginResult)) {
$_SESSION['user'] = $row1['loginName'];
$_SESSION['loggedin'] = "YES";
$table = $row1['tableName'];
$adminLevel = $row1['adminLevel'];
$authenticated = "TRUE";
echo "<BR>authentication complete";
}
return Array($table, $authenticated, $adminLevel);


--

Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@raoset.com

>
> 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?
>
>
>
I don't see anything wrong with that method. One thing I would suggest is
that you make username unique in your database if you want to avoid
duplicate results. But your way of checking is just fine as it is.

--
-Dan Joseph

"Build a man a fire, and he will be warm for the rest of the day.
Light a man on fire, and will be warm for the rest of his life."

On Mar 14, 2008, at 12:45 PM, Dan Joseph wrote:

> 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?
>
>
>
> I don't see anything wrong with that method. One thing I would
> suggest is that you make username unique in your database if you
> want to avoid duplicate results. But your way of checking is just
> fine as it is.


Hey Dan,

Thanks for the reply! I couldn't find any reason why it wouldn't work,
but just wanted someone else to look at it as well... I'm not the best
programmer yet... Or at all.. But I'm getting there!

Thanks for the tip about the username being unique.... I'll make that
change as well...


--

Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@raoset.com

> $rowcnt = mysqli_num_rows($loginResult);
> if($rowcnt !="1"){
> echo "Auth failed";
> die("Auth failed... Sorry");
>
>
>
> }else{
> while($row1 = mysqli_fetch_array($loginResult)) {
> $_SESSION['user'] = $row1['loginName'];
> $_SESSION['loggedin'] = "YES";

Eww. Use booleans:

$_SESSION['loggedin'] = true;

> $table = $row1['tableName'];
> $adminLevel = $row1['adminLevel'];
> $authenticated = "TRUE";

Like above, I would advise using booleans (true/false) and not strings
(text):

$authenticated = true; // Note the lack of quote marks

--
Richard Heyes
Employ me:
http://www.phpguru.org/cv

On Fri, Mar 14, 2008 at 1:02 PM, Richard Heyes <richardh@phpguru.org> wrote:
> > $rowcnt = mysqli_num_rows($loginResult);
> > if($rowcnt !="1"){
> > echo "Auth failed";
> > die("Auth failed... Sorry");
> >
> >
> >
> > }else{
> > while($row1 = mysqli_fetch_array($loginResult)) {
> > $_SESSION['user'] = $row1['loginName'];
> > $_SESSION['loggedin'] = "YES";
>
> Eww. Use booleans:
>
> $_SESSION['loggedin'] = true;
>
>
> > $table = $row1['tableName'];
> > $adminLevel = $row1['adminLevel'];
> > $authenticated = "TRUE";
>
> Like above, I would advise using booleans (true/false) and not strings
> (text):
>
> $authenticated = true; // Note the lack of quote marks
>
> --
> Richard Heyes
> Employ me:
> http://www.phpguru.org/cv
>
>
>
> --
> PHP General Mailing List (http://www.php.net/)
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

Yes, if it is a lack of being able to see your value using print_r or
echo, then use var_dump().

Dan Joseph wrote:
>> 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?
>>
>>
>>
> I don't see anything wrong with that method. One thing I would suggest is
> that you make username unique in your database if you want to avoid
> duplicate results. But your way of checking is just fine as it is.
>
To go along with this and making sure that usernames are unique, I would
LIMIT 1 on the query. With no LIMIT, if you have 300,000 users, then
the query will not stop if it matches the first user, it searches all
300,000. With LIMIT 1 it will stop on the first match.

-Shawn

On Mar 14, 2008, at 1:05 PM, Eric Butera wrote:

> On Fri, Mar 14, 2008 at 1:02 PM, Richard Heyes
> <richardh@phpguru.org> wrote:
>>>
>>
>
> Yes, if it is a lack of being able to see your value using print_r or
> echo, then use var_dump().
>
Seeing the value's and printing them arn't a problem... Just a hold
over from an old program I am rewriting now that I know more about
what I'm doing. Hopefully stream lining the entire thing!


--

Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@raoset.com

On Mar 14, 2008, at 1:15 PM, Shawn McKenzie wrote:

> Dan Joseph wrote:
>>> 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?
>>>
>>>
>>>
>> I don't see anything wrong with that method. One thing I would
>> suggest is
>> that you make username unique in your database if you want to avoid
>> duplicate results. But your way of checking is just fine as it is.
>>
> To go along with this and making sure that usernames are unique, I
> would
> LIMIT 1 on the query. With no LIMIT, if you have 300,000 users, then
> the query will not stop if it matches the first user, it searches all
> 300,000. With LIMIT 1 it will stop on the first match.

Hi Shawn,

I do have a LIMIT 0,1 that didn't make it into the actual copy/paste.
Thanks though for the tip!


>
>
> -Shawn
>
> --
> PHP General Mailing List (http://www.php.net/)
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

--

Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@raoset.com

Jason Pruim wrote:
>
> On Mar 14, 2008, at 1:15 PM, Shawn McKenzie wrote:
>
>> Dan Joseph wrote:
>>>> 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?
>>>>
>>>>
>>>>
>>> I don't see anything wrong with that method. One thing I would
>>> suggest is
>>> that you make username unique in your database if you want to avoid
>>> duplicate results. But your way of checking is just fine as it is.
>>>
>> To go along with this and making sure that usernames are unique, I would
>> LIMIT 1 on the query. With no LIMIT, if you have 300,000 users, then
>> the query will not stop if it matches the first user, it searches all
>> 300,000. With LIMIT 1 it will stop on the first match.
>
> Hi Shawn,
>
> I do have a LIMIT 0,1 that didn't make it into the actual copy/paste.
> Thanks though for the tip!
>

If this is the case, then you will never have a situation where your result set
will be larger then 1. It will be either 1 or 0 (zero).

--
Jim Lucas

"Some men are born to greatness, some achieve greatness,
and some have greatness thrust upon them."

Twelfth Night, Act II, Scene V
by William Shakespeare

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)

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?) .. 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 );

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

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

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.

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 generally try to distinguish between userland and php functions by using lowercase
for php funcs and CamelCase naming schemes for userland functions.

--
"ok, porky pig say your line."

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

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

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, ing 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

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, ing 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
>
>
>

Why is Jason schreefing again?

Jochem Maas wrote:
> 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, ing 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
>>
>>
>>
>

On Mar 19, 2008, at 12:02 AM, Shawn McKenzie wrote:

> Why is Jason schreefing again?

Because I'm good at it?

--

Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@raoset.com

Just to warn you... I've been up for about 30 minutes and I'm still on
my first shot of caffeine... Sorry if things don't make 100% sense


On Mar 18, 2008, at 10:27 PM, Jochem Maas wrote:

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

By the time I'm ready to release this, I'll have 50 versions
>
>
>> 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.

Would it make sense to set up a function to see if they are
authenticated, and if they aren't, have it call the authentication
function?
>
>
> 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';

I've seen these kinds of things in other scripts that I've looked at,
but don't totally understand what the : does between 2 options...
>
>
> 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?)

I'm going to try this suggestion in just a few minutes... Thanks for
your . I had it all written and working without using functions,
but then I wanted to extend and all hell broke loose



--

Jason Pruim
Raoset Inc.
Technology Manager
MQC Specialist
3251 132nd ave
Holland, MI, 49424-9337
www.raoset.com
japruim@raoset.com

Jason Pruim wrote:
>> $cmd = $iau ? 'verify account' : 'login';
>
> I've seen these kinds of things in other scripts that I've looked at,
> but don't totally understand what the : does between 2 options...

$cmd = $iau ? 'verify account' : 'login';
if (^^^^) { $cmd = ^^^^^^^^^^^^^^^^; } else { $cmd = ^^^^^^^; }

Simple as that.

-Stut

--
http://stut.net/