Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add a check for duplicate IdP ids #9184

Merged
merged 2 commits into from
Jan 21, 2021
Merged

Add a check for duplicate IdP ids #9184

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69e5e2c
Add a check for duplicate IdP ids
richvdh Jan 20, 2021
ac3d44d
Update out-of-date-comment
richvdh Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/9184.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Emit an error at startup if different Identity Providers are configured with the same `idp_id`.
12 changes: 12 additions & 0 deletions synapse/config/oidc_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# limitations under the License.

import string
from collections import Counter
from typing import Iterable, Optional, Tuple, Type

import attr
Expand Down Expand Up @@ -43,6 +44,17 @@ def read_config(self, config, **kwargs):
except DependencyException as e:
raise ConfigError(e.message) from e

# check we don't have any duplicate idp_ids
# XXX: this won't detect clashes with other IdP providers using other SSO
# mechanisms (such as SAML or CAS); that will be detected when we set up the
# listeners but by then synapse will have forked, so it's not ideal.
Copy link
Member

Choose a reason for hiding this comment

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

We could prefix them with an implementation specific bit if we wanted to avoid people having to care about this. 🤷 E.g. if you use google we would make that oidc-google or something of that nature.

Copy link
Member Author

Choose a reason for hiding this comment

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

true. If we're going to change it, we better do it before 1.26 ships and people start using the feature and getting entries in their user_external_ids table.

counterpoint: not including the SSO mechanism here means that we leave the path open for SSO providers to switch from, say, SAML to OIDC in the future without needing a db update (though that also requires the unique remote user id to remain constant, which seems unlikely on reflection)

Copy link
Member Author

Choose a reason for hiding this comment

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

on reflection: I think this change makes sense. I'm going to do it in a branch targetting release-v1.26.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok that's done in #9189. I don't think it changes anything in this PR apart from making this comment redundant so I'm going to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that seems fine. 👍

c = Counter([i.idp_id for i in self.oidc_providers])
for idp_id, count in c.items():
if count > 1:
raise ConfigError(
"Multiple OIDC providers have the idp_id %r." % idp_id
)
richvdh marked this conversation as resolved.
Show resolved Hide resolved

public_baseurl = self.public_baseurl
self.oidc_callback_url = public_baseurl + "_synapse/oidc/callback"

Expand Down