Allowing all blocked host and port

[purnendudash]

Not sure the purpose of this check

https://github.com/owncloud/moodle-repository_ocis/blob/7631344101dc97554f4f06915fe0933450dacb76/lib.php#L87

Its actually removes all host and port blocking. Could you please explain the same.

[individual-it]

This is for auto-provisioning, specially to make setup of automated test simpler. See also https://github.com/owncloud/moodle-repository_ocis?tab=readme-ov-file#auto-provisioning

[danmarsden]

nice catch purnendu - arbitrarily/silently changing moodle security settings is a blocker for approval.

please find a better way to do this - don't change moodle's core security settings directly within your code - the admin should have to turn this off manually themselves if required.

[danmarsden]

also the constant "MOODLE_DISABLE_CURL_SECURITY" does not follow our frankenstyle naming rules for constants (also a blocker for approval.)

please review: https://moodledev.io/general/development/policies/codingstyle/frankenstyle

[individual-it]

@danmarsden thank you for reviewing the extension

MOODLE_DISABLE_CURL_SECURITY is not a constant, but an environment variable, in moodle I see env. variables are used the same way https://github.com/moodle/moodle/blob/26649f5750ee3d35e56990512ee4c79d388cea8b/lib/aws-sdk/src/Token/ParsesIniTrait.php#L34 or https://github.com/moodle/moodle/blob/26649f5750ee3d35e56990512ee4c79d388cea8b/lib/aws-sdk/src/Credentials/EcsCredentialProvider.php#L87

I disagree that it arbitrarily/silently changing moodle security settings. It only changes it if MOODLE_DISABLE_CURL_SECURITY environment variable is set to true

I could not find any automatic way to change the security settings, so the alternative is not to run any automated tests in CI

[danmarsden]

If you need something like this for unit tests - put it in the tests themselves, not in your main lib and we can consider if they are appropriate there. I still think our policy for plugin specific environment vars requires frankenstyle but I'm not at my desk(replying on my phone right now.)

The files you link to there aren't moodle code at all but 3rd party libs which don't have the same requirements either.

[individual-it]

@danmarsden you are right about my examples, the other examples I see are in config.php, all the env. variables are following the schema TEST_EXTERNAL_FILES_HTTP_URL. Let's discuss it in #99. Happy to change that if needed.

coming back to MOODLE_DISABLE_CURL_SECURITY: this is not for unit test (we don't have any) but for

1. auto-provisioning of development environment
2. auto-provisioning for behat tests

not having 1. would be pretty annoying because there are multiple steps that needed to be done manually when setting up the plugin:

1. login as admin
2. change IP address in HTTP security settings
3. change ports in HTTP security settings
4. create a new OAuth2 service
5. create a new repository instance

But we could bite the bullet and live with that, if we cannot convince you that the admin has to be very deliberate to use this setting. Yes it is security related, but hard to do accidentally.

For 2. the tests: @amrita-shrestha please have a look if we can use set_config in the @BeforeScenario hook to set the config only for the test run. Or maybe we have to do something similar like we do for the sslproxy setting. See https://tracker.moodle.org/browse/MDL-80616 and https://github.com/owncloud/moodle-repository_ocis/blob/main/.drone.star#L308

[danmarsden]

Thanks @individual-it - for behat tests you can also use statements like:

    Given the following config values are set as admin:
      | curlsecurityblockedhosts | |

you might also be able to find another way of setting up the dev environment that doesn't quietly change security settings without intervention by the end-user.

feel free to push back again and we'll discuss it within the plugin review team on whether we will block approval on this, but it's definitely non-standard and breaks a number of the guidelines as it currently stands.

[individual-it]

@danmarsden putting the settings into the behat tests does not work, because we need them already during the creation of the OAuth service. But @amrita-shrestha found a way to do it by editing the config.php, so our code does not contain those settings lines anymore. See #104

[danmarsden]

nice work - thanks for cleaning that up!