Opened 16 years ago

Closed 14 years ago

#1879 enhancement closed fixed (fixed)

Enable PBClientFactory and PBServerFactory to use custom security options.

Reported by: pgstudy Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch:
Author:

Description

Sometimes we wish to have custom security options, i.e., decide ourselves what Modules or Classes to receive. This can be done using Broker, however, PBClientFactory and PBServerFactory can have this feature as well. This is just a few lines of changes.

Attachments (4)

CSFactorySecurity.diff (1.3 KB) - added by pgstudy 16 years ago.
The patch
testnis.py (2.1 KB) - added by pgstudy 16 years ago.
test for patch
test_pbsec.py (803 bytes) - added by pgstudy 16 years ago.
Test without reactor.run()
CSFactorySecurity.2.diff (1.3 KB) - added by pgstudy 16 years ago.
security are instantiate at Broker for ServerFacotry

Download all attachments as: .zip

Change History (20)

Changed 16 years ago by pgstudy

Attachment: CSFactorySecurity.diff added

The patch

Changed 16 years ago by pgstudy

Attachment: testnis.py added

test for patch

comment:1 Changed 16 years ago by Jean-Paul Calderone

Owner: changed from Glyph to pgstudy

Tests shouldn't have prints in them. If there really is very important information which needs to be recorded for someone to analyse after the test run, use log.msg().

The test should be using twisted.trial.unittest instead of the stdlib unittest. This will let it....

Return a Deferred from the test method. Don't call reactor.run() or reactor.stop() in the test. But...

You don't actually need to set up a connection here. You just need to make sure that the broker is instantiated with the right security options. You can do this by directly calling buildProtocol on PBServerFactory and then inspecting the resulting object.

There should be a docstring explaining the purpose of the test.

The Twisted coding standard is here - <http://twistedmatrix.com/projects/core/documentation/howto/policy/coding-standard.html>. It isn't completely exhaustive, so also please take a look at some other code in Twisted and try to make yours look similar.

comment:2 Changed 16 years ago by pgstudy

However, you will note that i also changed the PBClientFactory security settings and thsoe are only assigned when the function PBClientFactory.clientConnectionMade is executed which i guess only happens when the connection is made :) I.e., how can i not call reactor run and stop?

comment:3 Changed 16 years ago by Jean-Paul Calderone

One possibility is to change the code. One mantra of XP is "if it is hard to test it is wrong". Another option is to use a loopback connection. The first option seems preferable to me: is there any strong reason not to assign the security options in buildProtocol?

comment:4 Changed 16 years ago by pgstudy

exarkun: For the PBServerFactory, the buildprotocol is where the Broker is instantiated. I don't know of any other suitable place. For PBClientFactory, the entry point for the broker is the connectionMade, again, i don't see any other suitable place.

What is a loopback connection, currently i connect to the local host. Point me to the suitable test example that uses loopback :)

Changed 16 years ago by pgstudy

Attachment: test_pbsec.py added

Test without reactor.run()

comment:5 Changed 16 years ago by Jean-Paul Calderone

Why can't the Broker be created in the same method which sets its security options?

There are tons of examples of loopback connections in Twisted's test suite. Just grep for 'lookback' in test_*.py and you should find plenty.

Changed 16 years ago by pgstudy

Attachment: CSFactorySecurity.2.diff added

security are instantiate at Broker for ServerFacotry

comment:6 Changed 16 years ago by pgstudy

ok, at least for the ServerFactory i changed the patch to set the security options when instatiating in the buildprotocol. In the client factory, the Broker is handed out from outside the clientfactory so seemingly it must be set directly.

comment:7 Changed 14 years ago by therve

(In [21323]) Add the security parameter to client and server factories, test it.

Refs #1879

comment:8 Changed 14 years ago by therve

Owner: changed from pgstudy to therve
Priority: normalhighest

This is done in pb-security-options-1879, waiting a bit.

comment:9 Changed 14 years ago by therve

Cc: therve added
Keywords: review added
Owner: therve deleted

Please review!

comment:10 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

None is a better dummy than "" I think - the four new tests pass the latter to buildProtocol.

The change to PBServerFactory's docstring to use C{} would be trivially better if it used L{} instead.

In the docstring for PBClientFactory.__init__, I think tracebacks for exceptions reads slightly better than tracebacks of exceptions. Same for PBServerFactory.__init__'s docstring.

There's a typo in the @type for security in both of those docstrings, too, spred. That could also be using L{} .

Another typo in the docstring for PBClientFactory.buildProtocol - broket.

The documentation for the root parameter to PBServerFactory is a bit misleading, since root isn't really the root Referenceable, but a factory which will provide the root reference (via its rootObject method).

So lots of nits. The actual functionality looks great.

comment:11 Changed 14 years ago by therve

(In [21541]) Process review comments.

Refs #1879

comment:12 Changed 14 years ago by therve

Keywords: review added
Owner: therve deleted

Thanks for your rigor :). Back to you.

comment:13 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Any reason PBServerFactory.__init__.__doc__ still has C{IPBRoot} and C{twisted.spread.jelly.SecurityOptions} instead of using L{} for each?

The rest looks great.

comment:14 Changed 14 years ago by therve

(In [21584]) Forgot some stuff.

Refs #1879

comment:15 Changed 14 years ago by therve

Resolution: fixed
Status: newclosed

(In [21593]) Merge pb-security-options-1879

Author: therve, pgstudy Reviewer: exarkun Fixes #1879

Enable pb factories to use custom security options.

comment:16 Changed 11 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.