FIX Lookup HTTPRequest from Injector before owner Controller

silverstripe / cwp-core

CWP basic compatibility module

BSD 3-Clause "New" or "Revised" License

3 stars 12 forks source link

Closed robbieaverill closed 6 years ago

robbieaverill commented 6 years ago

robbieaverill commented 6 years ago

Thanks for the review @phptek! I've pushed an updated commit =)

phptek commented 6 years ago

@robbieaverill Cheers. Mate, if it works in dev and test, just push it to master. It's always deemed unstable anyway.

robbieaverill commented 6 years ago

@phptek I think I was referring to environment types rather than branches

tractorcow commented 6 years ago

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

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.

robbieaverill commented 6 years ago

@tractorcow I've logged your suggestion as https://github.com/silverstripe/cwp-core/issues/23 which we'll attack shortly!

This doesn't block merging this PR and fixing the OP.

dhensby commented 6 years ago

Please test with framework 4.0.2 to confirm this is still an issue

tractorcow commented 6 years ago

I don't think this is the right request. The controller should be the source of truth for the request object first.

We can rely on injector state being consistent for a single request; The way this is is intentional.

Given that controllers are initialised with NullHTTPRequest on construction, it actually makes it less of a source of truth than injector state.

tractorcow commented 6 years ago

@dhensby my comments at https://github.com/silverstripe/cwp-core/pull/22#issuecomment-360288446 regarding turning this into a middleware would make this redundant, since request is passed into the middleware.

This PR is simply a temporary and less-than-perfect-but-acceptable patch in the mean time.