Rename login_whitelist_ip config to login_allowlist_ip #16413
Conversation
tsteur
added
the
Pull Request WIP
tsteur
added
Needs Review
tsteur
changed the title
sgiehl
force-pushed
the
loginallowlist
branch
from
f4bf37b
to
5b83774
Compare
sgiehl
force-pushed
the
loginallowlist
branch
from
5b83774
to
e64574e
Compare
| @@ -60,72 +60,85 @@ public function tearDown(): void | |||
|
|
|||
| public function test_shouldWhitelistApplyToAPI_shouldBeEnabledByDefault() | |||
| { | |||
| $this->assertTrue($this->whitelist->shouldWhitelistApplyToAPI()); | |||
| $this->assertTrue($this->allowlist->shouldAllowlistApplyToAPI()); | |||
| } | |||
|
|
|||
| public function test_shouldWhitelistApplyToAPI_canBeDisabled() | |||
| { | |||
| $this->setGeneralConfig('login_whitelist_apply_to_reporting_api_requests', '0'); | |||
There was a problem hiding this comment.
| $this->setGeneralConfig('login_whitelist_apply_to_reporting_api_requests', '0'); | |
| $this->setGeneralConfig('login_allowlist_apply_to_reporting_api_requests', '0'); |
That should fix the failing test
There was a problem hiding this comment.
I'll commit this separately. I partially left some with whitelist to make sure we still support the previous setting re BC but will rather add an extra test for it
|
@sgiehl applied the review comments. replaced the word also in few other places but not in places like the validator or so when it requires changes across plugins as it's not as easy to do and needs a separate PR anyway |
| { | ||
| $general = $this->getGeneralConfig(); | ||
| return !empty($general['login_whitelist_apply_to_reporting_api_requests']); | ||
| return !empty($general['login_allowlist_apply_to_reporting_api_requests']) || !empty($general['login_whitelist_apply_to_reporting_api_requests']); |
There was a problem hiding this comment.
The fallback to the old name might actually not work as expected in all cases as the default value of those setting is 1.
Lets say someone does not configure login_allowlist_apply_to_reporting_api_requests, but uses login_whitelist_apply_to_reporting_api_requests = 0. That would actually not work, as login_allowlist_apply_to_reporting_api_requests defaults to 1.
I guess that might be fine, as login_allowlist_apply_to_reporting_api_requests will be set on update if needed, but actually I think we could remove the fallback here then...
|
Seems you added the screenshot directly, and not to LFS... |
|
fixed |
Not needed in developer changelog as we automatically migrate existing setting and if that doesn't work then we still support previous settings for BC (important as it is a security related feature, not always do config changes work eg when they forget to distribute it across multiple servers etc). Also it's not a developer setting but just a regular user setting.
Also renames
login_whitelist_apply_to_reporting_api_requeststologin_allowlist_apply_to_reporting_api_requestsonce merged will need to update the FAQ (I'll do this)