Opened 4 years ago

Closed 3 years ago

#6951 enhancement closed fixed (fixed)

twisted.web.static.File should allow customizing the ForbiddenResource

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/static-file-custom-forbidden-6951-2
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph, rwall

Description

Right now, we have this code which makes it harder for implementation to create their own custom forbidden resource.

        try:
            fileForReading = self.openForReading()
        except IOError, e:
            import errno
            if e[0] == errno.EACCES:
                return resource.ForbiddenResource().render(request)
            else:
                raise

Maybe this can be implemented in the same as childNotFound resource

childNotFound = resource.NoResource("File not found.")

Attachments (4)

6951-1.diff (1.2 KB) - added by Adi Roiban 4 years ago.
6951-2.diff (2.7 KB) - added by Adi Roiban 4 years ago.
6951-3.diff (3.1 KB) - added by Adi Roiban 4 years ago.
6951-4.diff (2.8 KB) - added by Adi Roiban 3 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have attached an initial patch.

It is as simple code and I have not created a dedicated test as I think that the current test_forbiddenResource should cover this part on Unix.

Please let me know if a dedicated test is required.

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6951-1.diff added

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

Keywords: review removed
Owner: set to Adi Roiban
Summary: twisted.web.static.File should allow subclassing the ForbiddenResourcetwisted.web.static.File should allow customizing the ForbiddenResource

Thanks. This looks like a nice feature. Can you add some unit tests?

Changed 4 years ago by Adi Roiban

Attachment: 6951-2.diff added

comment:4 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to Jean-Paul Calderone

Thanks for the review.

I have attached a new diff which includes a unit test.

It is a bit of duplicate from test_forbiddenResource , but it mocks the openForReading method so that we can have this test run on Windows.

It is more of an integration test since it checks full usage of the new custom 'forbidden' resource.

Thanks!

comment:5 Changed 4 years ago by Richard Wall

Just thought I'd mention that I did some related work in ticket:3765#comment:7 where I started writing a brand new set of static file rendering components. In that branch I added a pathNotFound initialiser argument which allows you to customise the behaviour without subclassing (source:branches/static-path-3765/twisted/web/static.py#L1125).

Not sure when I'm going to get round to finishing it though.

comment:6 Changed 4 years ago by Richard Wall

Owner: changed from Jean-Paul Calderone to Richard Wall
Status: newassigned

comment:7 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/static-file-custom-forbidden-6951

(In [41775]) Branching to 'static-file-custom-forbidden-6951'

comment:8 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Adi Roiban
Status: assignednew

Thanks Adi,

This does look useful. Surprised noone needed this before.

Notes:

As in my previous review, I note that the git a / b prefixes make the patch a little difficult to work with ...especially with the automated tools used by some twisted developers.

[richard@zorin static-file-custom-forbidden-6951]$ patch -p0 < ../../6951-2.diff can't find file to patch at input line 5 Perhaps you used the wrong -p or --strip option? The text leading up to this was:


|diff --git a/twisted/web/static.py b/twisted/web/static.py |index a8932e6..3ff2d7a 100644

a/twisted/web/static.py

|+++ b/twisted/web/static.py


File to patch: C [richard@zorin static-file-custom-forbidden-6951]$ patch -p1 < ../../6951-2.diff patching file twisted/web/static.py patching file twisted/web/test/test_static.py patching file twisted/web/topfiles/6951.misc

Patch applied cleanly though.

Points:

  1. twisted/web/topfiles/6951.misc
    1. This seems like a feature to me. Please add a .feature newsfile with a description
  2. twisted/web/test/test_static.py
    1. CustomForbiddenResource
      1. The fact that you have to create a static.File subclass seems ugly.
      2. Why not add a new "forbiddenResourceFactory" parameter to static.File.init? That way people can use this feature without having to create pointless subclasses.
      3. It also means that static.File._forbidden can be private -- rather than expanding the public API
      4. It should also make the test a little simpler.
    2. Add a test to demonstrate the default and overridden value of static.File.forbidden (and another showing that it can be overridden if you agree with the comment above)
    3. See source:branches/static-path-3765/twisted/web/test/test_static.py@39863#L1701 for an example of what I mean.
  3. twisted/web/static.py
    1. See suggested changes to static.File constructor arguments above.

Please address or answer the numbered points above and resubmit a new patch against trunk for review.

Find me on #twisted-dev if you'd like to discuss.

(once again, ignore the branch I created, that was premature)

Thanks.

-RichardW.

comment:9 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

As in my previous review, I note that the git a / b prefixes make the patch a little difficult to work with ...especially with the automated tools used by some twisted developers.

Sorry for that. The new diff should be without prefix.

Points:

  1. twisted/web/topfiles/6951.misc
    1. This seems like a feature to me. Please add a .feature newsfile

with a description

Done.

  1. twisted/web/test/test_static.py
    1. CustomForbiddenResource
      1. The fact that you have to create a static.File subclass seems

ugly.

  1. Why not add a new "forbiddenResourceFactory" parameter to static.File.init? That way people can use this feature

without

having to create pointless subclasses.

  1. It also means that static.File._forbidden can be private --

rather than expanding the public API

  1. It should also make the test a little simpler.

I used the same design as childNotFound.

As in #6933 , #6972 and maybe other ticket, twisted web has many things which requires customization. Instead of adding new arguments to File.init maybe we can define some configuration object and pass it around.

Let me know if you want to have this in init ... but then it would need a new ticket to also fix childNotFound.

I will open a discussion on twisted mailing list to gather feedback for configuration options.

  1. Add a test to demonstrate the default and overridden value of

static.File.forbidden (and another showing that it can be overridden if you agree with the comment above)

  1. See source:branches/static-

path-3765/twisted/web/test/test_static.py@39863#L1701 for an example of what I mean.

Not sure if this works as on is a class and another is an instance. I have added an ugly assertIsInstance assertion.

  1. twisted/web/static.py
    1. See suggested changes to static.File constructor arguments above.

Please address or answer the numbered points above and resubmit a new patch against trunk for review.

Please see attached patch and inline comments. I can go add add arguments to init methods, but I think that this is a generic problem and a separate ticket/discussion is required.

For this ticket it should use the same design as childNotFound.

Thanks for the review

Changed 4 years ago by Adi Roiban

Attachment: 6951-3.diff added

comment:10 Changed 3 years ago by Glyph

Author: rwallglyph, rwall
Branch: branches/static-file-custom-forbidden-6951branches/static-file-custom-forbidden-6951-2

(In [43375]) Branching to static-file-custom-forbidden-6951-2.

comment:11 Changed 3 years ago by Glyph

(In [43376]) apply 6951-3.diff refs #6951

comment:12 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks again, adiroiban :).

  1. It looks like there are some new twistedchecker errors (one of which is clearly a typo...).
  2. on another ticket recently, exarkun opined that test_something_somethingElse naming is not really in the spirit of the coding standard, since no actual dynamic dispatching is going on. For what it's worth, I was the one writing tests with that naming convention, but I think he's right, policy-wise :).
  3. I'm not too sure about the use of a cvar here. Generally speaking cvars are bad news; they're global, and can be overridden globally. Also, the extension point is treated like an ivar, (accessed via self, not a class) despite having a class-level and class-respected default. how would you feel about having this be a straight ivar instead, just to decrease (or rather, avoid further increasing) the amount of global state honored by this class? If you feel that class-level is really the right solution for consistency, or for some use-case I haven't considered, feel free to say so, but new global state should require at least a little extra justification every time.

The buildbots are otherwise very green. Please resubmit when you have some thoughts on how to address the above, this should be a good week to do that :-).

comment:13 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks for the review.

  • I have fixed the twistedchecker errors.
  • I have renamed the test to test_forbiddenResourceCustomize ... I still prefer test_forbiddenResource_customize as it is much easier to read that the test is targeted to the forbiddenResource attribute.
  • I don't want that forbidden resource to be a class member... I just find it easy to give it a default value as class member and easy to overwrite it in specialized classes. I have changed the code so that an instance member is used. Or maybe I can keep the previous code and just change the docstring. What do you think?

I have attached a new diff based on latest trunk.

Thanks!

PS: Sorry for the delay, I was busy last weeks.

Changed 3 years ago by Adi Roiban

Attachment: 6951-4.diff added

comment:14 Changed 3 years ago by Wilfredo Sánchez Vega

A downside to the cvar -> ivar change is that now it's instantiating a new ForbiddenResource object every time a File is instantiated. Further, it's doing so even when that ForbiddenResource is never used, so it's wasted churn.

comment:15 Changed 3 years ago by Wilfredo Sánchez Vega

glyph: what do you think of a /private/ cvar and a property that vends that but sets (and then vends) an overriding ivar?

I think that addresses the global state by making it clear that the default/global resource is meant to be immutable.

comment:16 Changed 3 years ago by Wilfredo Sánchez Vega

Quoting glyph from IRC:

wsanchez: sorry, don't get too hung up on the cvar thing wsanchez: we just have a history of polluting class scope with way too many default values because they just happen to be immutable even though they shouldn't be visible on the class wsanchez: it's more that I really want to make sure that issue is *raised* every time we add a @cvar default for something, not necessarily that we never do that, just that it shouldn't be the go-to default any more

adiroiban: I suggest going back to patch 3's cvar usage.

It seems like some documentation should discourage stepping on class variables due to the global state change problem, and setting overriding instance variables instead. But that doesn't seem like something to fix here, as it's a best practices thing not specific to this one case.

comment:17 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to Adi Roiban

comment:18 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks for the review.

6951-3.diff was already applied as HEAD of static-file-custom-forbidden-6951-2

I have fixed the variable names in tests as done in 6951-4.diff.

Please take another look.

Thanks!

comment:19 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to Adi Roiban

Cool. Test look good; I like the rendering of a marker. I'm noticing that there are no corresponding tests for childNotFound. That's not exactly this ticket, but perhaps you'd be game to add those opportunistically, since it's probably mostly identical.

OK to merge, but you'll need to camel-case some symbols to clear up a pair of new twistedchecker items… looks like you already fixed those… rebuilding… yup, good.

comment:20 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have added a test for childNotFound and also updated the docstrings to talk about class members.

Please take a look at latest changes.

Thanks!

comment:21 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to Adi Roiban

great thanks

comment:22 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [43934]) Merging static-file-custom-forbidden-6951-2: Allow custom resource for forbidden response.

Author: adiroiban Reviewer: rwall, wsanchez, glyph Fixes: #6951

twisted.web.static.File allows defining a custom resource for rendering forbidden pages.

Note: See TracTickets for help on using tickets.