код авторизации. Какие будут замечания?

Tranquil · 25 Май 2006

pj

Не используй вообще:

header('WWW-Authenticate: Basic realm="test"');
header('HTTP/1.0 401 Unauthorized');

Вот код входа через html-форму:

Сначала где-нибудь:

PHP:

session_start();

Затем в любом месте:

PHP:

if (empty($_SESSION['saName'])){
if (empty($_REQUEST["aName"])){
  @$error.=" логин";
}
if (empty($_REQUEST["pass"])){
  @$error.=" пароль";
}
if (@$error=="" && $_REQUEST["aName"]!=""){
   $hash = md5($_REQUEST["pass"]);
   //соединяемся с базой - берём логин и hash пароля   
   if ($rows==1){ // $rows - нашлось ли соответствие логина
      $hashbd = хэш из базы
      if ($hash==$hashbd){ // соответствие пароля
         $_SESSION['saName']= $_REQUEST["aName"];
         } else { echo "Неправильный пароль. <a href='index.php'>Повторить вход.</a>";}
   } else { echo "Неправильное имя. <a href='index.php'>Повторить вход.</a>";}
}
if ($_REQUEST["aName"]==""){
echo "<form name='FormName' action='index.php' method='post'>
<input name='aName' type='text' value=''><br />
<input name='pass' type='password' value=''><br />
<input type='submit' value='вход'>
</form>";
}
}
if (!empty($_SESSION['saName'])){
          echo "Вход выполнен - ".$_SESSION['saName'];
          }

А чтобы отлогиниться что-то типа "<a href='index.php?act=sessionunset'>выход</a>" и обработка:

PHP:

if ($_REQUEST["act"]=="sessionunset"){
     session_unset();
     header("Location: /products");

вот... у меня работает. Буду рад конструктивным замечаниям.

Фанат · 25 Май 2006

о, очнулся.
Скажи, ты действительно думаешь, что этот твой код тут кому-то нужен?
Единственный смысл публикации этого кода - это замечания. Но обсуждать твой код в чуой теме неправильно, так что едем в отдельную

Буду рад конструктивным замечаниям.

большие куски хтмл следует выводить не эхом, а закрывая и открывая тег пхп.

@$error

смотрится феерично.
когда же, наконец, ламеры всего мира поймут, что переменную надо объявлять не для того, чтобы избавиться от нотиса, а чтобы в $error не оказалось чего-то неожиданного!
переменную - объявить
собак - убрать

$error.=" логин";

информативность сообщения об ошибке просто поражает.
что логин? что пароль? неизвестно.

if (@$error=="" && $_REQUEST["aName"]!=""){

только что проверяли aName на пустоту - и опять снова проверяем! зачем?

if ($_REQUEST["aName"]==""){

третий раз проверяем. не многовато?
почему нельзя сделать одну проверку?
зачем проверять пароль на пустоту?

почему локейшена нету после успешного входа?

dr-sm · 25 Май 2006

1) не $_REQUEST["pass"], а $_POST['pass'] и иже с ним - сыкономишь три байта за вызов. данные от клиента пойдут через POST, вот и бери их оттуда.

2) не стоит бояцо об'являть переменные:
$pass = $_POST['pass']; и тп.
и функции:
function doLogin() {
if (....) {
return false;
}
.....
return true;
}
...
if (!doLogin()) {
echo "login invalid"; // почему об'яснять юзеру не стоит.
}
так код выглядит красивее и короче.

3) если в сессии есть имя пользователя это не значит что его не забанили.

Фанат · 25 Май 2006

так код выглядит красивее и короче.

ума палата. из одной строчки кода делать функцию.
откуда он станет короче?
добавляется объявление и вызов функции.
вместо одной строчки - три.

переменные тоже не стоит бояться объявлять.
если только они нужны.
то есть - не в этом случае.

я фигею.
раздувать код и писать "так короче". это что ж надо пить, чтобы так гнать?

dr-sm · 25 Май 2006

хех Фанат, your mileage may vary

.
при получении данных от клиента параноя никогда неповредит.
а функциональной декомпозиции нада учиться на простых примерах.

Фанат · 25 Май 2006

что-то я не заметил в твоих рекомендациях паранойи
а декомпозиция - дело хорошее, если она действительно улучшает читаемость, а не наоборот.
учиться надо, но не на примерах из одной строчки.

после того, как он сократит код, там декомпозировать нечего будет.
и это лучше, чем выносить в функцию гору бессмысленного кода.

dr-sm · 26 Май 2006

о так это всегда пожалуйста:
1) $user_name = get_magic_quotes_gpc() ? stripslashes($_REQUEST['aName']) : $_REQUEST['aName'];
2) вдруг там много?
$user_name = substr($user_name, 0, MAX_LOGIN_DATA_LENGTH);
3) строковые данные в сессии зло, только так:
$_SESSION['UID'] = user_id из базы.
4) обязательно проверить случай если логинимся уже залогиненые.
если да то снести сессию и создать новую, записать юзеру в базу
полученый SID.
5) документация глаголит, что session_unset() - старо.
6) sha вместо md5, и использовать salt.
7) вынести всю логику авторизации в отдельный скрипт, добавить в форму hidden поле с return_url.

пока все, больше немогу... )

zarus · 26 Май 2006

Автор оригинала: dr-sm
1) $user_name = get_magic_quotes_gpc() ? stripslashes($_REQUEST['aName']) : $_REQUEST['aName'];
2) вдруг там много?
$user_name = substr($user_name, 0, MAX_LOGIN_DATA_LENGTH);
пока все, больше немогу... )

вместо 2 - проверочку на "левые" символы через regexp [...]{min_len,max_len}

dr-sm · 26 Май 2006

zarus зачем?, мы же не нового юзера создаем (addslashes() конечно же вызывать всегда когда пихаем какую-нибуть строку в SQL, если пихаем число, то нада делать (int)). regexp к томуже значительно медленнее работают - танком по воробьям. на нормально настроеном сервере get_magic_quotes_gpc() должно возвращать - false, ибо расхолаживает отроков юных, внося в их неокрепшие умы иллюзию безопасности.

Фанат · 26 Май 2006

dr-sm
у тебя нет культуры общения на форумах.
ты разговариваешь с кем угодно, только не с собеседником.
остаётся только догадываться о пропущеных частях диалога.

мне зачем-то написал про паранойю, в таком виде, как будто я про неё спрашивал.
zarus-у пишешь про пункт первый, хотя он там остался просто потому, что форум так процитировал.

постарайся быть внимательнее.

-~{}~ 26.05.06 13:55:

а по второму - ты прав, да.

dr-sm · 26 Май 2006

Фанат, спасибо за конструктивные замечания.
как я понял, zarus писал, что вместо этих 2х пунктов, использовать regexp. я же хотел донести мысль о том, что наличие "левых" символов нам безразлично, тк мы не создаем нового пользователя.
про параною прошу прощения, был невсебе ).

Andreika · 26 Май 2006

regexp к томуже значительно медленнее работают
насколько медленнее, если не секрет? и медленнее, чем что? чем substr или даже медленнее, чем substr и поиск по базе при недопустимых символах в нике?

Фанат · 26 Май 2006

ё.
давайте здесь это не будем обсуждать, а?
и так уж замечание про регэкспы совсем не втему =)

zarus · 26 Май 2006

Автор оригинала: dr-sm
zarus зачем?, мы же не нового юзера создаем (addslashes() конечно же вызывать всегда когда пихаем какую-нибуть строку в SQL, если пихаем число, то нада делать (int)). regexp к томуже значительно медленнее работают - танком по воробьям. на нормально настроеном сервере get_magic_quotes_gpc() должно возвращать - false, ибо расхолаживает отроков юных, внося в их неокрепшие умы иллюзию безопасности.

Согласен, с регексом погорячился. С другой стороны надо указать пользователю, что он символ не тот вписал. Ну и пометочку сделать, что всякие символы нехорошие вводит.
з.ы. addslashes -> mysql_real_escape_string, если используется MySQL.

Tranquil · 28 Май 2006

Конечно же $_POST а не $_REQUEST, где это касается получения данных из формы, это я ошибся.
Про собаку и проверку приходящих от клиента данных понятно.
Фанат А зачем локейшен, если вход уже произведён?

Фанат · 28 Май 2006

чтобы не выскакивала надпись "невозможно отобразить страницу"
это нединственное, что вызвало у тебя вопросы?
с остальным всё ясно?
и как переписать весь этот код в 5 строчек - тоже?

Tranquil · 28 Май 2006

PHP:

if (empty($_SESSION['uid'])){
if ($_POST["aName"]!=""){
   $hash = sha1($_POST["pass"]);
   //соединяемся с базой - берём логин и хэш пароля
   if ($rows==1){ // $rows - нашлось ли соответствие логина
      $hashbd = $db[1]['hash']; //хэш из базы
      if ($hash==$hashbd){ // соответствие пароля
         $_SESSION['uid']= $bd[1]['id'];
         header("Location: index.php");
         }
   }
} else { echo "<form name='FormName' action='index.php' method='post'>
<input name='aName' type='text' value=''><br />
<input name='pass' type='password' value=''><br />
<input type='submit' value='вход'></form>";}
} else {echo "Вход выполнен - ".$bd[1]['login'];}

Фанат про 5 строчек я ещё подумаю, но трудно.

dr-sm чето почитал про salt, оно только в crypt используется вроде

Фанат · 28 Май 2006

$hashbd = $db[1]['hash']; //хэш из базы
if ($hash==$hashbd){ // соответствие пароля

зачем всё это?
почему сразу в запросе не проверить?

форму, как я уже говорил, проще выводить по-человечески, чистым html

Tranquil · 28 Май 2006

Фанат так форма же тогда всегда будет вылезать, если её из альтернативы PHP вытащить, а нужно либо форма, либо "Вход выполнен -"

Фанат · 28 Май 2006

а где я тебе сказал, что её из "альтернативы" вытаскивать?

код авторизации. Какие будут замечания?

Новичок

oncle terrible

Новичок

oncle terrible

Новичок

oncle terrible

Новичок

Хитрожопый макак

Новичок

oncle terrible

Новичок

"PHP for nubies" reader

oncle terrible

Хитрожопый макак

Новичок

oncle terrible

Новичок

oncle terrible

Новичок

oncle terrible