ログイン日時をセッションに格納したい

前提・実現したいこと

最終ログイン日時を取得したい。

発生している問題・エラーメッセージ

私の現在のコードだと日時アップデートqueryを実行する前に
if ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
ここのfetchの日時を取得しています。

query実行後に、
ログイン日時をセッションに格納したいのですが、どのように修正すれば宜しいでしょうか？

該当のソースコード

php
1<?php
2session_start();  
3
4if (isset($_SESSION["NAME"])) {
5header("Location: my.php");
6exit;
7}
8
9$db['host'] = "hoge";  
10$db['user'] = "hoge";  
11$db['pass'] = "hoge";
12$db['dbname'] = "hoge"; 
13$date = date('Y-m-d H:i:s');
14
15$errorMessage = "";
16$errorMessage_2 = "";
17
18require_once 'hasu_test.php';
19
20if (isset($_POST["login"])) {
21if (empty($_POST["mail"])) { 
22$errorMessage = '※空やでやで';
23} else if (empty($_POST["password"])) {
24$errorMessage_2 = '※空やで。';
25} else if (!preg_match('/^[!-~]+@[!-~]+$/',  $_POST["mail"])){   
26$errorMessage = '※正規表現外mail';
27} else if (!preg_match("/\A(?=.*?[a-z])(?=.*?\d)[a-z\d]{8,100}+\z/i", $_POST["password"])){ 
28$errorMessage_2 = '※正規表現外pass';
29} else if (!preg_match("/^.{4,32}$/",$_POST["mail"])){    
30$errorMessage = '※桁数上mail';
31} else if (!preg_match("/^.{4,32}$/",$_POST["password"])){
32$errorMessage_2 = '※桁数上pass';
33
34}else{
35
36if (!empty($_POST["mail"]) && !empty($_POST["password"])) {
37$mail = $_POST["mail"];
38
39$dsn = sprintf('mysql:host=%s; dbname=%s; charset=utf8;unix_socket=/tmp/mysql.sock', $db['host'], $db['dbname']);
40
41try {
42$pdo = new PDO($dsn, $db['user'], $db['pass'], array(PDO::ATTR_ERRMODE=>PDO::ERRMODE_EXCEPTION));
43$stmt = $pdo->prepare('SELECT * FROM member WHERE mail = ?');
44$stmt->execute(array($mail));
45$password = $_POST["password"];
46
47if ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
48if (password_verify($password, $row['password'])) {
49session_regenerate_id(true);
50
51$id = $row['id'];
52$sql = "SELECT * FROM member WHERE id = $id";  //入力したIDからユーザー名を取得
53$stmt = $pdo->query($sql);
54foreach ($stmt as $row) {
55$row['mail'];  
56}
57
58$stmt = $pdo->prepare('SELECT * FROM member WHERE sdate = ?');
59$stmt->execute(array($date));
60$sql = "UPDATE member SET sdate='$date' WHERE id='$id'";
61$result = $pdo->query($sql);
62
63
64
65$_SESSION["sdate"] = $row['sdate'];
66$_SESSION["mail"] = $row['mail'];
67header("location: my.php");
68exit();  
69
70
71} else {
72$errorMessage = '※mailまたはPasswordが間違っています。';
73}
74} else {
75$errorMessage = '※該当データなしmailまたはPasswordが間違っています。';
76}
77} catch (PDOException $e) {
78$errorMessage =   header("Location: 500.php");         
79}
80}
81}
82}
83
84?>

m.ts10806

2019/03/28 06:32

インデントがめちゃくちゃですのでお手元のコードのインデントを調整して、提示しなおしてください。 ※できれば手で直すのではなくフォーマット機能のあるエディタか、整形サービスを利用してください。

m.ts10806

2019/03/28 06:36

というか全角スペースだらけですね。このコードそもそもエラーで動かないですよ。

m.ts10806

2019/03/28 06:40 編集

ifも閉じてないし、try書いてるのにcatchがないし、かなり滅茶苦茶です。コードはきちんと書いて、元のコードにエラーがないのでしたら提示するのもエラーのないコードにしてください。でないとまともに再現確認できません。前後にコードあるんだろうけど、未定義の変数をいきなり入れていたり、いきなりこの質問から見た人には何も優しくないです。

m.ts10806

2019/03/28 06:45

どんなに簡単な問題でもコードが滅茶苦茶だと回答者の読む気力を奪ってしまい、回答が得られなくなります。（要件以外のことに気が散ります）

Roo

2019/03/28 06:50

失礼いたしました。修正いたします

m.ts10806

2019/03/28 07:15

全角スペースだいぶ残ってますけども。

行動規範の内容に同意します

回答2件

ベストアンサー

コードの細かい指摘についてはyambejpさんの指摘通りです。
プリペアドステートメント使ってたり使っていなかったりコーディングスタイルに一貫性がないのもまずいと思います。

今回はすごく簡単です。
「ログイン日時」は自動で更新がかかるCURRENT_TIMESTAMPではないんですよね？
sdateが「ログイン日時」としたらシステム側で払い出した日時でupdateをかけているように見えます。

それでしたら「システム側で払い出した日時」をそのままセッションに入れれば良いのではないでしょうか。（今回の答え）

既に指摘があるように無駄なコードが多いです。
少なくとも下記2点のSQLは不要
SELECT * FROM member WHERE id = $id
SELECT * FROM member WHERE sdate = ?

最初にSELECT * FROM member WHERE mail = ?を実行していることから「mailはプライマリーキーで一意」というのが推察されます。

ということはこの最初のmailによるSELECTでユーザーデータは確保できているわけです。
そこで「パスワードも照合してログイン認証できた」のであれば、updateもいっそのことmailをWHERE条件にしても差し支えないのでは？

同じメールアドレスで別ユーザをとれるような（悪さし放題の）仕組みであれば仕方ないかもしれませんが、それならパスワードも最初の検索条件に入れる必要がありますね。

無駄なコードを書かないためにも流れをきちんと整理しましょう。
YES/NOだけで良いのでフローを書いてみると良いです。（手書きでも良い）

ちなみにコードフォーマットはEclipseなどのフォーマット機能がついたエディタを使用してください。手でやるなら最初から組みなおすくらいじゃないと無理です。

下記、Eclipseのフォーマット機能を使った例（全角スペースは手動で取り除きました。）

php
1<?php
2session_start();
3
4if (isset($_SESSION["NAME"])) {
5    header("Location: my.php");
6    exit();
7}
8
9$db['host'] = "hoge";
10$db['user'] = "hoge";
11$db['pass'] = "hoge";
12$db['dbname'] = "hoge";
13$date = date('Y-m-d H:i:s');
14
15$errorMessage = "";
16$errorMessage_2 = "";
17
18require_once 'hasu_test.php';
19
20if (isset($_POST["login"])) {
21    if (empty($_POST["mail"])) {
22        $errorMessage = '※空やでやで';
23    } else if (empty($_POST["password"])) {
24        $errorMessage_2 = '※空やで。';
25    } else if (! preg_match('/^[!-~]+@[!-~]+$/', $_POST["mail"])) {
26        $errorMessage = '※正規表現外mail';
27    } else if (! preg_match("/\A(?=.*?[a-z])(?=.*?\d)[a-z\d]{8,100}+\z/i", $_POST["password"])) {
28        $errorMessage_2 = '※正規表現外pass';
29    } else if (! preg_match("/^.{4,32}$/", $_POST["mail"])) {
30        $errorMessage = '※桁数上mail';
31    } else if (! preg_match("/^.{4,32}$/", $_POST["password"])) {
32        $errorMessage_2 = '※桁数上pass';
33    } else {
34
35        if (! empty($_POST["mail"]) && ! empty($_POST["password"])) {
36            $mail = $_POST["mail"];
37
38            $dsn = sprintf('mysql:host=%s; dbname=%s; charset=utf8;unix_socket=/tmp/mysql.sock', $db['host'], $db['dbname']);
39
40            try {
41                $pdo = new PDO($dsn, $db['user'], $db['pass'], array(
42                    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
43                ));
44                $stmt = $pdo->prepare('SELECT * FROM member WHERE mail = ?');
45                $stmt->execute(array(
46                    $mail
47                ));
48                $password = $_POST["password"];
49
50                if ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
51                    if (password_verify($password, $row['password'])) {
52                        session_regenerate_id(true);
53
54                        $id = $row['id'];
55                        $sql = "SELECT * FROM member WHERE id = $id"; // 入力したIDからユーザー名を取得
56                        $stmt = $pdo->query($sql);
57                        foreach ($stmt as $row) {
58                            $row['mail'];
59                        }
60
61                        $stmt = $pdo->prepare('SELECT * FROM member WHERE sdate = ?');
62                        $stmt->execute(array(
63                            $date
64                        ));
65                        $sql = "UPDATE member SET sdate='$date' WHERE id='$id'";
66                        $result = $pdo->query($sql);
67
68                        $_SESSION["sdate"] = $row['sdate'];
69                        $_SESSION["mail"] = $row['mail'];
70                        header("location: my.php");
71                        exit();
72                    } else {
73                        $errorMessage = '※mailまたはPasswordが間違っています。';
74                    }
75                } else {
76                    $errorMessage = '※該当データなしmailまたはPasswordが間違っています。';
77                }
78            } catch (PDOException $e) {
79                $errorMessage = header("Location: 500.php");
80            }
81        }
82    }
83}
84
85?>

