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

Logging in on the wrong account when an email address is not unique #4039

Closed
wlohman-surfsara opened this issue Jun 27, 2022 · 5 comments · Fixed by #4200
Closed

Logging in on the wrong account when an email address is not unique #4039

wlohman-surfsara opened this issue Jun 27, 2022 · 5 comments · Fixed by #4200
Assignees
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug

Comments

@wlohman-surfsara
Copy link

wlohman-surfsara commented Jun 27, 2022

Description

On a free trial instance of the oCIS beta program

When two accounts have the same email address, logging in with the username/password of the second account logs you in to the first account

Steps to reproduce

  1. Log in as admin
  2. Create a user
    username: user1
    displayname: User One
    email: notunique@example.com
    password: somethingrandom
  3. Create a second user
    username: user2
    displayname: User Two
    email: notunique@example.com
    password: differentpassword
  4. Create a space user1_is_viewer
  5. Assign user1 as viewer to this space
  6. Create a space user2_is_viewer
  7. Assign user2 as viewer to this space
  8. Log out as admin
  9. Log in with user2/differentpassword

Expected behavior

I expect to be logged in with user2, I expect to have access to the space user2_is_viewer

Actual behavior

I am logged in as user1, I have access to the space user1_is_viewer

When I log back in as admin and change one of the email addresses the issue goes away

The JWT token contains the correct username (user2).

Setup

This is tested on my free trial instance of the oCIS beta program with Firefox and Chrome

@rhafer
Copy link
Contributor

rhafer commented Jun 27, 2022

@wlohman-surfsara Thanks, a lot for the report!

Indeed we currently to require the mail address to be unique (I guess we might want to reconsider this)

@wkloucek Additionally I think we should consider switching our defaults for PROXY_USER_OIDC_CLAIM and PROXY_USER_CS3_CLAIM away from the email address and use the username there instead. What do you think?

@phil-davis
Copy link
Contributor

Note: for business users IMO the email address that goes with each cloud storage user will be unique. (I am struggling to think of when a business would not do that)

For family cloud storage, it may still be "relatively common"/"happen" that multiple members of the family share an email account, but they have an account each on the cloud storage. I found this from way back in 2014 https://www.salon.com/2014/02/12/couples_get_your_own_email_accounts/ - I have no idea what the percentage is these days of couples that still use a shared email account.

So I don't really know if in "the modern internet" nowadays, apps/web sites/... would just insist that there can only be one username/account per email address.

@wkloucek
Copy link
Contributor

Indeed we currently to require the mail address to be unique (I guess we might want to reconsider this)

We require it but do not enforce it (which is bad). If we want to enable login with email address, we would need to enforce the uniqueness of email addresses, too #2572.

@wkloucek Additionally I think we should consider switching our defaults for PROXY_USER_OIDC_CLAIM and PROXY_USER_CS3_CLAIM away from the email address and use the username there instead. What do you think?

We should do this. It works with the builtin IDP and IDM, because the IDM does not allow duplicate usernames.

For external IDM / IDP configuration probably needs to be adapted. External IDPs also have further challenges in that direction #3866

@micbar can you assign a priority?

@wkloucek wkloucek added the Priority:p2-high Escalation, on top of current planning, release blocker label Jul 4, 2022
@micbar micbar added this to the 2.0.0 General Availability milestone Jul 13, 2022
@micbar micbar added Priority:p1-urgent Consider a hotfix release with only that fix and removed Priority:p2-high Escalation, on top of current planning, release blocker labels Jul 13, 2022
@rhafer rhafer self-assigned this Jul 14, 2022
@rhafer
Copy link
Contributor

rhafer commented Jul 14, 2022

While it should be possible to mitigate this issue with a configuration change, as outlined above, I just realized that there is also a bug in libregraph/idm (libregraph/idm#72) that made the problem worse ;-(

@rhafer
Copy link
Contributor

rhafer commented Jul 14, 2022

Partial fix: libregraph/idm#73 Note: with that neither of users sharing the same mail address, will be able to login. (Working on the ocis side fix now)

rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
Up to now the builtin lico was using the "username" as the login
attribute, while the proxy (and to some extend the auth-basic) service
tried to uniquely identify users by mail address. This aligns the
default configuration of the services to use the username everywhere.

Fixes: owncloud#4039
rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
Up to now the builtin lico was using the "username" as the login
attribute, while the proxy (and to some extend the auth-basic) service
tried to uniquely identify users by mail address. This aligns the
default configuration of the services to use the username everywhere.

Fixes: owncloud#4039
rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
Up to now the builtin lico was using the "username" as the login
attribute, while the proxy (and to some extend the auth-basic) service
tried to uniquely identify users by mail address. This aligns the
default configuration of the services to use the username everywhere.

Fixes: owncloud#4039
rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
Up to now the builtin lico was using the "username" as the login
attribute, while the proxy (and to some extend the auth-basic) service
tried to uniquely identify users by mail address. This aligns the
default configuration of the services to use the username everywhere.

Fixes: owncloud#4039
rhafer added a commit that referenced this issue Jul 14, 2022
rhafer added a commit that referenced this issue Jul 14, 2022
Up to now the builtin lico was using the "username" as the login
attribute, while the proxy (and to some extend the auth-basic) service
tried to uniquely identify users by mail address. This aligns the
default configuration of the services to use the username everywhere.

Fixes: #4039
ownclouders pushed a commit that referenced this issue Jul 14, 2022
Author: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date:   Thu Jul 14 12:23:20 2022 +0200

    Align default login attribute across services

    Up to now the builtin lico was using the "username" as the login
    attribute, while the proxy (and to some extend the auth-basic) service
    tried to uniquely identify users by mail address. This aligns the
    default configuration of the services to use the username everywhere.

    Fixes: #4039
rhafer added a commit to rhafer/ocis that referenced this issue Jul 14, 2022
@micbar micbar mentioned this issue Jul 19, 2022
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants