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

Improve SAML error messages #8248

Merged
merged 12 commits into from
Sep 14, 2020
Merged

Improve SAML error messages #8248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c34eca9
Separate the SAML2 mapping code from the response parsing code.
clokep Sep 3, 2020
5171d1f
Render errors for issues handling the SAML response.
clokep Sep 3, 2020
a11c959
Render errors for issues mapping to an MXID.
clokep Sep 3, 2020
0b3f8aa
Use the SAML error page for all SSO errors.
clokep Sep 3, 2020
f6fb379
Stop using the SAML error page.
clokep Sep 3, 2020
01978a2
Add changelog file.
clokep Sep 3, 2020
0b51a20
Review feedback.
clokep Sep 9, 2020
15dfc46
Merge branch 'develop' into clokep/better-saml-errs
clokep Sep 9, 2020
eb6be88
Add upgrade notes.
clokep Sep 10, 2020
ba9c7db
Merge remote-tracking branch 'origin/develop' into clokep/better-saml…
clokep Sep 11, 2020
3a9e289
Move upgrade notes.
clokep Sep 11, 2020
d097dba
Review feedback.
clokep Sep 14, 2020
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
30 changes: 4 additions & 26 deletions docs/sample_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1467,11 +1467,14 @@ trusted_key_servers:
# At least one of `sp_config` or `config_path` must be set in this section to
# enable SAML login.
#
# (You will probably also want to set the following options to `false` to
# You will probably also want to set the following options to `false` to
# disable the regular login/registration flows:
# * enable_registration
# * password_config.enabled
#
# You will also want to investigate the settings under the sso configuration
# section below.
#
# Once SAML support is enabled, a metadata file will be exposed at
# https://<server>:<port>/_matrix/saml2/metadata.xml, which you may be able to
# use to configure your SAML IdP with. Alternatively, you can manually configure
Expand Down Expand Up @@ -1594,31 +1597,6 @@ saml2_config:
# - attribute: department
# value: "sales"

# Directory in which Synapse will try to find the template files below.
# If not set, default templates from within the Synapse package will be used.
#
# DO NOT UNCOMMENT THIS SETTING unless you want to customise the templates.
# If you *do* uncomment it, you will need to make sure that all the templates
# below are in the directory.
#
# Synapse will look for the following templates in this directory:
#
# * HTML page to display to users if something goes wrong during the
# authentication process: 'saml_error.html'.
#
# When rendering, this template is given the following variables:
# * code: an HTML error code corresponding to the error that is being
# returned (typically 400 or 500)
#
# * msg: a textual message describing the error.
#
# The variables will automatically be HTML-escaped.
#
# You can see the default templates at:
# https://github.com/matrix-org/synapse/tree/master/synapse/res/templates
#
#template_dir: "res/templates"


# OpenID Connect integration. The following settings can be used to make Synapse
# use an OpenID Connect Provider for authentication, instead of its internal
Expand Down
34 changes: 4 additions & 30 deletions synapse/config/saml2_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,6 @@ def read_config(self, config, **kwargs):
saml2_config.get("saml_session_lifetime", "15m")
)

self.saml2_error_html_template = self.read_templates(
["saml_error.html"], saml2_config.get("template_dir")
)[0]

def _default_saml_config_dict(
self, required_attributes: set, optional_attributes: set
):
Expand Down Expand Up @@ -225,11 +221,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# At least one of `sp_config` or `config_path` must be set in this section to
# enable SAML login.
#
# (You will probably also want to set the following options to `false` to
# You will probably also want to set the following options to `false` to
# disable the regular login/registration flows:
# * enable_registration
# * password_config.enabled
#
# You will also want to investigate the settings under the sso configuration
clokep marked this conversation as resolved.
Show resolved Hide resolved
# section below.
#
# Once SAML support is enabled, a metadata file will be exposed at
# https://<server>:<port>/_matrix/saml2/metadata.xml, which you may be able to
# use to configure your SAML IdP with. Alternatively, you can manually configure
Expand Down Expand Up @@ -351,31 +350,6 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
# value: "staff"
# - attribute: department
# value: "sales"

# Directory in which Synapse will try to find the template files below.
# If not set, default templates from within the Synapse package will be used.
#
# DO NOT UNCOMMENT THIS SETTING unless you want to customise the templates.
# If you *do* uncomment it, you will need to make sure that all the templates
# below are in the directory.
#
# Synapse will look for the following templates in this directory:
#
# * HTML page to display to users if something goes wrong during the
# authentication process: 'saml_error.html'.
#
# When rendering, this template is given the following variables:
# * code: an HTML error code corresponding to the error that is being
# returned (typically 400 or 500)
#
# * msg: a textual message describing the error.
#
# The variables will automatically be HTML-escaped.
#
# You can see the default templates at:
# https://github.com/matrix-org/synapse/tree/master/synapse/res/templates
#
#template_dir: "res/templates"
""" % {
"config_dir_path": config_dir_path
}
Expand Down
4 changes: 3 additions & 1 deletion synapse/handlers/saml_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
saml2_auth.in_response_to, None
)

# Ensure that the attributes of the logged in user meet the required
# attributes.
for requirement in self._saml2_attribute_requirements:
if not _check_attribute_requirement(saml2_auth.ava, requirement):
self._render_error(
Expand All @@ -222,7 +224,7 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
# Call the mapper to register/login the user
try:
user_id = await self._map_saml_response_to_user(
resp_bytes, relay_state, user_agent, ip_address
saml2_auth, relay_state, user_agent, ip_address
)
except MappingException as e:
logger.exception("Could not map user")
Expand Down
52 changes: 0 additions & 52 deletions synapse/res/templates/saml_error.html
View file
Open in desktop

This file was deleted.

16 changes: 4 additions & 12 deletions synapse/rest/saml2/response_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from twisted.python import failure

from synapse.api.errors import SynapseError
from synapse.http.server import DirectServeHtmlResource, return_html_error
from synapse.http.server import DirectServeHtmlResource


class SAML2ResponseResource(DirectServeHtmlResource):
Expand All @@ -27,21 +25,15 @@ class SAML2ResponseResource(DirectServeHtmlResource):
def __init__(self, hs):
super().__init__()
self._saml_handler = hs.get_saml_handler()
self._error_html_template = hs.config.saml2.saml2_error_html_template

async def _async_render_GET(self, request):
# We're not expecting any GET request on that resource if everything goes right,
# but some IdPs sometimes end up responding with a 302 redirect on this endpoint.
# In this case, just tell the user that something went wrong and they should
# try to authenticate again.
f = failure.Failure(
SynapseError(400, "Unexpected GET request on /saml2/authn_response")
self._saml_handler._render_error(
Copy link
Member

Choose a reason for hiding this comment

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

Should we make render_error public, or since it's still used in synapse.rest.saml2 we consider it private?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was that since it is still used in the saml code it could be private so that random users of the SamlHandler don't use it?

Copy link
Member

Choose a reason for hiding this comment

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

Fair yeah, I agree that the one exception here outweighs the issue of it being used accidentally elsewhere.

request, "unexpected_get", "Unexpected GET request on /saml2/authn_response"
)
return_html_error(f, request, self._error_html_template)

async def _async_render_POST(self, request):
try:
await self._saml_handler.handle_saml_response(request)
except Exception:
f = failure.Failure()
return_html_error(f, request, self._error_html_template)
await self._saml_handler.handle_saml_response(request)