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

e2ee: also enable on p2p connections #1107

Merged
merged 2 commits into from Apr 21, 2020
Merged

e2ee: also enable on p2p connections #1107

merged 2 commits into from Apr 21, 2020

Conversation

fippo
Copy link
Member

@fippo fippo commented Apr 20, 2020

also enables e2ee on peer-to-peer connections. While a bit unnecessary
it avoids the confusing UX of things working until a third person joins

@fippo fippo requested a review from saghul April 20, 2020 05:35
@fippo
Copy link
Member Author

fippo commented Apr 20, 2020

@saghul I don't think that is quite enough and needs additional handling in _setupSenderE2EEForTrack (and the receiver), right?

@fippo
Copy link
Member Author

fippo commented Apr 20, 2020

(also is this more complicated? what about transitions...)

@fippo fippo changed the title e2ee: aso enable on p2p connections e2ee: also enable on p2p connections Apr 20, 2020
also enables e2ee on peer-to-peer connections. While a bit unnecessary
it avoids the confusing UX of things working until a third person joins
@saghul
Copy link
Member

saghul commented Apr 20, 2020

I'll check, I think some more stuff is needed.

@fippo
Copy link
Member Author

fippo commented Apr 20, 2020

I think I found them all. API change: _setupSenderE2EEForTrack gets the jvb/p2p session as first argument

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Let's make them symmetrical so it's easier to follow later on.

return;
}
const session = track.isP2P ? this.p2pJingleSession : this.jvbJingleSession;
const pc = session && session.peerconnection;
Copy link
Member

Choose a reason for hiding this comment

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

use getActiveTPC instead

if (!this._e2eeCtx) {
return;
}
const pc = session.peerconnection;
Copy link
Member

Choose a reason for hiding this comment

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

We ccould use geetActiveTPC` here as well instead of passsing the session around.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I want the peerconnection the track is received on, no matter what the current establishment status of the p2p ice connection is?

Copy link
Member

Choose a reason for hiding this comment

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

That is taken care of by caller function already, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this is called from JitsiConference.prototype.onCallAccepted (line 1701) for p2p and JitsiConference.prototype._acceptJvbIncomingCall for the sfu call. In both cases we know exactly where this is originating

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha.

@saghul saghul merged commit f97c37d into master Apr 21, 2020
@fippo fippo deleted the e2ee-p2p branch April 21, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants