-
Notifications
You must be signed in to change notification settings - Fork 4
Add support for mutilple certificate #59
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
Add support for mutilple certificate #59
Conversation
|
|
f0bf684 to
744389f
Compare
| #define IDD_DLG_AUTH_PWC 224 | ||
| #define IDI_ICON2 225 | ||
| #define IDI_ICON3 226 | ||
| #define IDS_STRING_STARTING_BROWSER 227 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自動生成されるファイル。今まで自動整形していたが、自動整形しても大量の差分が出るし、このファイルを直接扱うことはないはずなので、今後は自動整形の対象から外したい。
26adb6c to
1192ae8
Compare
resource.h
Outdated
| #define ID_ERROR_MSG_CERT_CONTAINS_ERRORS 345 | ||
| #define ID_ERROR_MSG_CERT_NO_REVOCATION_MECHANISM 346 | ||
| #define IDS_STRING_STARTING_BROWSER 227 | ||
| #define IDD_DLG_CERTIFICATION 227 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
227番が二つある。大丈夫なのだろうか。。。
Display details of a selected certification
63462e8 to
43ab998
Compare
|
大枠はできた。 TODO:
|
| #undef APSTUDIO_READONLY_SYMBOLS | ||
|
|
||
| ///////////////////////////////////////////////////////////////////////////// | ||
| // ���{�� (�j���[�g����) resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自動生成により生じた差分。このままとしたい。
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自動整形による差分。
|
レビュー可能な状態となったので、お手隙の際にレビューをお願いします。 |
| private: | ||
| CString GetSerialNumberAsHexString(const CefRefPtr<CefX509Certificate> certificate); | ||
| CString GetTimeString(const CefTime& value); | ||
| CString GetPrincipalString(const CefRefPtr<CefX509CertPrincipal> principal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このあたり、CefRefPrtなどのCefの定義を使わないような作りのほうが良さそうにも思うが、結局CefX509CertPrincipalなどのCefの定義を渡さなければならないので、とりあえずCefRefPtrを渡すようなつくりになっている。
DlgCertification.h
Outdated
| virtual ~CDlgCertification(); | ||
|
|
||
| CComboBox certificationComboBox; | ||
| int* m_selectedIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これがポインターである理由って何かありますか?
ポインターでない方がコードが分かりやすくなるような気がします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このあたりの事情について、以下の箇所にコメントしているのですが、Web上だと文字化けしていてわからないですね...。
// DoModalなどで結果を返し終えた後、certificationComboBoxなどが解放されてしまって参照できない。
// なので、このタイミングでクラス変数に結果を代入しておく。
また、最初はこのm_selectedIndexをOnCbnSelchangeCertificationComboで更新するようにしていたのですが、DoModal後のダイアログ解放のタイミングでもOnCbnSelchangeCertificationComboが呼ばれてしまい、その時certificationComboBox.GetCurSel()が0になり、最終的にDoModal実行後には常にm_selectedIndexが0になってしまっていたので、上手く行きませんでした。
現状がイマイチという点につきましては賛同ですので、もう少し良い方法がないか探ってみます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど。
だとしてもポインターである必要はない(DoModal後もdlgそのものが解放されるわけではないと思うのので、m_selectedIndexは参照できるはずであり、直接参照すれば良く、ローカル変数を介する必要がない)ような気がしたのですが、その点ではどうでしょう?
(確かめないで言っているので、解放されてしまうようでしたらごめんなさい)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_selectedIndexも解放されてしまうと思い込んでいたのですが、試したところ解放されなかったので、直接参照するような作りに変更します!ありがとうございます!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_selectedIndexをprivateなメンバーにしました。
また、結局m_hostもm_certificatesも外部から参照されていないので、こちらもprivateなメンバーにしてコンストラクタで渡すように変更しました。
Substitute m_host and m_certificates in ctr
Which issue(s) this PR fixes:
N/A
What this PR does / why we need it:
複数のクライアント証明書が存在する時に、どの証明書を使うかを選択可能にする。
一度選択した証明書は、同じサイトに対してChronosを再起動するまで同じものが使われる。
別のタブでアクセスしても、同じ証明書が使用される。
(Chronos側で何かをしているわけではなく、CEF側の機能でそうなっている。)
How to verify the fixed issue:
The steps to verify:
で配布されているCAcertの証明書をルート認証局証明書としてインポートする。
の「Create a '*.p12' file with OpenSSL」配にある
「john.doe_example.org.p12」のリンクからファイルをダウンロードする。
インポートする。
(パスワード入力を求められるが、何も入力せずそのまま確定する。)
を開く。
旨のエラーが表示され、Webサイトの内容を閲覧できない場合には、
警告を無視してWebサイトを表示するよう操作する必要がある。
Expected result:
以下のような、どのクライアント証明書を使用するかの選択ダイアログが表示されること。
「Jhon Doe」を選択すると、認証に成功して、認証に使われた証明書の情報が表示されること。