Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

管理画面ログイン履歴とIPアドレスによるアクセス拒否を実装 #4978

Merged
merged 8 commits into from
Mar 30, 2021

Conversation

dk-umebius
Copy link
Contributor

@dk-umebius dk-umebius commented Mar 21, 2021

概要(Overview・Refs Issue)

#4847

スクリーンショット 2021-03-21 14 07 22

方針(Policy)

  • 管理画面ログイン画面で入力したログインID、ログイン成否、IPアドレス、該当するMember、試行日時をテーブルに記録
  • 上で保存した記録を一覧画面で表示、検索できる
  • 管理画面へのアクセスを拒否するIPリストを指定できる (セキュリティ管理で登録)

実装に関する補足(Appendix)

4.0系プラグインから若干設計を変更しています。

  • メニューの位置を「設定>システム設定」の中に設定
  • エンティティ等のクラス名をLoginRecordからLoginHistoryに変更
  • ログイン成否のステータスをマスタテーブル化

問題があればご指摘願います。

テスト(Test)

日本語と英語の設定でインストール、ログイン、検索確認

相談(Discussion)

検索画面のinitializeと言ったフックポイントを追加していません。新画面でフックポイントは追加した方がよいのでしょうか?

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更
  • フックポイントの呼び出しタイミングの変更
  • フックポイントのパラメータの削除・データ型の変更
  • twigファイルに渡しているパラメータの削除・データ型の変更
  • Serviceクラスの公開関数の、引数の削除・データ型の変更
  • 入出力ファイル(CSVなど)のフォーマット変更

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

@codecov-io
Copy link

codecov-io commented Mar 21, 2021

Codecov Report

Attention: Patch coverage is 91.51786% with 19 lines in your changes missing coverage. Please review.

Project coverage is 76.36%. Comparing base (96b1b21) to head (e0817fc).

Files with missing lines Patch % Lines
...er/Admin/Setting/System/LoginHistoryController.php 88.88% 5 Missing ⚠️
...roller/Admin/Setting/System/SecurityController.php 0.00% 5 Missing ⚠️
src/Eccube/Entity/LoginHistory.php 87.87% 4 Missing ⚠️
src/Eccube/EventListener/IpAddrListener.php 71.42% 2 Missing ⚠️
src/Eccube/EventListener/LoginHistoryListener.php 95.23% 2 Missing ⚠️
src/Eccube/Entity/Master/LoginHistoryStatus.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##             4.1-feature    #4978      +/-   ##
=================================================
+ Coverage          76.18%   76.36%   +0.17%     
- Complexity          6124     6190      +66     
=================================================
  Files                437      445       +8     
  Lines              20736    20955     +219     
=================================================
+ Hits               15798    16002     +204     
- Misses              4938     4953      +15     
Flag Coverage Δ
tests 76.36% <91.51%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@okazy okazy added the enhancement 機能追加 label Mar 22, 2021
@okazy okazy added this to the 4.1 milestone Mar 22, 2021
@okazy
Copy link
Contributor

okazy commented Mar 22, 2021

プルリクありがとうございます!

ログイン履歴の動作確認して問題ありませんでした。

image

アクセス拒否リストも動作確認して問題ありませんでした。

image

.env の設定が更新されます。

image

アクセス拒否の場合は 403 のステータスコードが返ります。

image

@okazy okazy added the affected:admin_template 管理画面テンプレートのDOMに影響のある変更 label Mar 22, 2021
@okazy
Copy link
Contributor

okazy commented Mar 22, 2021

環境変数 ECCUBE_ADMIN_DENY_HOSTS はなしでも動くため、アップデート手順での追記は不要。

Copy link
Contributor

@okazy okazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

途中まで確認して問題ありませんでした。
Symfony4 対応に向けていくつかコメントしました。

確認残は

  • テンプレート
  • テスト
  • マイグレーション
  • FormAppend
  • クエリビルダーのカスタマイズ

@okazy
Copy link
Contributor

okazy commented Mar 23, 2021

postgres10 にてバージョンアップテスト

bin/console doctrine:schema:update --force --dump-sql

 The following SQL statements will be executed:

     CREATE TABLE dtb_login_history (id SERIAL NOT NULL, login_history_status_id SMALLINT NOT NULL, member_id INT DEFAULT NULL, user_name TEXT DEFAULT NULL, client_ip TEXT DEFAULT NULL, create_date TIMESTAMP(0) WITH TIME ZONE NOT NULL, update_date TIMESTAMP(0) WITH TIME ZONE NOT NULL, discriminator_type VARCHAR(255) NOT NULL, PRIMARY KEY(id));
     CREATE INDEX IDX_6191DD4F9FA62FDD ON dtb_login_history (login_history_status_id);
     CREATE INDEX IDX_6191DD4F7597D3FE ON dtb_login_history (member_id);
     COMMENT ON COLUMN dtb_login_history.create_date IS '(DC2Type:datetimetz)';
     COMMENT ON COLUMN dtb_login_history.update_date IS '(DC2Type:datetimetz)';
     CREATE TABLE mtb_login_history_status (id SMALLINT NOT NULL, name VARCHAR(255) NOT NULL, sort_no SMALLINT NOT NULL, discriminator_type VARCHAR(255) NOT NULL, PRIMARY KEY(id));
     ALTER TABLE dtb_login_history ADD CONSTRAINT FK_6191DD4F9FA62FDD FOREIGN KEY (login_history_status_id) REFERENCES mtb_login_history_status (id) NOT DEFERRABLE INITIALLY IMMEDIATE;
     ALTER TABLE dtb_login_history ADD CONSTRAINT FK_6191DD4F7597D3FE FOREIGN KEY (member_id) REFERENCES dtb_member (id) ON DELETE SET NULL NOT DEFERRABLE INITIALLY IMMEDIATE;

 Updating database schema...

     8 queries were executed


 [OK] Database schema updated successfully!
