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.

Sign up for GitHub

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

Jump to bottom

session: api to allow handling certificate verification #3344

Merged
merged 3 commits into from Nov 17, 2015
Merged

session: api to allow handling certificate verification #3344

merged 3 commits into from Nov 17, 2015

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 force-pushed the certificate_verifier_api_patch branch 4 times, most recently from 9c11e95 to 6bb8736 Compare November 5, 2015 19:37
@zcbenz
Copy link
Member

zcbenz commented Nov 9, 2015

Can you make it an verify-certificate event of app module, like the select-certificate and login events?

@deepak1556
Copy link
Member Author

certverifier is at a lower level than url_request and doesnt have a knowledge about anything above it. So emitting an event on the app cant associate to any webcontents and hence have tied the api to browserwindow which is tied to browsercontext, This also helps to provide verification handler optionally for request context in different partitions. Does it still need be emitted as an event ?

@zcbenz
Copy link
Member

zcbenz commented Nov 9, 2015

I didn't notice it is per-BrowserCntext, we can make it an event of session then.

Making this API a method of BrowserWindow doesn't tie it to the window, the certificate verification requests happened in other windows of the same session will all go through the handler.

@deepak1556 deepak1556 force-pushed the certificate_verifier_api_patch branch 2 times, most recently from 40acae6 to a9be697 Compare November 9, 2015 10:43
@deepak1556
Copy link
Member Author

yeah should have been an api of session. Thanks! have updated.

@deepak1556 deepak1556 changed the title browser: api to allow handling certificate verification session: api to allow handling certificate verification Nov 9, 2015
@zcbenz
Copy link
Member

zcbenz commented Nov 9, 2015

I still prefer using the verify-certificate event, it would be consistent with other APIs.

@deepak1556 deepak1556 force-pushed the certificate_verifier_api_patch branch 2 times, most recently from 21dfb74 to 46b3c2b Compare November 12, 2015 20:14
@deepak1556
Copy link
Member Author

@zcbenz have updated.

zcbenz
zcbenz reviewed Nov 16, 2015
View reviewed changes
atom/browser/api/atom_api_session.cc
} // namespace

Session::Session(AtomBrowserContext* browser_context)
: browser_context_(browser_context) {
AttachAsUserData(browser_context);

// Observe Browser to get certificate verification notification.
Browser::Get()->AddObserver(this);
Copy link
Member

Choose a reason for hiding this comment

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

Browser is application-wide, not session-wide. You can probably add a delegate class to AtomCertVerifier.

@deepak1556
Copy link
Member Author

@zcbenz have made the changes, thanks!

zcbenz
zcbenz reviewed Nov 16, 2015
View reviewed changes
atom/browser/atom_cert_verifier.cc
verify_result_,
base::Bind(&CertVerifyRequest::RunResult,
weak_ptr_factory_.GetWeakPtr()),
&new_out_req_,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pass the out_req from AtomCertVerifier::Verify here.

@zcbenz
Copy link
Member

zcbenz commented Nov 17, 2015

👍

zcbenz added a commit that referenced this pull request Nov 17, 2015
@zcbenz
Merge pull request #3344 from deepak1556/certificate_verifier_api_patch
24f573e 
session: api to allow handling certificate verification
@zcbenz zcbenz merged commit 24f573e into electron:master Nov 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@deepak1556 @zcbenz