Problem/Motivation

Ban module currently uses an event subscriber to implements its feature. Sadly this requires quite some code to be executed
before ban module can start its work.

Proposed resolution

Use a HTTP middlware, which has the big advantage that it runs much earlier. At the same time, there is even less code to be executed compared
to before, because you have

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#17 ban-middleware-2332983-17.patch6.98 KBdawehner
#17 interdiff.txt1.21 KBdawehner
#12 interdiff.txt388 bytesdawehner
#12 ban-middleware-2332983-12.patch6.91 KBdawehner
#8 interdiff.txt4.01 KBdawehner
#8 ban-middleware-2332983-8.patch6.89 KBdawehner
ban.patch4.15 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

it's not clear, what is the order of the middleware executed?
Previously priority was used to order subscribers...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, this is coming after page caching for example. Priority = 0.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

so any reason in ban after page cache?
suppose it should go before page cache...

Crell’s picture

Crell CreditAttribution: Crell commented

I'm not sure there's much advantage in this case, is there? Ban module is rarely enabled in the first place since it's much slower than doing the same thing at a web server or proxy level...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, I decided to do it after page cache, as it is also after page cache in HEAD. We also should try to make page caching fast
but I agree that security might be more important.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

No tests? Is it a good idea to add some phpunit here?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented 
+++ b/core/modules/ban/src/BanMiddleware.php
@@ -0,0 +1,58 @@
+  /**
+   * Constructs a ReverseProxyMiddleware object.
+   *

Nit - wrong comment.

Besides that: RTBC from my POV.

I don't think you ban a person from the page cache as usually banning is for preventing POSTs, etc. and less for preventing any page views. And yes that discussion / ticket would need to be follow-up as this is just a straight conversion.

For Tests: Yes, we could have a MockBanManager and a MockRequest to unit test the request stack.

Not sure it is needed though.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes
FileSize
ban-middleware-2332983-8.patch6.89 KB
interdiff.txt4.01 KB

No tests? Is it a good idea to add some phpunit here?

There are already ban module tests ... and well, we do have a DB call here, so I wonder how useful that would be.

Well, let's just write a unit test, to fast fail, in case we change this class in the future.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC, great tests!

And shows nicely how useful simple and nice to use middlewares are :).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs review

Actually the fact that banned user's can view cached pages could be viewed as a regression from D7 see #1929506: Banned IP addresses can still access cached pages and the corresponding CR https://www.drupal.org/node/1941682.

We could actually reverse this decision and give ban a higher priority than the page cache. The reason given in the CR is no longer valid since middlewares makes it entirely possible for the ban module to work as it did in D7.

Setting back to needs review for a bit of discussion.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Still think we should ditch the whole thing, but if we have it, then having it functionally equivalent to the 7.x logic seems better yes.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +WSCCI
FileSize
ban-middleware-2332983-12.patch6.91 KB
interdiff.txt388 bytes

Simple as that.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

OK then.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Issue tags: +Needs change record updates
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I guess we should just remove [#1941682] after that?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Yeah I think we can just delete the old CR after commit. Nothing probably needs to reference it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt1.21 KB
ban-middleware-2332983-17.patch6.98 KB

One suggestion of chx.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record updates

Committed a8b92fe and pushed to 8.0.x. Thanks!

  • alexpott committed a8b92fe on 
    Issue #2332983 by dawehner: Replace ban event subscriber with a ban...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.