Email notifications are not automatically enabled when authenticating via SSO or using `inhibit_login`

[manning-ncsa]

Description

Users that authenticate via oidc_provider do not have email notifications enabled by default, even though the server is configured correctly and they have an email address associated with their account.

The problem lies here, where access_token is set to None in post_registration_actions(). This leads to the call to _register_email_threepid() with a null access_token, which in turn causes the condition here to evaluate to false, thus preventing the email notification pusher from being added.

Steps to reproduce

• Configure Synapse to enable email notifications for new users: email.notif_for_new_users: true and email.enable_notifs: true.
• Log in a user for the first time using a custom oidc_provider (in our case Keycloak).
• Verify that they automatically have an email address associated with their account.
• Open the user settings and see that email notifications are disabled.

I expect the email notifications to be enabled upon first login.

Proposed Solution

One solution to this is to include LoginType.SSO in the auth_result object passed to the post_registration_actions() function here so that prior to the _register_email_threepid() call the access_token could be set to True if auth_result[LoginType.SSO] == True. (Disclaimer: I have not tried this to verify it is a good solution. The idea came from discussion with @kyrias.)

Version information

• Homeserver: independent self-hosted homeserver

If not matrix.org:

• Version: "server_version":"1.42.0", "python_version":"3.8.12"

• Install method: Docker image matrixdotorg/synapse:v1.42.0 (Helm chart: https://ananace.gitlab.io/charts/matrix-synapse:2.1.x)

• Platform: Kubernetes

[erikjohnston]

I think this is also true if the client registers with inhibit_login set to true. I think the solution here is to unconditionally add an email pusher even if there isn't an access token? I'm not 100% sure what the token ID is used for with email pushers though.

[manning-ncsa]

The access_token input is optional in the pusher creation function that is called, and it looks like the only reason it is included in the conditional here is so that the user_id search does not fail. However, since user_id is already available, perhaps the presence of a token can be checked inside the conditional block to obtain the user_id if a token is provided. The code in these lines would then become

        if (
            self.hs.config.email_enable_notifs
            and self.hs.config.email_notif_for_new_users
        ):
            # Pull the ID of the access token back out of the db
            # It would really make more sense for this to be passed
            # up when the access token is saved, but that's quite an
            # invasive change I'd rather do separately.
            if token:
                user_tuple = await self.store.get_user_by_access_token(token)
                # The token better still exist.
                assert user_tuple
                token_id = user_tuple.token_id
            else:
                token_id = None

[lukasdenk]

I'll try to fix the issue.

[lukasdenk]

The access_token input is optional in the pusher creation function that is called, and it looks like the only reason it is included in the conditional here is so that the user_id search does not fail. However, since user_id is already available, perhaps the presence of a token can be checked inside the conditional block to obtain the user_id if a token is provided. The code in these lines would then become

        if (
            self.hs.config.email_enable_notifs
            and self.hs.config.email_notif_for_new_users
        ):
            # Pull the ID of the access token back out of the db
            # It would really make more sense for this to be passed
            # up when the access token is saved, but that's quite an
            # invasive change I'd rather do separately.
            if token:
                user_tuple = await self.store.get_user_by_access_token(token)
                # The token better still exist.
                assert user_tuple
                token_id = user_tuple.token_id
            else:
                token_id = None

I mistakenly included these changes into a PR for another issue I was working on. Unfortunately, the proposed solution caused the issues mentioned in #11769. See also PR #11770.

So, @anoadragon453 @DMRobertson: Do you have any ideas on how to solve this issue? It seems like this might not be a good first issue anymore...

[manning-ncsa]

It seems like this might not be a good first issue anymore...

:astonished: I guess not. @lukasdenk Thanks for working on this and illuminating some underlying issues, intentional or not!

[lukasdenk]

I guess not. @lukasdenk Thanks for working on this and illuminating some underlying issues, intentional or not!

You're welcome :)