From 93c8dda431e22c2815476c50966b7807d1357dce Mon Sep 17 00:00:00 2001 From: Paul Van Eck Date: Sun, 27 Sep 2015 20:54:15 -0700 Subject: [PATCH] Associate results to user instead of key Results will now be tied to openids instead of public keys. With this, users won't lose access to their data if they delete associated public keys from their profiles. Addresses-Spec: https://review.openstack.org/#/c/228565 Change-Id: I916a8e588247c43e6d220c84b08610b10cc64933 --- .../partials/reportDetails.html | 5 ++ refstack/api/app.py | 3 +- refstack/api/constants.py | 2 +- refstack/api/controllers/results.py | 11 +++-- refstack/api/utils.py | 17 +++---- refstack/db/api.py | 8 ++++ .../428e5aef5534_associate_test_result.py | 46 +++++++++++++++++++ refstack/db/sqlalchemy/api.py | 25 ++++++++-- refstack/tests/unit/test_api.py | 10 ++-- refstack/tests/unit/test_api_utils.py | 44 ++++++++---------- refstack/tests/unit/test_app.py | 7 ++- refstack/tests/unit/test_db.py | 45 +++++++++++++++--- 12 files changed, 165 insertions(+), 58 deletions(-) create mode 100644 refstack/db/migrations/alembic/versions/428e5aef5534_associate_test_result.py diff --git a/refstack-ui/app/components/results-report/partials/reportDetails.html b/refstack-ui/app/components/results-report/partials/reportDetails.html index 56afe240..40de3c24 100644 --- a/refstack-ui/app/components/results-report/partials/reportDetails.html +++ b/refstack-ui/app/components/results-report/partials/reportDetails.html @@ -42,6 +42,7 @@ report page. diff --git a/refstack/api/app.py b/refstack/api/app.py index 2579a40d..38b94c1c 100644 --- a/refstack/api/app.py +++ b/refstack/api/app.py @@ -124,7 +124,8 @@ class JSONErrorHook(pecan.hooks.PecanHook): elif isinstance(exc, webob.exc.HTTPError): return webob.Response( body=json.dumps({'code': exc.status_int, - 'title': exc.title}), + 'title': exc.title, + 'detail': exc.detail}), status=exc.status_int, content_type='application/json' ) diff --git a/refstack/api/constants.py b/refstack/api/constants.py index 2a0b8687..95135e78 100644 --- a/refstack/api/constants.py +++ b/refstack/api/constants.py @@ -41,7 +41,7 @@ CSRF_TOKEN = 'csrf_token' USER_OPENID = 'user_openid' # Test metadata fields -PUBLIC_KEY = 'public_key' +USER = 'user' SHARED_TEST_RUN = 'shared' # Roles diff --git a/refstack/api/controllers/results.py b/refstack/api/controllers/results.py index 8ecf6c32..5e658fb7 100644 --- a/refstack/api/controllers/results.py +++ b/refstack/api/controllers/results.py @@ -92,12 +92,17 @@ class ResultsController(validation.BaseRestControllerWithValidation): """Handler for storing item. Should return new item id.""" test_ = test.copy() if pecan.request.headers.get('X-Public-Key'): + key = pecan.request.headers.get('X-Public-Key').strip().split()[1] if 'meta' not in test_: test_['meta'] = {} - test_['meta'][const.PUBLIC_KEY] = \ - pecan.request.headers.get('X-Public-Key') + pubkey = db.get_pubkey(key) + if not pubkey: + pecan.abort(400, 'User with specified key not found. ' + 'Please log into the RefStack server to ' + 'upload your key.') + + test_['meta'][const.USER] = pubkey.openid test_id = db.store_results(test_) - LOG.debug(test_) return {'test_id': test_id, 'url': parse.urljoin(CONF.ui_url, CONF.api.test_results_url) % test_id} diff --git a/refstack/api/utils.py b/refstack/api/utils.py index 4162d325..e9ad94d0 100644 --- a/refstack/api/utils.py +++ b/refstack/api/utils.py @@ -59,8 +59,8 @@ def _get_input_params_from_request(expected_params): def parse_input_params(expected_input_params): """Parse input parameters from request. - :param expecred_params: (array) Expected input - params specified in constants. + :param expected_input_params: (array) Expected input + params specified in constants. """ raw_filters = _get_input_params_from_request(expected_input_params) filters = copy.deepcopy(raw_filters) @@ -84,10 +84,6 @@ def parse_input_params(expected_input_params): if const.SIGNED in filters: if is_authenticated(): filters[const.OPENID] = get_user_id() - filters[const.USER_PUBKEYS] = [ - ' '.join((pk['format'], pk['pubkey'])) - for pk in get_user_public_keys() - ] else: raise api_exc.ParseInputsError( 'To see signed test results you need to authenticate') @@ -228,8 +224,8 @@ def get_user_role(test_id): def _check_user(test_id): """Check that user has access to shared test run.""" - test_pubkey = db.get_test_meta_key(test_id, const.PUBLIC_KEY) - if not test_pubkey: + test_owner = db.get_test_meta_key(test_id, const.USER) + if not test_owner: return True elif db.get_test_meta_key(test_id, const.SHARED_TEST_RUN): return True @@ -241,9 +237,8 @@ def _check_owner(test_id): """Check that user has access to specified test run as owner.""" if not is_authenticated(): return False - test_pubkey = db.get_test_meta_key(test_id, const.PUBLIC_KEY) - return test_pubkey in [' '.join((pk['format'], pk['pubkey'])) - for pk in get_user_public_keys()] + user = db.get_test_meta_key(test_id, const.USER) + return user and user == get_user_id() def check_permissions(level): diff --git a/refstack/db/api.py b/refstack/db/api.py index aaf9f04d..abf6da80 100644 --- a/refstack/db/api.py +++ b/refstack/db/api.py @@ -139,6 +139,14 @@ def user_save(user_info): return IMPL.user_save(user_info) +def get_pubkey(key): + """Get pubkey info for a given key. + + :param key: public key + """ + return IMPL.get_pubkey(key) + + def store_pubkey(pubkey_info): """Store public key in to DB.""" return IMPL.store_pubkey(pubkey_info) diff --git a/refstack/db/migrations/alembic/versions/428e5aef5534_associate_test_result.py b/refstack/db/migrations/alembic/versions/428e5aef5534_associate_test_result.py new file mode 100644 index 00000000..170ad0cb --- /dev/null +++ b/refstack/db/migrations/alembic/versions/428e5aef5534_associate_test_result.py @@ -0,0 +1,46 @@ +"""Associate test results to users. + +Revision ID: 428e5aef5534 +Revises: 534e20be9964 +Create Date: 2015-11-03 00:51:34.096598 + +""" + +# revision identifiers, used by Alembic. +revision = '428e5aef5534' +down_revision = '534e20be9964' +MYSQL_CHARSET = 'utf8' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + """Upgrade DB.""" + conn = op.get_bind() + res = conn.execute("select openid,format,pubkey from pubkeys") + results = res.fetchall() + + # Get public key to user mappings. + pubkeys = {} + for result in results: + pubkeys[result[1] + " " + result[2]] = result[0] + + res = conn.execute("select test_id,value from meta where " + "meta_key='public_key'") + results = res.fetchall() + + for result in results: + test_id = result[0] + if result[1] in pubkeys: + openid = pubkeys[result[1]] + conn.execute(sa.text("update meta set meta_key='user', " + "value=:value where " + "test_id=:testid and meta_key='public_key'" + ), + value=openid, testid=test_id) + + +def downgrade(): + """Downgrade DB.""" + pass diff --git a/refstack/db/sqlalchemy/api.py b/refstack/db/sqlalchemy/api.py index 6a90d7ef..2850f97c 100644 --- a/refstack/db/sqlalchemy/api.py +++ b/refstack/db/sqlalchemy/api.py @@ -218,17 +218,17 @@ def _apply_filters_for_query(query, filters): query = query.filter(models.Test.cpid == cpid) signed = api_const.SIGNED in filters + # If we only want to get the user's test results. if signed: query = (query .join(models.Test.meta) - .filter(models.TestMeta.meta_key == api_const.PUBLIC_KEY) - .filter(models.TestMeta.value.in_( - filters[api_const.USER_PUBKEYS])) + .filter(models.TestMeta.meta_key == api_const.USER) + .filter(models.TestMeta.value == filters[api_const.OPENID]) ) else: signed_results = (query.session .query(models.TestMeta.test_id) - .filter_by(meta_key=api_const.PUBLIC_KEY)) + .filter_by(meta_key=api_const.USER)) shared_results = (query.session .query(models.TestMeta.test_id) .filter_by(meta_key=api_const.SHARED_TEST_RUN)) @@ -280,6 +280,23 @@ def user_save(user_info): return user +def get_pubkey(key): + """Get the pubkey info corresponding to the given public key. + + The md5 hash of the key is used for the query for quicker lookups. + """ + session = get_session() + md5_hash = hashlib.md5(base64.b64decode(key.encode('ascii'))).hexdigest() + pubkeys = session.query(models.PubKey).filter_by(md5_hash=md5_hash).all() + if len(pubkeys) == 1: + return pubkeys[0] + elif len(pubkeys) > 1: + for pubkey in pubkeys: + if pubkey['pubkey'] == key: + return pubkey + return None + + def store_pubkey(pubkey_info): """Store public key in to DB.""" pubkey = models.PubKey() diff --git a/refstack/tests/unit/test_api.py b/refstack/tests/unit/test_api.py index 9e616a10..2caa890c 100644 --- a/refstack/tests/unit/test_api.py +++ b/refstack/tests/unit/test_api.py @@ -158,12 +158,16 @@ class ResultsControllerTestCase(BaseControllerTestCase): mock_store_results.assert_called_once_with({'answer': 42}) @mock.patch('refstack.db.store_results') - def test_post_with_sign(self, mock_store_results): + @mock.patch('refstack.db.get_pubkey') + def test_post_with_sign(self, mock_get_pubkey, mock_store_results): self.mock_request.body = '{"answer": 42}' self.mock_request.headers = { 'X-Signature': 'fake-sign', - 'X-Public-Key': 'fake-key' + 'X-Public-Key': 'ssh-rsa Zm9vIGJhcg==' } + + mock_get_pubkey.return_value.openid = 'fake_openid' + mock_store_results.return_value = 'fake_test_id' result = self.controller.post() self.assertEqual(result, @@ -171,7 +175,7 @@ class ResultsControllerTestCase(BaseControllerTestCase): 'url': self.test_results_url % 'fake_test_id'}) self.assertEqual(self.mock_response.status, 201) mock_store_results.assert_called_once_with( - {'answer': 42, 'meta': {const.PUBLIC_KEY: 'fake-key'}} + {'answer': 42, 'meta': {const.USER: 'fake_openid'}} ) @mock.patch('refstack.db.get_test') diff --git a/refstack/tests/unit/test_api_utils.py b/refstack/tests/unit/test_api_utils.py index ef39abd6..5d249977 100644 --- a/refstack/tests/unit/test_api_utils.py +++ b/refstack/tests/unit/test_api_utils.py @@ -142,8 +142,7 @@ class APIUtilsTestCase(base.BaseTestCase): @mock.patch.object(api_utils, '_get_input_params_from_request') @mock.patch.object(api_utils, 'is_authenticated', return_value=True) @mock.patch.object(api_utils, 'get_user_id', return_value='fake_id') - @mock.patch('refstack.db.get_user_pubkeys') - def test_parse_input_params_success(self, mock_get_user_pubkeys, + def test_parse_input_params_success(self, mock_get_user_id, mock_is_authenticated, mock_get_input): @@ -157,9 +156,7 @@ class APIUtilsTestCase(base.BaseTestCase): const.CPID: '12345', const.SIGNED: True } - fake_pubkeys = ({'format': 'fake', - 'pubkey': 'fake_pk'},) - mock_get_user_pubkeys.return_value = fake_pubkeys + expected_params = mock.Mock() mock_get_input.return_value = raw_filters @@ -179,11 +176,10 @@ class APIUtilsTestCase(base.BaseTestCase): const.CPID: '12345', const.SIGNED: True, const.OPENID: 'fake_id', - const.USER_PUBKEYS: ['fake fake_pk'], } result = api_utils.parse_input_params(expected_params) - self.assertEqual(result, expected_result) + self.assertEqual(expected_result, result) mock_get_input.assert_called_once_with(expected_params) @@ -333,8 +329,8 @@ class APIUtilsTestCase(base.BaseTestCase): @mock.patch('pecan.abort', side_effect=exc.HTTPError) @mock.patch('refstack.db.get_test_meta_key') @mock.patch.object(api_utils, 'is_authenticated') - @mock.patch.object(api_utils, 'get_user_public_keys') - def test_check_get_user_role(self, mock_get_user_public_keys, + @mock.patch.object(api_utils, 'get_user_id') + def test_check_get_user_role(self, mock_get_user_id, mock_is_authenticated, mock_get_test_meta_key, mock_pecan_abort): @@ -346,7 +342,7 @@ class APIUtilsTestCase(base.BaseTestCase): 'fake_test', const.ROLE_OWNER) mock_get_test_meta_key.side_effect = { - ('fake_test', const.PUBLIC_KEY): 'fake key', + ('fake_test', const.USER): 'fake_openid', ('fake_test', const.SHARED_TEST_RUN): 'true', }.get self.assertEqual(const.ROLE_USER, api_utils.get_user_role('fake_test')) @@ -355,10 +351,9 @@ class APIUtilsTestCase(base.BaseTestCase): 'fake_test', const.ROLE_OWNER) mock_is_authenticated.return_value = True - mock_get_user_public_keys.return_value = [{'format': 'fake', - 'pubkey': 'key'}] + mock_get_user_id.return_value = 'fake_openid' mock_get_test_meta_key.side_effect = { - ('fake_test', const.PUBLIC_KEY): 'fake key', + ('fake_test', const.USER): 'fake_openid', ('fake_test', const.SHARED_TEST_RUN): 'true', }.get self.assertEqual(const.ROLE_USER, api_utils.get_user_role('fake_test')) @@ -368,10 +363,9 @@ class APIUtilsTestCase(base.BaseTestCase): # Check owner level mock_is_authenticated.return_value = True - mock_get_user_public_keys.return_value = [{'format': 'fake', - 'pubkey': 'key'}] + mock_get_user_id.return_value = 'fake_openid' mock_get_test_meta_key.side_effect = lambda *args: { - ('fake_test', const.PUBLIC_KEY): 'fake key', + ('fake_test', const.USER): 'fake_openid', ('fake_test', const.SHARED_TEST_RUN): None, }.get(args) self.assertEqual(const.ROLE_OWNER, @@ -382,7 +376,7 @@ class APIUtilsTestCase(base.BaseTestCase): # Check negative cases mock_is_authenticated.return_value = False mock_get_test_meta_key.side_effect = lambda *args: { - ('fake_test', const.PUBLIC_KEY): 'fake key', + ('fake_test', const.USER): 'fake_openid', ('fake_test', const.SHARED_TEST_RUN): None, }.get(args) self.assertRaises(exc.HTTPError, api_utils.enforce_permissions, @@ -391,10 +385,9 @@ class APIUtilsTestCase(base.BaseTestCase): 'fake_test', const.ROLE_OWNER) mock_is_authenticated.return_value = True - mock_get_user_public_keys.return_value = [{'format': 'fake', - 'pubkey': 'key'}] + mock_get_user_id.return_value = 'fake_openid' mock_get_test_meta_key.side_effect = lambda *args: { - ('fake_test', const.PUBLIC_KEY): 'fake other_key', + ('fake_test', const.USER): 'some_other_user', ('fake_test', const.SHARED_TEST_RUN): None, }.get(args) self.assertEqual(None, api_utils.get_user_role('fake_test')) @@ -406,8 +399,8 @@ class APIUtilsTestCase(base.BaseTestCase): @mock.patch('pecan.abort', side_effect=exc.HTTPError) @mock.patch('refstack.db.get_test_meta_key') @mock.patch.object(api_utils, 'is_authenticated') - @mock.patch.object(api_utils, 'get_user_public_keys') - def test_check_permissions(self, mock_get_user_public_keys, + @mock.patch.object(api_utils, 'get_user_id') + def test_check_permissions(self, mock_get_user_id, mock_is_authenticated, mock_get_test_meta_key, mock_pecan_abort): @@ -431,11 +424,10 @@ class APIUtilsTestCase(base.BaseTestCase): public_test = 'fake_public_test' private_test = 'fake_test' - mock_get_user_public_keys.return_value = [{'format': 'fake', - 'pubkey': 'key'}] + mock_get_user_id.return_value = 'fake_openid' mock_get_test_meta_key.side_effect = lambda *args: { - (public_test, const.PUBLIC_KEY): None, - (private_test, const.PUBLIC_KEY): 'fake key', + (public_test, const.USER): None, + (private_test, const.USER): 'fake_openid', (private_test, const.SHARED_TEST_RUN): None, }.get(args) diff --git a/refstack/tests/unit/test_app.py b/refstack/tests/unit/test_app.py index af102d83..a661607f 100644 --- a/refstack/tests/unit/test_app.py +++ b/refstack/tests/unit/test_app.py @@ -58,11 +58,14 @@ class JSONErrorHookTestCase(base.BaseTestCase): self.CONF.set_override('app_dev_mode', False, 'api') exc = mock.Mock(spec=webob.exc.HTTPError, status=418, status_int=418, - title='fake_title') + title='fake_title', + detail='fake_detail') self._on_error( response, exc, expected_status_code=exc.status, - expected_body={'code': exc.status_int, 'title': exc.title} + expected_body={'code': exc.status_int, + 'title': exc.title, + 'detail': exc.detail} ) @mock.patch.object(webob, 'Response') diff --git a/refstack/tests/unit/test_db.py b/refstack/tests/unit/test_db.py index fbbfbe42..552de813 100644 --- a/refstack/tests/unit/test_db.py +++ b/refstack/tests/unit/test_db.py @@ -370,7 +370,7 @@ class DBBackendTestCase(base.BaseTestCase): unsigned_query.session.query.assert_has_calls(( mock.call(mock_meta.test_id), - mock.call().filter_by(meta_key='public_key'), + mock.call().filter_by(meta_key='user'), mock.call(mock_meta.test_id), mock.call().filter_by(meta_key='shared'), )) @@ -390,13 +390,16 @@ class DBBackendTestCase(base.BaseTestCase): query = mock.Mock() mock_test.created_at = six.text_type() mock_meta.test_id = six.text_type() + mock_meta.meta_key = 'user' + mock_meta.value = 'test-openid' filters = { api_const.START_DATE: 'fake1', api_const.END_DATE: 'fake2', api_const.CPID: 'fake3', api_const.USER_PUBKEYS: ['fake_pk'], - api_const.SIGNED: 'true' + api_const.SIGNED: 'true', + api_const.OPENID: 'test-openid' } signed_query = (query @@ -409,14 +412,12 @@ class DBBackendTestCase(base.BaseTestCase): signed_query.join.assert_called_once_with(mock_test.meta) signed_query = signed_query.join.return_value signed_query.filter.assert_called_once_with( - mock_meta.meta_key == api_const.PUBLIC_KEY + mock_meta.meta_key == api_const.USER ) signed_query = signed_query.filter.return_value - mock_meta.value.in_.assert_called_once_with( - filters[api_const.USER_PUBKEYS]) signed_query.filter.assert_called_once_with( - mock_meta.value.in_.return_value) - + mock_meta.value == filters[api_const.OPENID] + ) filtered_query = signed_query.filter.return_value self.assertEqual(result, filtered_query) @@ -518,6 +519,36 @@ class DBBackendTestCase(base.BaseTestCase): user.update.assert_called_once_with(user_info) session.begin.assert_called_once_with() + @mock.patch.object(api, 'get_session', + return_value=mock.Mock(name='session'),) + @mock.patch('refstack.db.sqlalchemy.models.PubKey') + def test_get_pubkey(self, mock_model, mock_get_session): + key = 'AAAAB3Nz' + khash = hashlib.md5(base64.b64decode(key.encode('ascii'))).hexdigest() + session = mock_get_session.return_value + query = session.query.return_value + filtered = query.filter_by.return_value + + # Test no key match. + filtered.all.return_value = [] + result = api.get_pubkey(key) + self.assertEqual(None, result) + + session.query.assert_called_once_with(mock_model) + query.filter_by.assert_called_once_with(md5_hash=khash) + filtered.all.assert_called_once_with() + + # Test only one key match. + filtered.all.return_value = [{'pubkey': key, 'md5_hash': khash}] + result = api.get_pubkey(key) + self.assertEqual({'pubkey': key, 'md5_hash': khash}, result) + + # Test multiple keys with same md5 hash. + filtered.all.return_value = [{'pubkey': 'key2', 'md5_hash': khash}, + {'pubkey': key, 'md5_hash': khash}] + result = api.get_pubkey(key) + self.assertEqual({'pubkey': key, 'md5_hash': khash}, result) + @mock.patch.object(api, 'get_session') @mock.patch('refstack.db.sqlalchemy.api.models') def test_store_pubkey(self, mock_models, mock_get_session):