ckan / ckan

CKAN is an open-source DMS (data management system) for powering data hubs and data portals. CKAN makes it easy to publish, share and use data. It powers catalog.data.gov, open.canada.ca/data, data.humdata.org among many other sites.
https://ckan.org/
Other
4.48k stars 2k forks source link

Password reset fails in internal server error when ckan.auth.public_user_details is configured to false #5185

Open Zharktas opened 4 years ago

Zharktas commented 4 years ago

CKAN Version if known (or site URL)

2.8.3

Please describe the expected behaviour

When ckan.auth.public_user_details is configured to false, password reset requests should work.

Please describe the actual behaviour

Password reset request fails in internal server error as https://github.com/ckan/ckan/blob/dev-v2.8/ckan/views/user.py#L511 does not include ignore_auth.

What steps can be taken to reproduce the issue?

Configure ckan.auth.public_user_details to false. Try requesting password reset.

https://github.com/ckan/ckan/pull/4636 probably should be backported which includes ignore_auth to password reset.

Zharktas commented 4 years ago

https://github.com/ckan/ckan/pull/4636/commits/8a5beb9b53d23173c8d072926a6b84f76131fcd8#diff-989e559988ddb2baccfd41061a98e135R512, ignore_auth was added in part on larger commit, so cherry-picking would add all sorts of things, so it might be that wiser just to add it with another commit instead.

ThrawnCA commented 4 years ago

Testing of CKAN 2.8.4 shows that this is incomplete. Password reset emails can now be successfully sent, but the link to actually perform the reset does not work.

Compare 0fbe25e281055d53279f20e17a7587a891fd8534, in the qld-gov-au fork, which fixes this.

Zharktas commented 4 years ago

I'll look into it, probably today at some point.

Zharktas commented 4 years ago

I tested reset links in 2.8.4 and the passwords were reset like they should. And they are quite well tested in tests too. You probably should file a new issue about it.

ThrawnCA commented 4 years ago

@Zharktas To be clear, the failure happens when ckan.auth.public_user_details is false (which is what this issue is about). I don't think that the tests cover that.

Zharktas commented 4 years ago

@ThrawnCA well, technically you are correct, tests don't cover that.

But I precisely tested this while setting public_user_details to false. Only place where it might fail is during reset process is during the actual reset itself which calls user_show https://github.com/ckan/ckan/blob/ckan-2.8.4/ckan/views/user.py#L594, which checks the public_user_details https://github.com/ckan/ckan/blob/ckan-2.8.4/ckan/logic/auth/get.py#L218, but since it auths itself against user_reset https://github.com/ckan/ckan/blob/ckan-2.8.4/ckan/views/user.py#L589 which is always allowed https://github.com/ckan/ckan/blob/ckan-2.8.4/ckan/logic/auth/get.py#L338, it will not fail. At least on my environment auth function for user_show is never called, so I would require more information from your end as I can't reproduce this.

ThrawnCA commented 4 years ago

You're correct that the failure point is in user_show when actually performing the reset. When public_user_details is false, it calls restrict_anon(context), which immediately fails if not logged in.

Furthermore, extensions can override the user_show auth function with some other implementation (we have an implementation that tightens it up so you have to be an organisation or group admin to list or display other users, not just logged in), making it all the more necessary to simply skip all authorisation checks when resetting passwords.

Zharktas commented 4 years ago

But I would like to fix the actual issue though, it is somewhat counter intuitive that reset view calls user_reset auth function instead of user_show and for me at least user_show is never called. But I need a reproducible test case to find out the actual issue, instead of just ignoring authentication.

ThrawnCA commented 4 years ago

I don't think it's counter intuitive, really. The reset process needs to load up user details to do its work. Normally that is restricted, but in this case the presence of the reset key gives us sufficient confidence that the user is entitled to access the account anyway, so bypassing the usual auth checks is logical.

Zharktas commented 4 years ago

With counter intuitiveness I meant that it calls different auth function, usually they are the same than the action itself.

ThrawnCA commented 4 years ago

Well, it only explicitly calls the auth function for user_reset, but then calling user_show (because we need user details in order to perform the reset) implicitly calls that auth function too. (And that's the part where we don't really need or want the implicit auth check, thus I think it should set ignore_auth)

Zharktas commented 4 years ago

I was able to reproduce this in one of our projects so I'll do proper fix for it.

ThrawnCA commented 4 years ago

0fbe25e has a fix, although it doesn't have tests.

Zharktas commented 4 years ago

@ThrawnCA are you chaining user_show or user_update auth functions or overriding them in anyway? I've been tracking why I can't reproduce this in stock ckan and I believe it's due the auth_allow_anonymous_access not present in chained auth functions. There is a related issue https://github.com/ckan/ckan/issues/4597. I've somewhat disliking just ignoring the authentication if I can't produce a test proving it being broken, even though it will fix the issue.

ThrawnCA commented 4 years ago

Yes, we do override it, actually. Are you saying that if our override is annotated to allow anonymous access, that would fix the problem?

Zharktas commented 4 years ago

In my experience it would, we did a fix in our instance https://github.com/vrk-kpa/api-catalog/pull/47/files and it solved it. The patch to the core is ugly, it needs a better solution though.

The problem emerges as https://github.com/ckan/ckan/blob/master/ckan/authz.py#L212 is_authorized returns early if the auth function is not annotated for anonymous access and the actual auth function is never called.

TomeCirun commented 2 years ago

Testing of CKAN 2.8.4 shows that this is incomplete. Password reset emails can now be successfully sent, but the link to actually perform the reset does not work.

can you try to change user.id to user.name here: https://github.com/ckan/ckan/blob/96cb766218f723a398cd2a03059bd32b978701f1/ckan/lib/mailer.py#L171 and see if this helps because at line 304: https://github.com/ckan/ckan/blob/96cb766218f723a398cd2a03059bd32b978701f1/ckan/logic/__init__.py#L304 we are trying to get the user by its name ?

@ThrawnCA

ThrawnCA commented 2 years ago

@TomeCirun I'm fairly sure that's not the problem, because we've been successfully running on our fork of 2.8.8 (with ignore_auth set before calling user_show, see https://github.com/qld-gov-au/ckan/commit/8d817feb3343bfc9f36c8eb9aa4aaed90150acc0) for quite some time now.

gp86041 commented 2 years ago

I am seeing "internal server error" when I reset password.

ThrawnCA commented 2 years ago

I am seeing "internal server error" when I reset password.

What is the stack trace?

gp86041 commented 2 years ago

I am a noob in this. We are just using an image from the Amazon Web Service: https://aws.amazon.com/marketplace/pp/prodview-ag6z2fqishchg.

ThrawnCA commented 2 years ago

@gp86041 Have you set ckan.auth.public_user_details to False, and does the problem go away if it's set to True? If not, then it's unrelated to this issue.

markstuart commented 1 year ago

@Zharktas I'm having issues with password reset in CKAN 2.10.1

I've removed all plugins from my config to hopefully rule out any conflicting user_show overrides, and I have ckan.auth.public_user_details set to False.

I'm seeing this error after I click the link in the email: password_reset_error

I added some debug logging to try to dig into the problem:

ckan  | 2023-10-16 21:41:00,390 CRITI [ckan.authz] AUTH CONTEXT {'model': <module 'ckan.model' from '/ckan_repo_code/src/ckan-core/ckan/model/__init__.py'>, 'session': <sqlalchemy.orm.scoping.scoped_session object at 0x7f1664c11360>, 'user': '00cd4042-e0a0-43ab-b2e4-2dfbdcc37a1b', 'keep_email': True, 'auth_user_obj': <User id=00cd4042-e0a0-43ab-b2e4-2dfbdcc37a1b name=optimusprime password=$pbkdf2-sha512$25000$kTIGQAih9N4b43zvfc85Bw$0MPiEFxqEFkh/7t5RYB4AXcMi3o1Us7wdaUdqdDMjq1C5RlhIqbqswsOYDNg6wig/dEfoIZmAWL8985GAb5rag fullname=None email=optimusprime@autobots.com apikey=None created=2023-10-16 01:06:09.277002 reset_key=08c06bfafd3178658b65b34d1aa78734 about=None last_active=None activity_streams_email_notifications=False sysadmin=False state=active image_url=None plugin_extras=None>, '__auth_user_obj_checked': True, '__auth_audit': [], 'user_obj': <User id=00cd4042-e0a0-43ab-b2e4-2dfbdcc37a1b name=optimusprime password=$pbkdf2-sha512$25000$kTIGQAih9N4b43zvfc85Bw$0MPiEFxqEFkh/7t5RYB4AXcMi3o1Us7wdaUdqdDMjq1C5RlhIqbqswsOYDNg6wig/dEfoIZmAWL8985GAb5rag fullname=None email=optimusprime@autobots.com apikey=None created=2023-10-16 01:06:09.277002 reset_key=08c06bfafd3178658b65b34d1aa78734 about=None last_active=None activity_streams_email_notifications=False sysadmin=False state=active image_url=None plugin_extras=None>}
ckan  | 2023-10-16 21:41:00,391 CRITI [ckan.authz] AUTH FUNCTION <function user_show at 0x7f1660a8eef0>
ckan  | 2023-10-16 21:41:00,391 CRITI [ckan.authz] CALLING auth_function
ckan  | 2023-10-16 21:41:00,391 CRITI [ckan.logic.auth.get] user_show context {'model': <module 'ckan.model' from '/ckan_repo_code/src/ckan-core/ckan/model/__init__.py'>, 'session': <sqlalchemy.orm.scoping.scoped_session object at 0x7f1664c11360>, 'user': '00cd4042-e0a0-43ab-b2e4-2dfbdcc37a1b', 'keep_email': True, 'auth_user_obj': <User id=00cd4042-e0a0-43ab-b2e4-2dfbdcc37a1b name=optimusprime password=$pbkdf2-sha512$25000$kTIGQAih9N4b43zvfc85Bw$0MPiEFxqEFkh/7t5RYB4AXcMi3o1Us7wdaUdqdDMjq1C5RlhIqbqswsOYDNg6wig/dEfoIZmAWL8985GAb5rag fullname=None email=optimusprime@autobots.com apikey=None created=2023-10-16 01:06:09.277002 reset_key=08c06bfafd3178658b65b34d1aa78734 about=None last_active=None activity_streams_email_notifications=False sysadmin=False state=active image_url=None plugin_extras=None>, '__auth_user_obj_checked': True, '__auth_audit': [], 'user_obj': <User id=00cd4042-e0a0-43ab-b2e4-2dfbdcc37a1b name=optimusprime password=$pbkdf2-sha512$25000$kTIGQAih9N4b43zvfc85Bw$0MPiEFxqEFkh/7t5RYB4AXcMi3o1Us7wdaUdqdDMjq1C5RlhIqbqswsOYDNg6wig/dEfoIZmAWL8985GAb5rag fullname=None email=optimusprime@autobots.com apikey=None created=2023-10-16 01:06:09.277002 reset_key=08c06bfafd3178658b65b34d1aa78734 about=None last_active=None activity_streams_email_notifications=False sysadmin=False state=active image_url=None plugin_extras=None>}
ckan  | 2023-10-16 21:41:00,391 CRITI [ckan.logic.auth] user <ckan.model.user.AnonymousUser object at 0x7f1661421de0>

The first 3 lines are in ckan.auth is_authorised, and clearly there has been a user hydrated into the context from the reset token. The next line is in ckan.logic.auth.get user_show (also with user info in context):

def user_show(context: Context, data_dict: DataDict) -> AuthResult:
    # By default, user details can be read by anyone, but some properties like
    # the API key are stripped at the action level if not not logged in.
    log.critical('user_show context %s', context)
    if not config.get('ckan.auth.public_user_details'):
        return restrict_anon(context)
    else:
        return {'success': True}

As expected, the logic goes into ckan.logic.auth restrict_anon:

def restrict_anon(context: Context) -> AuthResult:
    log.critical('user %s', current_user)
    if current_user.is_anonymous:
        return {'success': False}
    else:
        return {'success': True}

And flask_login's current_user is an AnonymousUser at this point, so the auth check fails.

I'd suggest CKAN isn't registering the user with flask-login when it's hydrating the user from the reset token?

Any ideas to help me get past this hurdle in the interim? My first thought was to override user_show or monkey patch restrict_anon in a plugin.

markstuart commented 1 year ago

I can confirm that this must be a disparity between the flask-login state and the context state in this scenario. If I revert the implementation of restrict_anon to what it was in CKAN 2.9, then the password reset works:

def restrict_anon(context: Context) -> AuthResult:
    if authz.auth_is_anon_user(context):
        return {'success': False}
    else:
        return {'success': True}
Zharktas commented 1 year ago

@markstuart I've happened to arrive the same conclusion in #7869 and made PR #7871 with tests.

markstuart commented 1 year ago

Great, thanks for that @Zharktas, looks like CKAN 2.10.2 will be a bit more stable :)