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

Make importing display name and email optional #9277

Merged
merged 2 commits into from
Feb 1, 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/9277.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve the user experience of setting up an account via single-sign on.
5 changes: 3 additions & 2 deletions synapse/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
# limitations under the License.

"""Contains functions for registering clients."""

import logging
from typing import TYPE_CHECKING, List, Optional, Tuple
from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple

from synapse import types
from synapse.api.constants import MAX_USERID_LENGTH, EventTypes, JoinRules, LoginType
Expand Down Expand Up @@ -152,7 +153,7 @@ async def register_user(
user_type: Optional[str] = None,
default_display_name: Optional[str] = None,
address: Optional[str] = None,
bind_emails: List[str] = [],
bind_emails: Iterable[str] = [],
by_admin: bool = False,
user_agent_ips: Optional[List[Tuple[str, str]]] = None,
) -> str:
Expand Down
52 changes: 44 additions & 8 deletions synapse/handlers/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,16 @@
# limitations under the License.
import abc
import logging
from typing import TYPE_CHECKING, Awaitable, Callable, Dict, List, Mapping, Optional
from typing import (
TYPE_CHECKING,
Awaitable,
Callable,
Dict,
Iterable,
Mapping,
Optional,
Set,
)
from urllib.parse import urlencode

import attr
Expand All @@ -29,7 +38,7 @@
from synapse.http import get_request_user_agent
from synapse.http.server import respond_with_html, respond_with_redirect
from synapse.http.site import SynapseRequest
from synapse.types import JsonDict, UserID, contains_invalid_mxid_characters
from synapse.types import Collection, JsonDict, UserID, contains_invalid_mxid_characters
from synapse.util.async_helpers import Linearizer
from synapse.util.stringutils import random_string

Expand Down Expand Up @@ -115,7 +124,7 @@ class UserAttributes:
# enter one.
localpart = attr.ib(type=Optional[str])
display_name = attr.ib(type=Optional[str], default=None)
emails = attr.ib(type=List[str], default=attr.Factory(list))
emails = attr.ib(type=Collection[str], default=attr.Factory(list))


@attr.s(slots=True)
Expand All @@ -130,7 +139,7 @@ class UsernameMappingSession:

# attributes returned by the ID mapper
display_name = attr.ib(type=Optional[str])
emails = attr.ib(type=List[str])
emails = attr.ib(type=Collection[str])

# An optional dictionary of extra attributes to be provided to the client in the
# login response.
Expand All @@ -144,6 +153,8 @@ class UsernameMappingSession:

# choices made by the user
chosen_localpart = attr.ib(type=Optional[str], default=None)
use_display_name = attr.ib(type=bool, default=True)
emails_to_use = attr.ib(type=Collection[str], default=())


# the HTTP cookie used to track the mapping session id
Expand Down Expand Up @@ -710,7 +721,12 @@ async def check_username_availability(
return not user_infos

async def handle_submit_username_request(
self, request: SynapseRequest, localpart: str, session_id: str
self,
request: SynapseRequest,
session_id: str,
localpart: str,
use_display_name: bool,
emails_to_use: Iterable[str],
) -> None:
"""Handle a request to the username-picker 'submit' endpoint

Expand All @@ -720,11 +736,30 @@ async def handle_submit_username_request(
request: HTTP request
localpart: localpart requested by the user
session_id: ID of the username mapping session, extracted from a cookie
use_display_name: whether the user wants to use the suggested display name
emails_to_use: emails that the user would like to use
"""
session = self.get_mapping_session(session_id)

# update the session with the user's choices
session.chosen_localpart = localpart
session.use_display_name = use_display_name

emails_from_idp = set(session.emails)
filtered_emails = set() # type: Set[str]

# we iterate through the list rather than just building a set conjunction, so
# that we can log attempts to use unknown addresses
for email in emails_to_use:
if email in emails_from_idp:
filtered_emails.add(email)
else:
logger.warning(
"[session %s] ignoring user request to use unknown email address %r",
session_id,
email,
)
session.emails_to_use = filtered_emails

# we're done; now we can register the user
respond_with_redirect(request, b"/_synapse/client/sso_register")
Expand All @@ -747,11 +782,12 @@ async def register_sso_user(self, request: Request, session_id: str) -> None:
)

attributes = UserAttributes(
localpart=session.chosen_localpart,
display_name=session.display_name,
emails=session.emails,
localpart=session.chosen_localpart, emails=session.emails_to_use,
)

if session.use_display_name:
attributes.display_name = session.display_name

# the following will raise a 400 error if the username has been taken in the
# meantime.
user_id = await self._register_mapped_user(
Expand Down
23 changes: 23 additions & 0 deletions synapse/res/templates/sso_auth_account_details.html
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@
border-top: 1px solid #E9ECF1;
padding: 12px;
}
.idp-pick-details .check-row {
display: flex;
align-items: center;
}

.idp-pick-details .check-row .name {
flex: 1;
}

.idp-pick-details .use, .idp-pick-details .idp-value {
color: #737D8C;
Expand Down Expand Up @@ -91,16 +99,31 @@ <h1>Your account is nearly ready</h1>
<h2><img src="{{ idp.idp_icon | mxc_to_http(24, 24) }}"/>Information from {{ idp.idp_name }}</h2>
{% if user_attributes.avatar_url %}
<div class="idp-detail idp-avatar">
<div class="check-row">
<label for="idp-avatar" class="name">Avatar</label>
<label for="idp-avatar" class="use">Use</label>
<input type="checkbox" name="use_avatar" id="idp-avatar" value="true" checked>
</div>
<img src="{{ user_attributes.avatar_url }}" class="avatar" />
</div>
{% endif %}
{% if user_attributes.display_name %}
<div class="idp-detail">
<div class="check-row">
<label for="idp-displayname" class="name">Display name</label>
<label for="idp-displayname" class="use">Use</label>
<input type="checkbox" name="use_display_name" id="idp-displayname" value="true" checked>
</div>
<p class="idp-value">{{ user_attributes.display_name }}</p>
</div>
{% endif %}
{% for email in user_attributes.emails %}
<div class="idp-detail">
<div class="check-row">
<label for="idp-email{{ loop.index }}" class="name">E-mail</label>
<label for="idp-email{{ loop.index }}" class="use">Use</label>
<input type="checkbox" name="use_email" id="idp-email{{ loop.index }}" value="{{ email }}" checked>
</div>
<p class="idp-value">{{ email }}</p>
</div>
{% endfor %}
Expand Down
14 changes: 11 additions & 3 deletions synapse/rest/synapse/client/pick_username.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# limitations under the License.

import logging
from typing import TYPE_CHECKING
from typing import TYPE_CHECKING, List

from twisted.web.http import Request
from twisted.web.resource import Resource
Expand All @@ -26,7 +26,7 @@
DirectServeJsonResource,
respond_with_html,
)
from synapse.http.servlet import parse_string
from synapse.http.servlet import parse_boolean, parse_string
from synapse.http.site import SynapseRequest
from synapse.util.templates import build_jinja_env

Expand Down Expand Up @@ -113,11 +113,19 @@ async def _async_render_POST(self, request: SynapseRequest):

try:
localpart = parse_string(request, "username", required=True)
use_display_name = parse_boolean(request, "use_display_name", default=False)

try:
emails_to_use = [
val.decode("utf-8") for val in request.args.get(b"use_email", [])
] # type: List[str]
Comment on lines +119 to +121
Copy link
Member

Choose a reason for hiding this comment

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

Does this list need to be checked against the list that was given to the user? (Right now it seems like it could be used to inject any email?)

Copy link
Member Author

Choose a reason for hiding this comment

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

that is (hopefully) checked in the handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK! Just done in a different layer than expected!

Copy link
Member

Choose a reason for hiding this comment

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

Arg, #6739 adds a parse_list_from_args which would be useful here.

Copy link
Member Author

Choose a reason for hiding this comment

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

that feels a bit overengineered, if I'm honest.

except ValueError:
raise SynapseError(400, "Query parameter use_email must be utf-8")
except SynapseError as e:
logger.warning("[session %s] bad param: %s", session_id, e)
self._sso_handler.render_error(request, "bad_param", e.msg, code=e.code)
return

await self._sso_handler.handle_submit_username_request(
request, localpart, session_id
request, session_id, localpart, use_display_name, emails_to_use
)