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

Support for scraping email addresses from OIDC providers #9245

Merged
merged 2 commits into from
Jan 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/9245.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support to the OpenID Connect integration for adding the user's email address.
15 changes: 12 additions & 3 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1791,9 +1791,9 @@ saml2_config:
#
# For the default provider, the following settings are available:
#
# sub: name of the claim containing a unique identifier for the
# user. Defaults to 'sub', which OpenID Connect compliant
# providers should provide.
# subject_claim: name of the claim containing a unique identifier
# for the user. Defaults to 'sub', which OpenID Connect
# compliant providers should provide.
#
# localpart_template: Jinja2 template for the localpart of the MXID.
# If this is not set, the user will be prompted to choose their
Expand All @@ -1802,6 +1802,9 @@ saml2_config:
# display_name_template: Jinja2 template for the display name to set
# on first login. If unset, no displayname will be set.
#
# email_template: Jinja2 template for the email address of the user.
# If unset, no email address will be added to the account.
#
# extra_attributes: a map of Jinja2 templates for extra attributes
# to send back to the client during login.
# Note that these are non-standard and clients will ignore them
Expand Down Expand Up @@ -1837,6 +1840,12 @@ oidc_providers:
# userinfo_endpoint: "https://accounts.example.com/userinfo"
# jwks_uri: "https://accounts.example.com/.well-known/jwks.json"
# skip_verification: true
# user_mapping_provider:
# config:
# subject_claim: "id"
# localpart_template: "{ user.login }"
# display_name_template: "{ user.name }"
# email_template: "{ user.email }"

# For use with Keycloak
#
Expand Down
15 changes: 12 additions & 3 deletions synapse/config/oidc_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
#
# For the default provider, the following settings are available:
#
# sub: name of the claim containing a unique identifier for the
# user. Defaults to 'sub', which OpenID Connect compliant
# providers should provide.
# subject_claim: name of the claim containing a unique identifier
Copy link
Member

Choose a reason for hiding this comment

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

This was just documented wrong it looks like?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup.

# for the user. Defaults to 'sub', which OpenID Connect
# compliant providers should provide.
#
# localpart_template: Jinja2 template for the localpart of the MXID.
# If this is not set, the user will be prompted to choose their
Expand All @@ -154,6 +154,9 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# display_name_template: Jinja2 template for the display name to set
# on first login. If unset, no displayname will be set.
#
# email_template: Jinja2 template for the email address of the user.
# If unset, no email address will be added to the account.
#
# extra_attributes: a map of Jinja2 templates for extra attributes
# to send back to the client during login.
# Note that these are non-standard and clients will ignore them
Expand Down Expand Up @@ -189,6 +192,12 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# userinfo_endpoint: "https://accounts.example.com/userinfo"
# jwks_uri: "https://accounts.example.com/.well-known/jwks.json"
# skip_verification: true
# user_mapping_provider:
# config:
# subject_claim: "id"
# localpart_template: "{{ user.login }}"
# display_name_template: "{{ user.name }}"
# email_template: "{{ user.email }}"

# For use with Keycloak
#
Expand Down
52 changes: 28 additions & 24 deletions synapse/handlers/oidc_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -1056,7 +1056,8 @@ class OidcSessionData:


UserAttributeDict = TypedDict(
"UserAttributeDict", {"localpart": Optional[str], "display_name": Optional[str]}
"UserAttributeDict",
{"localpart": Optional[str], "display_name": Optional[str], "emails": List[str]},
)
C = TypeVar("C")

Expand Down Expand Up @@ -1135,11 +1136,12 @@ def jinja_finalize(thing):
env = Environment(finalize=jinja_finalize)


@attr.s
@attr.s(slots=True, frozen=True)
class JinjaOidcMappingConfig:
subject_claim = attr.ib(type=str)
localpart_template = attr.ib(type=Optional[Template])
display_name_template = attr.ib(type=Optional[Template])
email_template = attr.ib(type=Optional[Template])
extra_attributes = attr.ib(type=Dict[str, Template])


Expand All @@ -1156,23 +1158,17 @@ def __init__(self, config: JinjaOidcMappingConfig):
def parse_config(config: dict) -> JinjaOidcMappingConfig:
subject_claim = config.get("subject_claim", "sub")

localpart_template = None # type: Optional[Template]
if "localpart_template" in config:
def parse_template_config(option_name: str) -> Optional[Template]:
if option_name not in config:
return None
try:
localpart_template = env.from_string(config["localpart_template"])
return env.from_string(config[option_name])
except Exception as e:
raise ConfigError(
"invalid jinja template", path=["localpart_template"]
) from e
raise ConfigError("invalid jinja template", path=[option_name]) from e

display_name_template = None # type: Optional[Template]
if "display_name_template" in config:
try:
display_name_template = env.from_string(config["display_name_template"])
except Exception as e:
raise ConfigError(
"invalid jinja template", path=["display_name_template"]
) from e
localpart_template = parse_template_config("localpart_template")
display_name_template = parse_template_config("display_name_template")
email_template = parse_template_config("email_template")

extra_attributes = {} # type Dict[str, Template]
if "extra_attributes" in config:
Expand All @@ -1192,6 +1188,7 @@ def parse_config(config: dict) -> JinjaOidcMappingConfig:
subject_claim=subject_claim,
localpart_template=localpart_template,
display_name_template=display_name_template,
email_template=email_template,
extra_attributes=extra_attributes,
)

Expand All @@ -1213,16 +1210,23 @@ async def map_user_attributes(
# a usable mxid.
localpart += str(failures) if failures else ""

display_name = None # type: Optional[str]
if self._config.display_name_template is not None:
display_name = self._config.display_name_template.render(
user=userinfo
).strip()
def render_template_field(template: Optional[Template]) -> Optional[str]:
if template is None:
return None
return template.render(user=userinfo).strip()

display_name = render_template_field(self._config.display_name_template)
if display_name == "":
display_name = None

if display_name == "":
display_name = None
emails = [] # type: List[str]
email = render_template_field(self._config.email_template)
if email:
emails.append(email)

return UserAttributeDict(localpart=localpart, display_name=display_name)
return UserAttributeDict(
localpart=localpart, display_name=display_name, emails=emails
)
Comment on lines +1227 to +1229
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I had wonder if we should just return synapse.handlers.sso.UserAttributes here instead of having a class that is almost identical. 🤷 I think I had mypy issues with it though and couldn't easily fix it.

No change necessary, just idly thinking. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same thought. Then I realised this was an extension module, so changing the expected return type could break existing implementations, and suddenly I lost enthusiasm...

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure that was what I did as well. 😟


async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict:
extras = {} # type: Dict[str, str]
Expand Down