全体的な懸念事項(既存指摘以外メイン)：

１．今回のコード上、どこにもNAMEって保存されてないのに下記がある。

php
1if (isset($_SESSION["NAME"])) {
2    header("Location: my.php");
3    exit();
4}

フロー上はログイン日時に入れた現在日時をセッションに格納のところで入れるものだと思う。mailは入れてるからそれでいいのでは？
またはtrue/falseだけ保管しておくキーを別途設けるとか。

２．header()は送信するだけで返り値は持たない、のにこれは何？

php
1$errorMessage = header("Location: 500.php");

３．$errorMessageとか$errorMessage_2とかつけてるけど、変数名で何のエラーかが不明。
それなら下記みたいに配列で持っておいて、適宜取り出したほうが良い。
マジックナンバーは使ってはいけません。

php
1$errorMessage['mail'][] ='必須です';
2$errorMessage['password][] ='必須です';

４．というかこれはログイン画面ですよね。
パスワード何桁とかわざわざチェックするのは悪意あるユーザーのヒントになります。
どっちも必須チェックくらいにとどめるべき。

５．ifのスコープが長すぎる
フォーマットかけたらよく分かると思います。特に短い方が後に来るコードは読みづらいです。
可読性が低く、それはメンテナンス性の低下も招きます。
elseを使うにしても以下のどちらが読みやすいか考えてみてください。
1:

php
1if($test){
2  /*
3
4   長い処理
5
6
7
8
9  */
10}else{
11  echo 'error!';
12}

php
1if(!$test){
2  echo 'error!';
3}else{
4  /*
5
6   長い処理
7
8
9
10
11  */
12}

３:

php
1if(!$test){
2  die('error!'); //関数ならここでreturn
3}
4/*
5
6   長い処理
7
8
9
10
11*/

正常な場合のみにフォーカスを当てがちでですが、異常な方がプログラムとしては大事です。
先に異常なほうを書きましょう。正常なほうは後回しでも問題ないです。

入力チェックの処理を外だしして関数呼び出してメインの処理はエラーメッセージの有無だけにしたほうがスッキリします。
「本筋の処理と直接関係がないのなら外出し」「同じようなコードは変数で分岐かけて共通化」が基本です。

６：PDOExceptionほぼ握りつぶしてる

php
1            } catch (PDOException $e) {
2                $errorMessage = header("Location: 500.php");
3            }

何のために例外捕捉するようにしたのか。これでは何か異常を検知したときに原因を調べられない。
セキュリティ観点から画面に出力する必要はないので、せめてログに出力してください。オブジェクトなので$eをjson_encode()かけたものでもいいから。

例外処理について](https://donow.jp/skillup/?p=291)

※Javaの記事ばかりですが考え方は同じ。握りつぶしちゃダメです。何のための例外をキャッチさせてるのか考えましょう。

投稿2019/03/28 07:32

編集2019/03/28 08:04

m.ts10806

総合スコア80850

Roo

2019/03/28 07:59

懇切丁寧なご回答心より感謝いたします。一旦詳細読ませていただきます。その後、1～６解答させていただきます。

Roo

2019/03/28 08:23

``sdate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' ON UPDATE CURRENT_TIMESTAMP,` 「ログイン日時」はCURRENT_TIMESTAMPです。テーブルには正しくログイン日時が更新されています。しかし、表示されている時間はアップデートを実行する前の時間となっている現状です。（例） 1回目ログイン：15:00:00 実際に表示される時間：00:00:00 2回目ログイン(１回目より5分後)：15:05:00 実際に表示される時間：15:00:00 3回目ログイン(２回目より5分後):15:10:00 実際に表示される時間：15:05:00 プライマリーキーは別途idを設けております。（既存指摘以外）： 1.～5. ご指摘通り参考にさせていただきたいと思います。 6, 確かにその通りなのですが、500に強制的に飛ばさなければならない状況にあります。目をつぶっていただけると幸いです

m.ts10806

2019/03/28 08:27

>「ログイン日時」はCURRENT_TIMESTAMPです。 →CURRENT_TIMESTAMPであっても同じ。プログラムとDBは同じサーバーであればdate()でとってくれば同じ値だと思います。どうしても心配ならupdateしたあとにmailでSELECTかければ良いです。 >プライマリーキーは別途idを設けております。 →回答に書いたようにmailで認証で出来るなら特に無理してid使う必要はないですが、updateとそのあとのselectのときに使ったらいいんじゃないでしょうか。 >6, 確かにその通りなのですが、500に強制的に飛ばさなければならない状況にあります。目をつぶっていただけると幸いです → (回答再掲)画面に出力する必要はないので、せめてログに出力してください。

Roo

2019/03/28 08:39

＞mailでselectかければ良いです。なるほど、、。試してみます。ありがとうございます。

Roo

2019/03/28 08:40

こちらの疑問終了次第ログの出力試します。

m.ts10806

2019/03/28 08:50

いやおかしい。ログイン日時がCURRENT_TIMESTAMPっておかしい。ログイン以外のときに更新されちゃダメじゃないですか？ユーザー情報をユーザーが更新したら更新されるわけですよね。「最終ログイン日時」ならCURRENT_TIMESTAMPじゃなくてnull okのtimestamp型にしてプログラム側から更新するようにすべきです。なんの用途に使うのか知らないですが… 「自動ログアウトのための無操作時間の計測のため」ならそれこそセッションに「last_action_time」とか用意しといてそこだけ上書きしていけば良いですし。ユーザーが更新を意識しない操作のたびにテーブルにupdateかけては重くなる一方かと。

Roo

2019/03/28 09:05

ご指摘感謝いたします。そうですね、現状ユーザーからのアクションは、ログインのみですが、今後I/Passの変更等あった際に、きっと同じところでつまずていました。ありがとうございます。＞CURRENT_TIMESTAMPであっても同じ。プログラムとDBは同じサーバーであればdate()でとってくれば同じ値だと思います。＞どうしても心配ならupdateしたあとにmailでSELECTかければ良いですここのコードがわかりません。もしよろしければご助言いただけませんか？

m.ts10806

2019/03/28 09:47

フロー書いたら良いかと。というか、私が提示したフローが一部変わるだけです。プログラムから払いだすか、DBから改めてとってくるか。

行動規範の内容に同意します

$password = $_POST["password"];

で受けた生パスワードを
SQLから取り出した$row['password']と比較している

if (password_verify($password, $row['password'])) {

ということはDBに生パスワードを保持しているということですか？
まずはそれを直すところから始めてください

$sql = "SELECT * FROM member WHERE id = $id";

についても一応DBから取り出した$idを利用しているので
省略しても問題ないですが、値を渡すときはprepareでやる
くせをつけたほうがよいです。

  foreach ($stmt as $row) {

    $row['mail'];  // メール
  }

意味不明

  $stmt = $pdo->prepare('SELECT * FROM member WHERE sdate = ?');

  $stmt->execute(array($date));

検索後に参照しないで更新処理をするのも意味がわからない。
ここもprepareで処理してみてください

  $_SESSION["sdate"] = $row['sdate'];

  $_SESSION["mail"] = $row['mail'];

結局この$rowは最初の「SELECT * FROM member WHERE mail = ?」の
値ですよね？間の処理がすべて無駄？

そもそもがsession_start()してませんよね？

投稿2019/03/28 06:48

yambejp

総合スコア114829

Roo

2019/03/28 07:25

ご指摘感謝します・ハッシュ化しています・prepareくせ努力します・session_startしています・等々コード修正していますので再度ご指摘いただけると幸いです。

行動規範の内容に同意します

あなたの回答

tips

プレビュー

行動規範の内容に同意します

質問の解決につながる回答をしましょう。サンプルコードなど、より具体的な説明があると質問者の理解の助けになります。また、読む側のことを考えた、分かりやすい文章を心がけましょう。

15分調べてもわからないことは
teratailで質問しよう！

ただいまの回答率
85.48%

質問をまとめることで
思考を整理して素早く解決

テンプレート機能で
簡単に質問をまとめる

質問する

質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう！