Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GDPR consent to registration flow to unbreak ILAG #7168

Closed
lampholder opened this issue Aug 14, 2018 · 8 comments
Closed

Add GDPR consent to registration flow to unbreak ILAG #7168

lampholder opened this issue Aug 14, 2018 · 8 comments

Comments

@lampholder
Copy link
Member

Riot web issue for: element-hq/riot-meta#196

@dbkr
Copy link
Member

dbkr commented Sep 10, 2018

I've had a quick look at this to see how complex it is. Basically it involves:

Spec:

  • Speccing m.login.terms

Synapse:

  • Adding m.login.terms as a UI auth flow
  • Adding a fallback auth resource for it. This would essentially be the fallback auth resource deferring to the current consent resource, but with a UI auth session instead of a user ID, and calling add_oob_auth when the form is submitted.
  • When the registration is completed, check for a completed m.login.terms flow and if so, mark the newly created user as having consented.

Riot / react-sdk:

  • Support the m.login.terms flow

We also have the choice to either have clients that don't support the new flow to use fallback auth, or continue to get a DM from the server notices bot as they currently do.

Here's a patch to synapse that illustrates roughly how one would go about adding a fallback auth resource to synapse:

diff --git a/synapse/api/constants.py b/synapse/api/constants.py
index c2630c4c6..b2815da0a 100644
--- a/synapse/api/constants.py
+++ b/synapse/api/constants.py
@@ -51,6 +51,7 @@ class LoginType(object):
     EMAIL_IDENTITY = u"m.login.email.identity"
     MSISDN = u"m.login.msisdn"
     RECAPTCHA = u"m.login.recaptcha"
+    TERMS = u"m.login.terms"
     DUMMY = u"m.login.dummy"
 
     # Only for C/S API v1
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 2a5eab124..f08a2cdd7 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -59,6 +59,7 @@ class AuthHandler(BaseHandler):
             LoginType.EMAIL_IDENTITY: self._check_email_identity,
             LoginType.MSISDN: self._check_msisdn,
             LoginType.DUMMY: self._check_dummy_auth,
+            LoginType.TERMS: self._check_terms_auth,
         }
         self.bcrypt_rounds = hs.config.bcrypt_rounds
 
@@ -431,6 +432,9 @@ class AuthHandler(BaseHandler):
     def _check_dummy_auth(self, authdict, _):
         return defer.succeed(True)
 
+    def _check_terms_auth(self, authdict, _):
+        return defer.succeed(True)
+
     @defer.inlineCallbacks
     def _check_threepid(self, medium, authdict):
         if 'threepid_creds' not in authdict:
diff --git a/synapse/rest/client/v2_alpha/auth.py b/synapse/rest/client/v2_alpha/auth.py
index bd8b5f4af..bc3bfee4a 100644
--- a/synapse/rest/client/v2_alpha/auth.py
+++ b/synapse/rest/client/v2_alpha/auth.py
@@ -130,6 +130,26 @@ class AuthRestServlet(RestServlet):
             request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
             request.setHeader(b"Content-Length", b"%d" % (len(html_bytes),))
 
+            request.write(html_bytes)
+            finish_request(request)
+            defer.returnValue(None)
+        elif stagetype == LoginType.TERMS:
+            session = request.args['session'][0]
+            authdict = {
+                'session': session,
+            }
+            success = yield self.auth_handler.add_oob_auth(
+                LoginType.TERMS,
+                authdict,
+                self.hs.get_ip_from_request(request)
+            )
+
+            html = "<html><body>hai</body></html>"
+            html_bytes = html.encode("utf8")
+            request.setResponseCode(200)
+            request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
+            request.setHeader(b"Content-Length", b"%d" % (len(html_bytes),))
+
             request.write(html_bytes)
             finish_request(request)
             defer.returnValue(None)
diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py
index 192f52e46..dedf5269e 100644
--- a/synapse/rest/client/v2_alpha/register.py
+++ b/synapse/rest/client/v2_alpha/register.py
@@ -359,6 +359,21 @@ class RegisterRestServlet(RestServlet):
                     [LoginType.MSISDN, LoginType.EMAIL_IDENTITY]
                 ])
 
+        if self.hs.config.block_events_without_consent_error is not None:
+            new_flows = []
+            for flow in flows:
+                # To only allow registration if completing GDPR auth,
+                # making clients that don't support it use fallback auth.
+                #flow.append(LoginType.TERMS)
+
+                # or to duplicate all the flows above with the GDPR flow on the
+                # end so clients that support it can use it but clients that don't
+                # continue to consent via the DM from server notices bot.
+                new_flows.extend([
+                    flow + [LoginType.TERMS]
+                ])
+            flows.extend(new_flows)
+
         auth_result, params, session_id = yield self.auth_handler.check_auth(
             flows, body, self.hs.get_ip_from_request(request)
         )

@dbkr dbkr removed their assignment Sep 10, 2018
@ara4n
Copy link
Member

ara4n commented Sep 11, 2018

Yet another reason to want this: the current flow completely interrupts you as you try to join a room at first and dumps you on the /home page :|

@ara4n ara4n added T-Defect P1 and removed P2 labels Sep 12, 2018
@ara4n
Copy link
Member

ara4n commented Sep 12, 2018

bumping this up in priority given it currently completely breaks the onboarding experience if you follow a link like https://riot.im/#/room/#/develop without an account, given when you create your account it promptly GDPRs you and then forgets where you were going.

@ara4n ara4n added A-Registration and removed T-Task Tasks for the team like planning labels Sep 12, 2018
@ara4n ara4n changed the title Add GDPR consent to registration flow. Add GDPR consent to registration flow to unbreak ILAG Sep 12, 2018
@ara4n
Copy link
Member

ara4n commented Sep 12, 2018

hang on, though - @dbkr, would fixing this actually fix ILAG? given ILAG doesn't currently go anywhere near UI auth? :/

@dbkr
Copy link
Member

dbkr commented Sep 14, 2018

ILAG does UI auth - it's how it does the ReCAPTCHA, although now we've made it force you through normal registration if it sees any unrecognised flows, it will disable ILAG until we change Riot to know about it in at least some way.

@turt2live
Copy link
Member

After discussion with the synapse team it was deemed that a more supportive API should be implemented to handle changes in documents, etc. This is now a proposal at matrix-org/matrix-spec-proposals#1692

As mentioned in the proposal's description, Riot is going to support the registration side of the proposal to fix this issue right away. The remainder of the proposal would be implemented once approved.

@turt2live
Copy link
Member

Just an update: The Riot side of this is done, however the synapse side is incomplete. Leaving this open to track the synapse side.

Synapse PR: matrix-org/synapse#4004

@turt2live
Copy link
Member

matrix-org/synapse#4004 landed 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants