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

Detect existing email/user on frontend and backend #168

Merged
merged 2 commits into from
Oct 24, 2017

Conversation

PVince81
Copy link
Contributor

Prevent creating users using an email that already exists on the
backend.

The frontend now discards the guest creation entry if there is at least
one exact match for the search terms, which could refer to the user id
or email address (the latter being handled by the autocomplete backend)

Fixes #167

Requires owncloud/core#29223 from core to be able to detect exact user matches. Without this core PR the app will still work, but the "Guest" entry will be visible. Clicking it will however display an error message. I think this is acceptable for people still on 10.0.3.

Prevent creating users using an email that already exists on the
backend.

The frontend now discards the guest creation entry if there is at least
one exact match for the search terms, which could refer to the user id
or email address (the latter being handled by the autocomplete backend)
@PVince81
Copy link
Contributor Author

PVince81 commented Oct 12, 2017

To test:

  • TEST: guest user can be created with a non-duplicate email address
  • TEST: existing local user who has an email address
  • TEST: existing local user whoses user id is the email address
  • TEST: existing LDAP user with email address, but login/display name is different
  • TEST: existing LDAP user with email address used also as login / user id
  • N/A TEST: existing LDAP user where email address is in a secondary search term field

@davitol can you test the LDAP cases above ? For the secondary search terms you might want to ask @tomneedham how to do it

@cdamken
Copy link

cdamken commented Oct 12, 2017

  • TEST: guest user can be created with a non-duplicate guest email address

Copy link
Contributor

@cortho cortho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable and works for me

@cdamken
Copy link

cdamken commented Oct 13, 2017

@PVince81 @cortho Now is not possible to create new users.

@PVince81
Copy link
Contributor Author

Works for me even when disabled federated sharing.

Would be good to check the JS console and config report in case some non-default setting were used with sharing that might affect this somehow.

@cortho
Copy link
Contributor

cortho commented Oct 16, 2017

Just double checked the creation of new guest users, which was working as expected with current core master and this branch. @cdamken Could it be possible, that in your case the guests app was not enabled for any reason?

@PVince81 fyi

@davitol
Copy link
Contributor

davitol commented Oct 16, 2017

  • A current scenario is:
    1 -You can share a file A [email protected] and it appears (guest) in the sharebox
    2 - You share another file B with [email protected] and (guest ) is no longer shown even when that guest user haven't accepted the mail yet. The mail receives both sharing mails from file A and B.
    3 - When guest user accepts through file B mail, when he logs in. The file A is also included in his account

@davitol
Copy link
Contributor

davitol commented Oct 16, 2017

TEST: existing LDAP user with email address, but login/display name is different
TEST: existing LDAP user with email address used also as login / user id

Tested and works

@PVince81
Copy link
Contributor Author

Let's not merge until we get clarification from @cdamken. I wonder if some option like "restrict sharing to own groups" or something is checked ?

@PVince81
Copy link
Contributor Author

@cdamken any news ? This important PR is blocked. Important because it fixes an issue that could cause trouble. Blocked because you found a case (need env info) which seem to break guest sharing completely.

@davitol
Copy link
Contributor

davitol commented Oct 18, 2017

TEST: existing LDAP user where email address is in a secondary search term field

@PVince81 userManager->find() by default searches for displayname, email, and their search terms
so email in their search terms wont have an effect.

@cdamken any news or info about the apps enabled, whitelist apps, environment etc so we can move forward with this PR?

@PVince81
Copy link
Contributor Author

@felixboehm can you help here ?

@PVince81
Copy link
Contributor Author

Ok, turns out this PR doesn't work with OC 10.0.3 but only with OC 10.0.4...

I'll try and make it compatible, it was supposed to be compatible.

@PVince81
Copy link
Contributor Author

  • TEST: all of the above with 10.0.3 🚫 FAIL

@PVince81
Copy link
Contributor Author

looks like I was too optimistic with the code and discarded my former workaround for 10.0.3... optimistic that people would upgrade the core version. Ok, I'll add back the workaround to make it compatible whenever the core changes are not there.

@PVince81
Copy link
Contributor Author

Ok, the last commit brings compatibility with 10.0.2 and 10.0.3.

I have tested the basic case with 10.0.2 and 10.0.3:

  • the "Add guest" entry properly appears, always
  • selecting "Add guest" for an existing email will show an error popup

Only with 10.0.4 the "Add guest" entry will disappear for existing email addresses.

@PVince81
Copy link
Contributor Author

@davitol can you retest, also with 10.0.3 ?

@cortho can you check the code additions ? it's basically the old code as fallback

@davitol
Copy link
Contributor

davitol commented Oct 23, 2017

@davitol can you retest, also with 10.0.3 ?

@PVince81 LGTM in 10.0.3. The guest user dropdown option is no longer available when the mail written has been already used by other guest user

@cortho
Copy link
Contributor

cortho commented Oct 23, 2017

@PVince81 code looks fine to me

@PVince81
Copy link
Contributor Author

@PVince81
Copy link
Contributor Author

@davitol ok to merge then ? The core PR is independent and an additional fix.

@davitol
Copy link
Contributor

davitol commented Oct 24, 2017

@davitol ok to merge then ? The core PR is independent and an additional fix.

@PVince81 Yes, I think is fine for merging

@PVince81 PVince81 merged commit 6986fb7 into master Oct 24, 2017
@PVince81 PVince81 deleted the detect-existing-email branch October 24, 2017 12:23
@davitol davitol mentioned this pull request Oct 26, 2017
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants