search

inveniosoftware / invenio-accounts

Invenio module for managing user accounts.
https://invenio-accounts.readthedocs.io
MIT License
6 stars 67 forks source link

ext: make monkey patching optional #239

Closed jacquerie closed 7 years ago

jacquerie commented 7 years ago

The release of invenio-accounts==1.0.0b7 led to a failure in our build: https://travis-ci.org/inspirehep/inspire-next/jobs/262243957.

In particular, what seems to be failing is the fact that the user in our acceptance is not logged in, that is our mechanism for saving/restoring cookies to bypass manual authentication is no longer working. In fact, I see some commits between invenio-accounts==1.0.0b6 and invenio-accounts=1.0.0b7 that seem to relate to this: https://github.com/inveniosoftware/invenio-accounts/commit/c80ee61e686d8929dde6c0d7fad770729b7b12a8, https://github.com/inveniosoftware/invenio-accounts/commit/b611762ed65ce064c7743cf5847f497a32caf2f7, and https://github.com/inveniosoftware/invenio-accounts/commit/0d22f84bef21a0a25581abc1f3ac85c51a7f1055.

Note that login_user_via_session won't work for us here as we have no app.test_client object to use, as the acceptance test suite is interacting through Selenium with another application.

One possible fix is outlined in the title: we make the above monkey patching configurable, and we disable it in the configuration of INSPIRE. Another possibility is that we revert https://github.com/inspirehep/inspire-next/commit/63043361c5508c31d7c26ed728916f678c40589c on our side, but this incurs in a performance penalty in our test suite. There's probably more, but this is all I can think of right now...

CC: @lnielsen

lnielsen commented 7 years ago

Thanks for posting. It seems bit strange that your mechanism should not work. From a quick look, it seems that you make a normal browser login, which should give you back the session cookie, and that this session cookie should allow you to make further authenticated requests.

Do you have anyway to inspect which cookies you actually have in the cookie jar

The remember me cookie has been disabled, but you should nevertheless have the session cookie which should be fine.

Besides the remember me cookie, we disabled login via header but you don't seem to be using that:

GET /
Authorization: auth_token=.....
diegodelemos commented 7 years ago

Could it be related with the fact that you are using http (i.e. http://test-web:5000/login/?next=%2Fauthors%2Fnew) and talisman configuration prevents session cookies to be sent if https is not enabled? see https://github.com/inveniosoftware/invenio/issues/3815

lnielsen commented 7 years ago

@diegodelemos Tailsman is only installed via Invenio-App which I don't think they are using.

lnielsen commented 7 years ago

@jacquerie Did you manage to check which cookies you get back when you login and if the session cookie is being used for subsequent requests?

jacquerie commented 7 years ago

Did you manage to check which cookies you get back when you login and if the session cookie is being used for subsequent requests?

I did not yet do that, but I will ASAP. In https://github.com/inspirehep/inspire-next/pull/2609 I investigated if reverting our commit fixed the issue, and it did, and in another PR I mitigate the decreased performance of the tests, so at least we have a workaround on our side...

jacquerie commented 7 years ago

...and when I say ASAP, I mean: we've already reverted the commit on our side, and found a workaround to speed up that test suite, so this might not be needed from us after all.