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

インストール完了画面で有効化するプラグインを選択する #5074

Merged
merged 10 commits into from
Jul 20, 2021

Conversation

nanasess
Copy link
Contributor

@nanasess nanasess commented Jun 23, 2021

概要(Overview・Refs Issue)

インストール完了画面で有効化するプラグインを選択する
有効化するプラグインは、予めインストール状態としておく必要がある

TODO

  • インストール完了から10分以上経過した場合のエラーハンドリング
  • プラグイン有効化中にモーダルを表示して操作を受け付けないようにする
  • Ajax のセキュリティ対策
  • CSRF対策
  • プレインストールプラグインの選定
  • SQLite3 の場合の動作

方針(Policy)

  • プレインストールプラグインを Ajax で有効化する
  • dtb_plugin に登録されているプラグインが対象

実装に関する補足(Appendix)

  • APP_ENV=installAPP_ENV=prod では、セッションが共有できないため、 CSRF トークンは var 以下のファイルに保存している

テスト(Test)

4.1-beta2 に本PRを適用し、 API プラグインが有効化されるのを確認

相談(Discussion)

インストーラの bootstrap が古いので、ボタンの見た目がよくない。管理画面と同様に bootstrap5 を使いたい

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

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

レビュワー確認項目

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

@nanasess nanasess requested review from okazy and matsuoshi June 23, 2021 09:24
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #5074 (b4e8618) into 4.1 (8c3ae69) will decrease coverage by 0.30%.
The diff coverage is 0.00%.

❗ Current head b4e8618 differs from pull request most recent head 0ab140d. Consider uploading reports for the commit 0ab140d to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##                4.1    #5074      +/-   ##
============================================
- Coverage     75.96%   75.65%   -0.31%     
- Complexity     5946     6032      +86     
============================================
  Files           449      456       +7     
  Lines         20922    21165     +243     
============================================
+ Hits          15893    16012     +119     
- Misses         5029     5153     +124     
Flag Coverage Δ
tests 75.65% <0.00%> (-0.31%) ⬇️

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

Impacted Files Coverage Δ
...rc/Eccube/Controller/Install/InstallController.php 0.00% <0.00%> (ø)
src/Eccube/Controller/InstallPluginController.php 0.00% <0.00%> (ø)
src/Eccube/Exception/PluginApiException.php 0.00% <0.00%> (-92.31%) ⬇️
src/Eccube/Entity/Member.php 84.94% <0.00%> (-7.83%) ⬇️
src/Eccube/Controller/Admin/AdminController.php 78.18% <0.00%> (-0.46%) ⬇️
...Eccube/Form/EventListener/HTMLPurifierListener.php 100.00% <0.00%> (ø)
...be/Twig/Extension/SafeTextmailEscaperExtension.php 100.00% <0.00%> (ø)
src/Eccube/Form/Type/Admin/TwoFactorAuthType.php 0.00% <0.00%> (ø)
src/Eccube/Service/TwoFactorAuthService.php 31.48% <0.00%> (ø)
...r/Admin/Setting/System/TwoFactorAuthController.php 0.00% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c3ae69...0ab140d. Read the comment docs.

@chihiro-adachi chihiro-adachi deleted the branch EC-CUBE:4.1 June 25, 2021 01:43
@okazy okazy reopened this Jun 25, 2021
@okazy okazy changed the base branch from 4.1-beta2 to 4.1-core June 25, 2021 01:47
@okazy
Copy link
Contributor

okazy commented Jun 25, 2021

4.1-beta2 ブランチが削除されたため閉じられてしまいました。
プルリク先のブランチを 4.1-core に切り替えています。

@okazy okazy added this to the 4.1 milestone Jun 25, 2021
@okazy okazy added the enhancement 機能追加 label Jun 25, 2021
@nanasess nanasess changed the base branch from 4.1-core to 4.1 July 6, 2021 04:19
nanasess added 2 commits July 6, 2021 14:41
- 有効化/無効化中に操作できないよう改善
- CSRF対策
- SQLite3 の場合は API プラグインを有効化できないよう修正
- エラーハンドリング修正
- 管理画面へ遷移する際にトランザクションファイルを削除するよう修正
@nanasess
Copy link
Contributor Author

nanasess commented Jul 9, 2021

install/complete の時点では、 EntityManager::getRepository() が使用できない場合がある模様。
プラグイン一覧を Ajax で表示する実装へ変更する

- install/complete の段階では EntityManager::getRepository() が使用で
きないため
- 有効化エンドポイントは APP_ENV=install と別のセッションとなるため
CSRF トークンが使用できない.他の実装を検討する必要がある
@nanasess
Copy link
Contributor Author

nanasess commented Jul 9, 2021

有効化エンドポイントは APP_ENV=install と別のセッションとなるためCSRF トークンが使用できない。他の実装を検討する必要がある

@nanasess nanasess marked this pull request as ready for review July 12, 2021 02:52
@nanasess
Copy link
Contributor Author

ファイルで CSRF トークンをチェックするように変更して、 WIP はずしました

@nanasess nanasess changed the title [WIP]インストール完了画面で有効化するプラグインを選択する インストール完了画面で有効化するプラグインを選択する Jul 12, 2021
@okazy
Copy link
Contributor

okazy commented Jul 19, 2021

動作確認項目のメモ

  • sqlite/mysql/postgres
  • databaseUrl
  • コマンド

テスト用

id,name,code,enabled,version,source,initialized,create_date,update_date,discriminator_type
1,Web API,Api,f,2.0.0,1891,f,2021-04-12 08:59:45+00,2021-04-12 08:59:45+00,plugin
2,Composer2Test,Composer2Test,f,1.0.0,1912,f,2021-04-12 08:59:45+00,2021-04-12 08:59:45+00,plugin

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.

mysql/sqlite にて動作を確認しました。

image

image

image

image

image


return $this->json(['success' => true]);
return $this->redirectToRoute('admin_homepage');
Copy link
Contributor

Choose a reason for hiding this comment

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

無駄なリダイレクトが発生するのでログイン画面の方が良いと思います。

Suggested change
return $this->redirectToRoute('admin_homepage');
return $this->redirectToRoute('admin_login');

@@ -499,12 +501,15 @@ public function complete(Request $request)
$this->removeSessionData($this->session);

// 有効化URLのトランザクションチェックファイルを生成する
file_put_contents($this->getParameter('kernel.project_dir').self::TRANSACTION_CHECK_FILE, time() + (60 * 10));
$token = StringUtil::random(32);
file_put_contents($this->getParameter('kernel.project_dir').self::TRANSACTION_CHECK_FILE, time() + (60 * 10).':'.$token.':'.$databaseUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

なぜ databaseUrl を保存しているんでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不要な項目でしたので削除しました

$.get({ url: '{{ url('install_plugins') }}' },
function (data) {
$('#plugins').children().remove();
data.forEach(function (plugin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

plugin が0個の場合はプラグイン有効化のcolumnごと非表示で良いと思います。(git clone でインストール時など)

@okazy
Copy link
Contributor

okazy commented Jul 20, 2021

@nanasess
ご対応ありがとうございます。
気になったところをプルリクしましたのでご確認と取り込みをお願いしたいです。
nanasess#44

.httransaction に DATABASE_URL を保存しているのって何故なんでしょうか?

@nanasess
Copy link
Contributor Author

@okazy ありがとうございます。 databaseUrl はデバッグ時の削除漏れでした😥

初期インストールプラグインがない場合にプラグイン有効化の文言を非表示 / インストール完了後のリダイレクト先をログイン画面に修正
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 okazy merged commit 3c1e5f8 into EC-CUBE:4.1 Jul 20, 2021
@okazy
Copy link
Contributor

okazy commented Jul 20, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants