search

silverstripe / cwp-core

CWP basic compatibility module
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

API Remove CwpControllerExtension and replace with configuration and/or a custom HTTPMiddleware #23

Closed robbieaverill closed 6 years ago

robbieaverill commented 6 years ago

See @tractorcow's comment from https://github.com/silverstripe/cwp-core/pull/22#issuecomment-360288446:

I think that most of this extension can be shifted either to config, or to a custom middleware instead.

List of recommendations for refactor at https://github.com/silverstripe/cwp-core/compare/master...creative-commoners:pulls/2.0/cwp-recommendations?expand=1

A custom basic auth middleware would be needed to handle new logic:

  • exclude based on configured IP

  • authenticate based on change password token

    But the rest is already codified in basic auth config variables.

phptek commented 6 years ago

To be honest I disagree with this approach. While @tractorcow idea is prob valid in the medium term, if you introduce features during an upgrade process (which I believe we are still dealing with) then you risk introducing all sorts of new problems that wern't there originally.

If that wasn't convincing enough, then you risk confusing the s*t out of me when I'm chasing my tail reporting bugs in code that is due to be superceded. Can we just make these cwp- modules work "as-is" and tag as 2.0.0 and then use a 2.1.x for these sorts of API changes?

As it is, I have an issue with CwpControllerExtension I as about to report when I saw this ticket :-(

Using SS Platform as my test-bed, I (and my client) get inconsistent results with BasicAuth displaying for the entire site (until you whitrelist their IP via CWP_IP_BYPASS_BASICAUTH except it works for me and partially works for them - they only see the dialogue at /admin). So if this logic is going to be refactored (which I strongly advise not doing while people like me are trying to develop on top of it at the same time as trying to help upgrade them) then where's the best replace to report the bug?

Yours sadly, Russ.

robbieaverill commented 6 years ago

Thanks for voicing your concern! We don't want to upset anyone who's already using these modules, even if they aren't stable yet.

Would you be happier with the idea if we ensured that it was done in a way that would be backwards compatible with the existing CWP environment variables, whilst still removing the duplicated code from this module?

phptek commented 6 years ago

Possibly. I'm already chasing multiple moving goalposts what's another one aye?

phptek commented 6 years ago

But for all that is holy (if not for me, for the CWP platform team, keep the constants the same yes)

tractorcow commented 6 years ago

I'm sorry about the upgrade challenges this poses, but if we are only able to make breaking changes at major version increments, and fail to make those changes, we run the risk of never being able to shift and advance our standards. Forever holding ourselves to historic standards will prevent us from being able to improve our self-expectations.

I don't want to change any of the constants or platform-level configuration (platform shouldn't need ss4 specific configs), but the implementation of core behaviour will need to be shifted from riskier locations (controller init methods) to reliable ones (ordered middleware).

robbieaverill commented 6 years ago

Pull requests at https://github.com/silverstripe/cwp-core/pull/30 and https://github.com/silverstripe/cwp/pull/43

robbieaverill commented 6 years ago

FYI @phptek we've finished this card - you'll still be able to use the CWP environment variable to whitelist IPs from basic authentication, but other rules like forcing SSL etc will need to use YAML configuration now