diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index d6c90f0fae..0921398597 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -2,6 +2,7 @@ import os import io +from base64 import urlsafe_b64encode from datetime import datetime from flask import (Blueprint, render_template, flash, redirect, url_for, g, session, current_app, request, Markup, abort) @@ -37,9 +38,17 @@ def generate(): return redirect(url_for('.lookup')) codename = generate_unique_codename(config) - session['codename'] = codename + + # Generate a unique id for each browser tab and associate the codename with this id. + # This will allow retrieval of the codename displayed in the tab from which the source has + # clicked to proceed to /generate (ref. issue #4458) + tab_id = urlsafe_b64encode(os.urandom(64)).decode() + codenames = session.get('codenames', {}) + codenames[tab_id] = codename + session['codenames'] = codenames + session['new_user'] = True - return render_template('generate.html', codename=codename) + return render_template('generate.html', codename=codename, tab_id=tab_id) @view.route('/org-logo') def select_logo(): @@ -51,33 +60,43 @@ def select_logo(): @view.route('/create', methods=['POST']) def create(): - filesystem_id = current_app.crypto_util.hash_codename( - session['codename']) - - source = Source(filesystem_id, current_app.crypto_util.display_id()) - db.session.add(source) - try: - db.session.commit() - except IntegrityError as e: - db.session.rollback() - current_app.logger.error( - "Attempt to create a source with duplicate codename: %s" % - (e,)) - - # Issue 2386: don't log in on duplicates - del session['codename'] - - # Issue 4361: Delete 'logged_in' if it's in the session - try: - del session['logged_in'] - except KeyError: - pass - - abort(500) + if session.get('logged_in', False): + flash(gettext("You are already logged in. Please verify your codename below as it " + + "may differ from the one displayed on the previous page."), + 'notification') else: - os.mkdir(current_app.storage.path(filesystem_id)) + tab_id = request.form['tab_id'] + codename = session['codenames'][tab_id] + session['codename'] = codename + + del session['codenames'] + + filesystem_id = current_app.crypto_util.hash_codename(codename) + + source = Source(filesystem_id, current_app.crypto_util.display_id()) + db.session.add(source) + try: + db.session.commit() + except IntegrityError as e: + db.session.rollback() + current_app.logger.error( + "Attempt to create a source with duplicate codename: %s" % + (e,)) + + # Issue 2386: don't log in on duplicates + del session['codename'] + + # Issue 4361: Delete 'logged_in' if it's in the session + try: + del session['logged_in'] + except KeyError: + pass + + abort(500) + else: + os.mkdir(current_app.storage.path(filesystem_id)) - session['logged_in'] = True + session['logged_in'] = True return redirect(url_for('.lookup')) @view.route('/lookup', methods=('GET',)) diff --git a/securedrop/source_templates/generate.html b/securedrop/source_templates/generate.html index 51136ad8b6..4be2171061 100644 --- a/securedrop/source_templates/generate.html +++ b/securedrop/source_templates/generate.html @@ -38,6 +38,7 @@

{{ gettext('Welcome') }}

+ diff --git a/securedrop/tests/functional/source_navigation_steps.py b/securedrop/tests/functional/source_navigation_steps.py index edc94c65cc..3778c30dc0 100644 --- a/securedrop/tests/functional/source_navigation_steps.py +++ b/securedrop/tests/functional/source_navigation_steps.py @@ -40,6 +40,9 @@ def _source_clicks_submit_documents_on_homepage(self): # a diceware codename they can use for subsequent logins assert self._is_on_generate_page() + def _source_regenerates_codename(self): + self.safe_click_by_id("regenerate-submit") + def _source_chooses_to_submit_documents(self): self._source_clicks_submit_documents_on_homepage() @@ -48,7 +51,7 @@ def _source_chooses_to_submit_documents(self): assert len(codename.text) > 0 self.source_name = codename.text - def _source_shows_codename(self): + def _source_shows_codename(self, verify_source_name=True): content = self.driver.find_element_by_id("codename-hint-content") assert not content.is_displayed() @@ -57,7 +60,8 @@ def _source_shows_codename(self): self.wait_for(lambda: content.is_displayed()) assert content.is_displayed() content_content = self.driver.find_element_by_css_selector("#codename-hint-content p") - assert content_content.text == self.source_name + if verify_source_name: + assert content_content.text == self.source_name def _source_hides_codename(self): content = self.driver.find_element_by_id("codename-hint-content") @@ -223,3 +227,17 @@ def _source_sees_document_attachment_item(self): def _source_does_not_sees_document_attachment_item(self): with pytest.raises(NoSuchElementException): self.driver.find_element_by_class_name("attachment") + + def _source_sees_already_logged_in_in_other_tab_message(self): + notification = self.driver.find_element_by_css_selector(".notification") + + if not hasattr(self, "accepted_languages"): + expected_text = "You are already logged in." + assert expected_text in notification.text + + def _source_sees_redirect_already_logged_in_message(self): + notification = self.driver.find_element_by_css_selector(".notification") + + if not hasattr(self, "accepted_languages"): + expected_text = "You were redirected because you are already logged in." + assert expected_text in notification.text diff --git a/securedrop/tests/functional/test_source.py b/securedrop/tests/functional/test_source.py index f6f734d006..8063d12a5e 100644 --- a/securedrop/tests/functional/test_source.py +++ b/securedrop/tests/functional/test_source.py @@ -29,3 +29,106 @@ def test_journalist_key_from_source_interface(self): data = data.decode('utf-8') assert "BEGIN PGP PUBLIC KEY BLOCK" in data + + +class TestDuplicateSourceInterface( + functional_test.FunctionalTest, + source_navigation_steps.SourceNavigationStepsMixin): + + def get_codename_generate(self): + return self.driver.find_element_by_css_selector("#codename").text + + def get_codename_lookup(self): + return self.driver.find_element_by_css_selector("#codename-hint-content p").text + + def test_duplicate_generate_pages(self): + # Test generation of multiple codenames in different browser tabs, ref. issue 4458. + + # Generate a codename in Tab A + assert len(self.driver.window_handles) == 1 + tab_a = self.driver.current_window_handle + self._source_visits_source_homepage() + self._source_chooses_to_submit_documents() + codename_a = self.get_codename_generate() + + # Generate a different codename in Tab B + self.driver.execute_script("window.open()") + tab_b = self.driver.window_handles[1] + self.driver.switch_to.window(tab_b) + assert self.driver.current_window_handle == tab_b + self._source_visits_source_homepage() + self._source_chooses_to_submit_documents() + codename_b = self.get_codename_generate() + + assert tab_a != tab_b + assert codename_a != codename_b + + # Proceed to submit documents in Tab A + self.driver.switch_to.window(tab_a) + assert self.driver.current_window_handle == tab_a + self._source_continues_to_submit_page() + assert self._is_on_lookup_page() + self._source_shows_codename(verify_source_name=False) + codename_lookup_a = self.get_codename_lookup() + assert codename_lookup_a == codename_a + self._source_submits_a_message() + + # Proceed to submit documents in Tab B + self.driver.switch_to.window(tab_b) + assert self.driver.current_window_handle == tab_b + self._source_continues_to_submit_page() + assert self._is_on_lookup_page() + self._source_sees_already_logged_in_in_other_tab_message() + self._source_shows_codename(verify_source_name=False) + codename_lookup_b = self.get_codename_lookup() + # We expect the codename to be the one from Tab A + assert codename_lookup_b == codename_a + self._source_submits_a_message() + + def test_duplicate_generate_pages_with_refresh(self): + # Test generation of multiple codenames in different browser tabs, including behavior + # of refreshing the codemae in each tab. Ref. issue 4458. + + # Generate a codename in Tab A + assert len(self.driver.window_handles) == 1 + tab_a = self.driver.current_window_handle + self._source_visits_source_homepage() + self._source_chooses_to_submit_documents() + codename_a1 = self.get_codename_generate() + # Regenerate codename in Tab A + self._source_regenerates_codename() + codename_a2 = self.get_codename_generate() + assert codename_a1 != codename_a2 + + # Generate a different codename in Tab B + self.driver.execute_script("window.open()") + tab_b = self.driver.window_handles[1] + self.driver.switch_to.window(tab_b) + assert self.driver.current_window_handle == tab_b + self._source_visits_source_homepage() + self._source_chooses_to_submit_documents() + codename_b = self.get_codename_generate() + assert codename_b != codename_a1 != codename_a2 + + # Proceed to submit documents in Tab A + self.driver.switch_to.window(tab_a) + assert self.driver.current_window_handle == tab_a + self._source_continues_to_submit_page() + assert self._is_on_lookup_page() + self._source_shows_codename(verify_source_name=False) + codename_lookup_a = self.get_codename_lookup() + assert codename_lookup_a == codename_a2 + self._source_submits_a_message() + + # Regenerate codename in Tab B + self.driver.switch_to.window(tab_b) + assert self.driver.current_window_handle == tab_b + self._source_regenerates_codename() + # We expect the source to be directed to /lookup with a flash message + assert self._is_on_lookup_page() + self._source_sees_redirect_already_logged_in_message() + # Check codename + self._source_shows_codename(verify_source_name=False) + codename_lookup_b = self.get_codename_lookup() + assert codename_lookup_b == codename_a2 + self._source_submits_a_message() diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index cb2fb0e944..971a1ac4bf 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -56,7 +56,8 @@ def test_submit_message(source_app, journalist_app, test_journo): with source_app.test_client() as app: app.get('/generate') - app.post('/create', follow_redirects=True) + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}, follow_redirects=True) filesystem_id = g.filesystem_id # redirected to submission form resp = app.post('/submit', data=dict( @@ -153,7 +154,8 @@ def test_submit_file(source_app, journalist_app, test_journo): with source_app.test_client() as app: app.get('/generate') - app.post('/create', follow_redirects=True) + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}, follow_redirects=True) filesystem_id = g.filesystem_id # redirected to submission form resp = app.post('/submit', data=dict( @@ -254,7 +256,8 @@ def _helper_test_reply(journalist_app, source_app, config, test_journo, with source_app.test_client() as app: app.get('/generate') - app.post('/create', follow_redirects=True) + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}, follow_redirects=True) codename = session['codename'] filesystem_id = g.filesystem_id # redirected to submission form @@ -474,7 +477,8 @@ def test_delete_collection(mocker, source_app, journalist_app, test_journo): # first, add a source with source_app.test_client() as app: app.get('/generate') - app.post('/create') + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}) resp = app.post('/submit', data=dict( msg="This is a test.", fh=(BytesIO(b''), ''), @@ -523,7 +527,8 @@ def test_delete_collections(mocker, journalist_app, source_app, test_journo): num_sources = 2 for i in range(num_sources): app.get('/generate') - app.post('/create') + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}) app.post('/submit', data=dict( msg="This is a test " + str(i) + ".", fh=(BytesIO(b''), ''), @@ -577,7 +582,8 @@ def test_filenames(source_app, journalist_app, test_journo): # add a source and submit stuff with source_app.test_client() as app: app.get('/generate') - app.post('/create') + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}) _helper_filenames_submit(app) # navigate to the collection page @@ -603,7 +609,8 @@ def test_filenames_delete(journalist_app, source_app, test_journo): # add a source and submit stuff with source_app.test_client() as app: app.get('/generate') - app.post('/create') + tab_id = next(iter(session['codenames'].keys())) + app.post('/create', data={'tab_id': tab_id}) _helper_filenames_submit(app) # navigate to the collection page @@ -714,7 +721,8 @@ def test_prevent_document_uploads(source_app, journalist_app, test_admin): # Check that the source interface accepts only messages: with source_app.test_client() as app: app.get('/generate') - resp = app.post('/create', follow_redirects=True) + tab_id = next(iter(session['codenames'].keys())) + resp = app.post('/create', data={'tab_id': tab_id}, follow_redirects=True) assert resp.status_code == 200 text = resp.data.decode('utf-8') @@ -739,7 +747,8 @@ def test_no_prevent_document_uploads(source_app, journalist_app, test_admin): # Check that the source interface accepts both files and messages: with source_app.test_client() as app: app.get('/generate') - resp = app.post('/create', follow_redirects=True) + tab_id = next(iter(session['codenames'].keys())) + resp = app.post('/create', data={'tab_id': tab_id}, follow_redirects=True) assert resp.status_code == 200 text = resp.data.decode('utf-8') diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 7cf06ae7f4..c74bbdd6bd 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -83,7 +83,7 @@ def test_generate(source_app): with source_app.test_client() as app: resp = app.get(url_for('main.generate')) assert resp.status_code == 200 - session_codename = session['codename'] + session_codename = next(iter(session['codenames'].values())) text = resp.data.decode('utf-8') assert "This codename is what you will use in future visits" in text @@ -113,11 +113,13 @@ def test_create_new_source(source_app): with source_app.test_client() as app: resp = app.get(url_for('main.generate')) assert resp.status_code == 200 - resp = app.post(url_for('main.create'), follow_redirects=True) + tab_id = next(iter(session['codenames'].keys())) + resp = app.post(url_for('main.create'), data={'tab_id': tab_id}, follow_redirects=True) assert session['logged_in'] is True # should be redirected to /lookup text = resp.data.decode('utf-8') assert "Submit Files" in text + assert 'codenames' not in session def test_generate_too_long_codename(source_app): @@ -143,17 +145,18 @@ def test_create_duplicate_codename_logged_in_not_in_session(source_app): with source_app.test_client() as app: resp = app.get(url_for('main.generate')) assert resp.status_code == 200 + tab_id = next(iter(session['codenames'].keys())) # Create a source the first time - resp = app.post(url_for('main.create'), follow_redirects=True) + resp = app.post(url_for('main.create'), data={'tab_id': tab_id}, follow_redirects=True) assert resp.status_code == 200 codename = session['codename'] with source_app.test_client() as app: # Attempt to add the same source with app.session_transaction() as sess: - sess['codename'] = codename - resp = app.post(url_for('main.create'), follow_redirects=True) + sess['codenames'] = {tab_id: codename} + resp = app.post(url_for('main.create'), data={'tab_id': tab_id}, follow_redirects=True) logger.assert_called_once() assert ("Attempt to create a source with duplicate codename" in logger.call_args[0][0]) @@ -163,26 +166,31 @@ def test_create_duplicate_codename_logged_in_not_in_session(source_app): def test_create_duplicate_codename_logged_in_in_session(source_app): - with patch.object(source.app.logger, 'error') as logger: - with source_app.test_client() as app: - resp = app.get(url_for('main.generate')) - assert resp.status_code == 200 - - # Create a source the first time - resp = app.post(url_for('main.create'), follow_redirects=True) - assert resp.status_code == 200 + with source_app.test_client() as app: + resp = app.get(url_for('main.generate')) + assert resp.status_code == 200 + tab_id = next(iter(session['codenames'].keys())) - # Attempt to add the same source - resp = app.post(url_for('main.create'), follow_redirects=True) - logger.assert_called_once() - assert ("Attempt to create a source with duplicate codename" - in logger.call_args[0][0]) - assert resp.status_code == 500 - assert 'codename' not in session + # Create a source the first time + resp = app.post(url_for('main.create'), data={'tab_id': tab_id}, follow_redirects=True) + assert resp.status_code == 200 + codename = session['codename'] + logged_in = session['logged_in'] - # Reproducer for bug #4361 - resp = app.post(url_for('main.index'), follow_redirects=True) - assert 'logged_in' not in session + # Attempt to add another source in the same session + with source_app.test_client() as app: + resp = app.get(url_for('main.generate')) + assert resp.status_code == 200 + tab_id = next(iter(session['codenames'].keys())) + with app.session_transaction() as sess: + sess['codename'] = codename + sess['logged_in'] = logged_in + resp = app.post(url_for('main.create'), data={'tab_id': tab_id}, follow_redirects=True) + assert resp.status_code == 200 + assert session['codename'] == codename + text = resp.data.decode('utf-8') + assert "You are already logged in." in text + assert "Submit Files" in text def test_lookup(source_app): diff --git a/securedrop/tests/utils/db_helper.py b/securedrop/tests/utils/db_helper.py index e598561c4b..bb2f14761a 100644 --- a/securedrop/tests/utils/db_helper.py +++ b/securedrop/tests/utils/db_helper.py @@ -173,6 +173,6 @@ def new_codename(client, session): """Helper function to go through the "generate codename" flow. """ client.get('/generate') - codename = session['codename'] - client.post('/create') + tab_id, codename = next(iter(session['codenames'].items())) + client.post('/create', data={'tab_id': tab_id}) return codename