From 49a8d7d70ca3146895556925840e682f02038c93 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Sun, 2 Jun 2019 10:52:44 +0300 Subject: [PATCH 1/5] remove legacy session identifier support --- redash/authentication/__init__.py | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/redash/authentication/__init__.py b/redash/authentication/__init__.py index eeb9a1d7c6..c4cb42ed48 100644 --- a/redash/authentication/__init__.py +++ b/redash/authentication/__init__.py @@ -43,30 +43,14 @@ def sign(key, path, expires): def load_user(user_id_with_identity): org = current_org._get_current_object() - ''' - Users who logged in prior to https://github.com/getredash/redash/pull/3174 going live are going - to have their (integer) user_id as their session user identifier. - These session user identifiers will be updated the first time they visit any page so we add special - logic to allow a frictionless transition. - This logic will be removed 2-4 weeks after going live, and users who haven't - visited any page during that time will simply have to log in again. - ''' - - is_legacy_session_identifier = str(user_id_with_identity).find('-') < 0 - - if is_legacy_session_identifier: - user_id = user_id_with_identity - else: - user_id, _ = user_id_with_identity.split("-") + user_id, _ = user_id_with_identity.split("-") try: user = models.User.get_by_id_and_org(user_id, org) if user.is_disabled: return None - if is_legacy_session_identifier: - login_user(user, remember=True) - elif user.get_id() != user_id_with_identity: + if user.get_id() != user_id_with_identity: return None return user From fb39eea184573e24b77a4ffd834b5edfee777776 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Sun, 2 Jun 2019 10:56:48 +0300 Subject: [PATCH 2/5] remove redundant test --- redash/authentication/__init__.py | 5 +---- tests/test_handlers.py | 8 -------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/redash/authentication/__init__.py b/redash/authentication/__init__.py index c4cb42ed48..86a3631045 100644 --- a/redash/authentication/__init__.py +++ b/redash/authentication/__init__.py @@ -47,10 +47,7 @@ def load_user(user_id_with_identity): try: user = models.User.get_by_id_and_org(user_id, org) - if user.is_disabled: - return None - - if user.get_id() != user_id_with_identity: + if user.is_disabled or user.get_id() != user_id_with_identity: return None return user diff --git a/tests/test_handlers.py b/tests/test_handlers.py index 04d93a55cb..68f443ab9c 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -27,14 +27,6 @@ def test_responds_with_success_for_signed_in_user(self): self.assertEquals(200, rv.status_code) - def test_responds_with_success_for_signed_in_user_with_a_legacy_identity_session(self): - with self.client as c: - with c.session_transaction() as sess: - sess['user_id'] = str(self.factory.user.id) - rv = self.client.get("/default/") - - self.assertEquals(200, rv.status_code) - def test_redirects_for_nonsigned_in_user(self): rv = self.client.get("/default/") self.assertEquals(302, rv.status_code) From 7794b201a3546e058074ac445116e8f7cb1369d1 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Sun, 2 Jun 2019 11:11:05 +0300 Subject: [PATCH 3/5] redirect to login to support any invalid session identifiers --- redash/authentication/__init__.py | 5 ++--- tests/test_handlers.py | 8 ++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/redash/authentication/__init__.py b/redash/authentication/__init__.py index 86a3631045..600d81b2a7 100644 --- a/redash/authentication/__init__.py +++ b/redash/authentication/__init__.py @@ -43,15 +43,14 @@ def sign(key, path, expires): def load_user(user_id_with_identity): org = current_org._get_current_object() - user_id, _ = user_id_with_identity.split("-") - try: + user_id, _ = user_id_with_identity.split("-") user = models.User.get_by_id_and_org(user_id, org) if user.is_disabled or user.get_id() != user_id_with_identity: return None return user - except models.NoResultFound: + except (models.NoResultFound, Exception): return None diff --git a/tests/test_handlers.py b/tests/test_handlers.py index 68f443ab9c..e9f835becc 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -30,6 +30,14 @@ def test_responds_with_success_for_signed_in_user(self): def test_redirects_for_nonsigned_in_user(self): rv = self.client.get("/default/") self.assertEquals(302, rv.status_code) + + def test_redirects_for_invalid_session_identifier(self): + with self.client as c: + with c.session_transaction() as sess: + sess['user_id'] = 100 + rv = self.client.get("/default/") + + self.assertEquals(302, rv.status_code) class PingTest(BaseTestCase): From db1ef8476abefaf9084ae6c0e39a730ffd1168cc Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Mon, 3 Jun 2019 13:06:08 +0300 Subject: [PATCH 4/5] be more specific with caught errors --- redash/authentication/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redash/authentication/__init__.py b/redash/authentication/__init__.py index 600d81b2a7..04af7a29ac 100644 --- a/redash/authentication/__init__.py +++ b/redash/authentication/__init__.py @@ -50,7 +50,7 @@ def load_user(user_id_with_identity): return None return user - except (models.NoResultFound, Exception): + except (models.NoResultFound, ValueError, AttributeError): return None From 52b4fc576fe2d8af7670d8a2e5f59f602a36573b Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 4 Jun 2019 13:02:45 +0300 Subject: [PATCH 5/5] use authorization according to api_key (if provided) over session --- redash/authentication/__init__.py | 4 ++++ tests/test_authentication.py | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/redash/authentication/__init__.py b/redash/authentication/__init__.py index 04af7a29ac..989ed52b11 100644 --- a/redash/authentication/__init__.py +++ b/redash/authentication/__init__.py @@ -41,6 +41,10 @@ def sign(key, path, expires): @login_manager.user_loader def load_user(user_id_with_identity): + user = api_key_load_user_from_request(request) + if user: + return user + org = current_org._get_current_object() try: diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 3cf1d8b1f2..192bb53316 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -127,6 +127,19 @@ def test_user_api_key(self): self.assertEqual(user.id, hmac_load_user_from_request(request).id) +class TestSessionAuthentication(BaseTestCase): + def test_prefers_api_key_over_session_user_id(self): + user = self.factory.create_user() + query = self.factory.create_query(user=user) + + other_org = self.factory.create_org() + other_user = self.factory.create_user(org=other_org) + models.db.session.flush() + + rv = self.make_request('get', '/api/queries/{}?api_key={}'.format(query.id, query.api_key), user=other_user) + self.assertEqual(rv.status_code, 200) + + class TestCreateAndLoginUser(BaseTestCase): def test_logins_valid_user(self): user = self.factory.create_user(email=u'test@example.com')