bin/console doctrine:migrations:migrate

                    Application Migrations


WARNING! You are about to execute a database migration that could result in schema changes and data loss. Are you sure you wish to continue? (y/n)y
Migrating up to 20210319122142 from 0

(中略)

  ++ migrating 20210319122142

     -> INSERT INTO mtb_login_history_status (id, name, sort_no, discriminator_type) VALUES (?, ?, ?, 'loginhistorystatus') with parameters ([0], [Failure], [0])
     -> INSERT INTO mtb_login_history_status (id, name, sort_no, discriminator_type) VALUES (?, ?, ?, 'loginhistorystatus') with parameters ([1], [Success], [1])

  ++ migrated (0.01s)

  ------------------------

  ++ finished in 1.6s
  ++ 7 migrations executed
  ++ 4 sql queries

image

</div>
</div>

{# エンティティ拡張の自動出力 #}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dk-umebius
こちらって SearchLoginHistoryType->configureOptions()data_class が定義されていないので実質出力できないですよね?
他の検索画面のテンプレートにも「エンティティ拡張の自動出力」入っていますが、出力方法がわかりません。。。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FormTypeのeccube_form_optionsオプションで追加するものですよね。
https://github.com/EC-CUBE/mail-magazine-plugin/blob/6ff710f8e6f342a11b1121889c8656af796b159b/Form/Extension/EntryMailMagazineTypeExtension.php#L75-L78

確かに「エンティティ拡張」ではないですよね..。いつからかこの表現のまま残っていたんでしょうか。全く気付きませんでした。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

勉強になりました。
FormAppend 用とばかり思っていました。
ずっと勘違いしていましたので、このコメントは過去の私がやらかした気もします。。。

@okazy
Copy link
Contributor

okazy commented Mar 23, 2021

QueryBuilder の拡張も問題なく動いていました。

<?php

namespace Customize\Repository;

use Eccube\Doctrine\Query\OrderByClause;
use Eccube\Doctrine\Query\OrderByCustomizer;
use Eccube\Repository\QueryKey;

class AdminLoginHistoryListCustomizer extends OrderByCustomizer
{
    /**
     * 常にユーザ名でソートする
     */
    protected function createStatements($params, $queryKey)
    {
        return [new OrderByClause('lh.user_name')];
    }

    public function getQueryKey()
    {
        return QueryKey::LOGIN_HISTORY_SEARCH_ADMIN;
    }
}

image

@okazy
Copy link
Contributor

okazy commented Mar 23, 2021

@dk-umebius
プルリクありがとうございます!
テストもしっかり追加いただいていて大変助かります🙇‍♂️

マイグレーションの処理で気になったところがあったのでプルリクしました。
U-Mebius#15

ご確認と取り込みをお願いできますでしょうか。

管理画面ログイン履歴機能の微調整をしました
Copy link
Contributor Author

@dk-umebius dk-umebius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

修正ありがとうございます。マージさせていただきました。

</div>
</div>

{# エンティティ拡張の自動出力 #}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FormTypeのeccube_form_optionsオプションで追加するものですよね。
https://github.com/EC-CUBE/mail-magazine-plugin/blob/6ff710f8e6f342a11b1121889c8656af796b159b/Form/Extension/EntryMailMagazineTypeExtension.php#L75-L78

確かに「エンティティ拡張」ではないですよね..。いつからかこの表現のまま残っていたんでしょうか。全く気付きませんでした。

@okazy
Copy link
Contributor

okazy commented Mar 23, 2021

マージありがとうございます!
新規でページを追加いただいていますので、簡単なE2Eテストも追加しておきたいと思います。
いくつかテストが落ちていますが、おそらく別の要因です。(調査中)

@okazy
Copy link
Contributor

okazy commented Mar 25, 2021

@dk-umebius
E2Eテストを追加しましたのでご確認とマージをよろしくお願いします 🙏
U-Mebius#16

IP制限(拒否リスト)とログイン履歴とE2Eテストを追加
@dk-umebius
Copy link
Contributor Author

@okazy
E2Eテストのご追加ありがとうございます。
マージさせていただきました。

Copy link
Contributor

@okazy okazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます!
動作確認、ソースコード確認して問題ありませんでした。

@okazy
Copy link
Contributor

okazy commented Mar 26, 2021

ログイン履歴の結合試験項目書を追加しました。
EC-CUBE/eccube-specification#67

@dk-umebius
すみません、非常にしょうもないミスをしておりました。
U-Mebius#17
ご確認とマージをお願いします 🙏

@okazy okazy merged commit 3f08e3f into EC-CUBE:4.1-feature Mar 30, 2021
@okazy
Copy link
Contributor

okazy commented Mar 30, 2021

@dk-umebius ありがとうございます!マージしました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected:admin_template 管理画面テンプレートのDOMに影響のある変更 enhancement 機能追加 Status: ready-for-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants