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

Fallback mechanism on external authentication effectively makes email address a primary identifier #5872

Closed
Ithanil opened this issue Jul 1, 2024 · 5 comments

Comments

@Ithanil
Copy link
Contributor

Ithanil commented Jul 1, 2024

In app/controllers/external_controller.rb (https://github.com/bigbluebutton/greenlight/blob/6b1f93d8ca2e536702e0c6638627b31345d3665e/app/controllers/external_controller.rb#L34C1-L40C8) there is a fallback mechanism implemented, which effectively renders the email address a primary identifier of a user. According to the code, it behaves as follows:

  1. On Login via external authentication, it checks if a user with external_id already exists
  2. If it doesn't exist, it checks if the email address already exists for a user
  3. If a user with that email is found, it updates the external_id of that user with the value according to the current login

This is a very unexpected behavior in the context of external authentication, where the external_id should be the identifier of the user, and results in problems in environments where email addresses may eventually be reassigned to new persons.

Consider the following:

  1. User A logs into GL3 at some point
  2. User A leaves the organization or changes his email address
  3. User B joins the organization and receives the email address that User A once had
  4. User B logs into GL3 and has now access to the account/rooms of User A, including all recordings

I'm not sure in which context you would want such a fallback, but it should be at the minimum configurable and not be the default.

@Ithanil Ithanil changed the title Fallback mechanism on external authentication effectively makes email address the primary identifier Fallback mechanism on external authentication effectively makes email address a primary identifier Jul 1, 2024
@farhatahmad
Copy link
Collaborator

This was done as a back up when we were migrating from v2 to v3. The fallback can be removed

@FSeifer
Copy link

FSeifer commented Jul 31, 2024

@farhatahmad, @Ithanil

I generally agree that the external_id of a user should be the leading indentifier and thus we should be able to update a changed email address....

But what would be the expected behaviour in the following scenario:

  • A GLV3-Setup with local accounts is switched to an IDP Setup
  • Hence there are users, who have an email-address set but no external_id yet
  • How would the initial mapping be done if not via the email?

I feel this is not something we can toggle via a configuration switch but rather a fundamental feature of the IDP-controller..

I hope you can understand what I mean.

Regards
FSeifer

@Ithanil
Copy link
Contributor Author

Ithanil commented Jul 31, 2024

@FSeifer What I meant with my response in #5876 is a config flag to enable the previous behavior, i.e. fallback to mail as ID. If the previous behavior was what you wanted, that should serve you well, right?

Otherwise, in your situation I would go and update all the external_id entries in the database to their correct new values according to the IDP.

@FSeifer
Copy link

FSeifer commented Jul 31, 2024

@Ithanil
But can that be our goal?
To force admins who want to switch from local accounts to an IDP to manually change the greenlight databse entries of potentially hundreds or thousands of users and update it with some arbitrary external_id in the hope that a mapping can take place?

For example in my specific setup, I am not even sure how the external IDP-ID is generated. It seems to be derived from the OIDC-Token somehow, but I can't confirm this, as the communication between greenlight and the IDP is encrypted...

I guess the question is, what scenario do we think is more common?
The one where a new greenlight is connected to an IDP, so there is no mapping to be done, in which case everything works out of the box, or a scenario where we have pre-existing users, that have to be mapped... Ideally automatically according to some kind of criteria....

At the least, I would suggest to make the old mapping behaviour default again and give people the option to deactivate it via a flag like you suggested.

Regards
FSeifer

@Ithanil
Copy link
Contributor Author

Ithanil commented Jul 31, 2024

@Ithanil But can that be our goal? To force admins who want to switch from local accounts to an IDP to manually change the greenlight databse entries of potentially hundreds or thousands of users and update it with some arbitrary external_id in the hope that a mapping can take place?

For example in my specific setup, I am not even sure how the external IDP-ID is generated. It seems to be derived from the OIDC-Token somehow, but I can't confirm this, as the communication between greenlight and the IDP is encrypted...

I guess the question is, what scenario do we think is more common? The one where a new greenlight is connected to an IDP, so there is no mapping to be done, in which case everything works out of the box, or a scenario where we have pre-existing users, that have to be mapped... Ideally automatically according to some kind of criteria....

At the least, I would suggest to make the old mapping behaviour default again and give people the option to deactivate it via a flag like you suggested.

Regards FSeifer

As I said, for that case it might be the best idea to reintroduce the fallback mechanism behind a config flag. Having the fallback as default is a bad idea, for the reasons I stated in the issue above. It is unexpected behavior and in the worst case it gives users access to other users accounts in GL.

Anyway, keep in mind I'm just an external contributor and not maintainer of this project.

SebastianAppDev added a commit to SebastianAppDev/greenlight that referenced this issue Aug 2, 2024
farhatahmad added a commit that referenced this issue Sep 16, 2024
#5930)

* Email fallback as discussed in Issue #5872

* fix use of wrong methode

---------

Co-authored-by: Ahmad Farhat <ahmad.af.farhat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants