Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix exception on LDAP mapping during login #12693

Merged
merged 4 commits into from Dec 17, 2018

Conversation

@blizzz
Copy link
Member

blizzz commented Nov 27, 2018

fixes #11474

It seems the issue was introduced with c92d742, but I cannot (and will not :D) blame @weeman1337 for it. It is a side effect of how the LDAP Backend detects name collisions when creating a mapping, and caching.

A user object is fetched before matching, the yet non-existing user is found as non-existing and cached accordingly. The mapping follows later, is successful, but when a hook to announce this user is thrown a different listener wants to pull and fetch the brand new user object, but get's a null instead (and does not check for it).

The fix is essentially to cache the user after mapping. I removed some duplication as it is stupid to maintain and had a copy-paste issue in the past.

blizzz added 2 commits Nov 27, 2018
during login they might be cached as non-existing and cause an Exception
in the long run

reduces some duplication, too

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Copy link
Member

rullzer left a comment

🐘

@weeman1337

This comment has been minimized.

Copy link
Contributor

weeman1337 commented Nov 29, 2018

but I cannot (and will not :D) blame @weeman1337 for it.

@blizzz ha ha :) Then I would thank you for not blaming me ;)

Is there any way I can test this? May it make sense to add some testing stuff here?

@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Nov 29, 2018

Is there any way I can test this?

Yes, have LDAP and memcache configured and do a first login with an unmapped user (→ one that does not appear in the Users page, best don't open it after configuring, just try the login directly). Before you will have the Exception happening exactly once (and the user would not be added to the system addressbook).

May it make sense to add some testing stuff here?

In general yes, just to increase the possible cases… It probably would be already caught if the LDAP integration tests would go with a memcache. But all the tests would need to be duplicated, or run twice somehow, with a different configuration.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Dec 3, 2018

I think I enabled now Redis with the LDAP integration tests. This should suffice, as running it without memcache is not ideal or recommended

@SolomonSklash

This comment has been minimized.

Copy link

SolomonSklash commented Dec 10, 2018

Hello,
Just wanted to see if there are any updates on this PR. There are a lot of Cloudron users eagerly awaiting this landing in order to upgrade to Nextcloud 14/15.
Thanks!

@nebulade

This comment has been minimized.

Copy link

nebulade commented Dec 13, 2018

Just wanted to let you know, that we at Cloudron have successfully tested this against 14.0.4
@blizzz thanks a lot for the fix!

@doodlemania2

This comment has been minimized.

Copy link

doodlemania2 commented Dec 15, 2018

Pretty please!?

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Dec 17, 2018

Added clearing the cache on modification via API and CLI as well (Config wizard did it already), so the integration tests now run and succeed with Redis :) @weeman1337 OK with you?

@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Dec 17, 2018

@nebulade thanks for testing and reporting back!

@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Dec 17, 2018

/backport to stable15

@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Dec 17, 2018

/backport to stable14

@juliushaertl

This comment has been minimized.

Copy link
Member

juliushaertl commented Dec 17, 2018

CI failures unrelated:

  • git fetch --no-tags --depth=1 origin +refs/pull/12693/merge:
    fatal: The remote end hung up unexpectedly
@blizzz blizzz merged commit e7950a5 into master Dec 17, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/drone/pr the build failed
Details
DCO DCO
Details
fixupbot No fixup commits found. The commit history is clean
Details
@blizzz blizzz deleted the fix/11474/fix-first-ldap-login branch Dec 17, 2018
@backportbot-nextcloud

This comment has been minimized.

Copy link

backportbot-nextcloud bot commented Dec 17, 2018

backport to stable15 in #13119

@backportbot-nextcloud

This comment has been minimized.

Copy link

backportbot-nextcloud bot commented Dec 17, 2018

The backport to stable14 failed. Please do this backport manually.

@danielbotting-mcr

This comment has been minimized.

Copy link

danielbotting-mcr commented Jan 23, 2019

Hi,

We've got a production environment Nextcloud 14 (docker) and a staging environment Nextcloud 15 (docker). I ran into the above issue when logging into the staging environment just after setting up LDAP app. I assumed I would see an update for the LDAP app that fixes this, but there is none. What do I need to resolve this issue in Nextcloud 14 and 15?

Thanks

@blizzz

This comment has been minimized.

Copy link
Member Author

blizzz commented Jan 23, 2019

The fix was deployed to 14.0.5 and 15.0.1. Compare your systems, if you run them at least, open a new bug report. Otherwise, update :)

@danielbotting-mcr

This comment has been minimized.

Copy link

danielbotting-mcr commented Jan 24, 2019

Blizz, Thanks for your prompt response, I am not running those versions. I shall update accordingly and open a bug report as applicable. Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.