search

data-govt-nz / ckanext-security

A CKAN extension to hold various security improvements for CKAN
GNU Affero General Public License v3.0
25 stars 32 forks source link

Account Lockout Email Outside of App Context #78

Open JVickery-TBS opened 1 month ago

JVickery-TBS commented 1 month ago

The account lockout emails will not work with flask render as the Authenticator classes are being loaded into the stack app at this time. So there is no available current_app to Authenticator classes.

So there is no way to really get the emails to work with flask/jinja template rendering inside of the CKAN app.

markstuart commented 1 month ago

Hi, thanks for raising this. Do you mean specifically this line https://github.com/data-govt-nz/ckanext-security/blob/62203cf4180b4288afc0b5841d0035afef45bc81/ckanext/security/mailer.py#L48?

JVickery-TBS commented 1 month ago

@markstuart I forgot to mention this is for CKAN <=2.9. I think the 2.10 support (as issued here https://github.com/data-govt-nz/ckanext-security/issues/77) would not have this issue as the authenticators work

Its actually just the entire mailer.py stuff. Mainly the use of the render, but yes, the flask.current_app would be an empty app at the time of the mailer.py script.

The mailer.py stuff gets called during the CKANLoginThrottle here: https://github.com/data-govt-nz/ckanext-security/blob/master/ckanext/security/authenticator.py#L89

So when we define the CKANLoginThrottle in a who.ini as an authenticator, those authenticator classes from who.ini do not have any app context yet as the app has not been fully initiated yet. The authenticator classes are some of the first things to be added to the app stack.

It works fine with the totp login as that is in a view and happens inside of the app and request contexts. But it is with the basic auth that the lockout email would not send.

I tried fixing it for our fork here: https://github.com/open-data/ckanext-security/pull/6/files#diff-6e70f488d50a5eec96c5c8b298e9d3de980caa56e3c5b57164a781562f0be466 which does work...but the downfall is that it would not use the flask/jinja app environment and render stuff. So it would prevent users/devs from overriding the email templates in their plugin.

Again, I don't think this would be an issue in 2.10 as you have to add your plugin to the list of plugins in the INI file and not the who.ini file. Which your CKANLoginThrottle is already implementing the plugin, so just need an entry point for 2.10: https://github.com/data-govt-nz/ckanext-security/blob/master/setup.py#L25 (or have the already existing plugin entry point class implement IAuthenticator and call the throttle stuff)

markstuart commented 1 month ago

Interesting... I guess this is an untested workflow that has only been possible since a recent change that added the config var to disable MFA. When the MFA support was added originally, this module was being tested against a CKAN 2.7.x instance, and there was no way to disable the MFA. Then we upgraded to CKAN 2.9.x and made the necessary modifications to this plugin to support 2.7/2.8/2.9. Then the 2.10.x upgrade, and more changes, and support for CKAN < 2.9 was dropped. Around that same time, an external contributor provided the changes for the MFA config option to disable, but we had no easy way to test that against a 2.9 installation at that time.

Thanks for catching this. It'd be really appreciated if you could raise a pull request that only fixes the mailer for CKAN 2.9 when MFA is disabled, and includes a README note to make end users aware of the inability to override the template in this specific use case.

markstuart commented 1 month ago

Obviously, if that's not something you have capacity for we can look at other options 😃

JVickery-TBS commented 1 month ago

@markstuart yeah that sounds good! I am having a colleague review some of our security fork changes today. So I will work this PR into my next week's work.

Also it looks like it was us who made that PR for disabling totp (https://github.com/data-govt-nz/ckanext-security/pull/52), so it is only fitting I fix this now hahaha.

I will do a check on totp_disabled and if the version is less than 2.10, then I will do the fix I made. Otherwise, it should work with 2.10 (will confirm that too for sure).

JVickery-TBS commented 1 month ago

@markstuart should be fixed with this: https://github.com/data-govt-nz/ckanext-security/pull/81

Just need to add the check for totp_disabled/totp_enabled and add to the README.