From 1adae77b7c39348d1ac5a30d9ec3e6824ca56dc1 Mon Sep 17 00:00:00 2001 From: Go Hayakawa Date: Mon, 30 Sep 2019 15:44:05 +0900 Subject: [PATCH 1/2] Fix external provider user creation bug --- .../me_external_provider_user_create.py | 14 ++++- .../test_me_external_provider_user_create.py | 54 ++++++++++++------- 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py b/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py index 740534f7..1acc68eb 100644 --- a/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py +++ b/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py @@ -8,6 +8,7 @@ from botocore.exceptions import ClientError from user_util import UserUtil from crypto_util import CryptoUtil +from record_not_found_error import RecordNotFoundError class MeExternalProviderUserCreate(LambdaBase): @@ -36,7 +37,16 @@ def exec_main_proc(self): users_table = self.dynamodb.Table(os.environ['USERS_TABLE_NAME']) external_provider_users_table = self.dynamodb.Table(os.environ['EXTERNAL_PROVIDER_USERS_TABLE_NAME']) external_provider_user_id = params['requestContext']['authorizer']['claims']['cognito:username'] - exist_check_user = users_table.get_item(Key={'user_id': body['user_id']}).get('Item') + + # usersテーブルのユーザIDの重複チェック + already_user_exists_in_users_table = users_table.get_item(Key={'user_id': body['user_id']}).get('Item') is not None + # cognitoのユーザIDの重複チェック + already_user_exists_in_cognito = False + try: + already_user_exists_in_cognito = UserUtil.get_cognito_user_info(self.cognito, body['user_id']) is not None + except RecordNotFoundError: + pass + already_user_exists = already_user_exists_in_users_table or already_user_exists_in_cognito external_provider_user = external_provider_users_table.get_item(Key={ 'external_provider_user_id': external_provider_user_id @@ -45,7 +55,7 @@ def exec_main_proc(self): if (external_provider_user is not None) and ('user_id' in external_provider_user): raise ValidationError('The user id of this user has been added.') - elif exist_check_user is None: + elif not already_user_exists: # EXTERNAL_PROVIDERのidで作成したcognitoのユーザーを除去 if UserUtil.delete_external_provider_id_cognito_user(self.cognito, external_provider_user_id): diff --git a/tests/handlers/me/external_provider_user/create/test_me_external_provider_user_create.py b/tests/handlers/me/external_provider_user/create/test_me_external_provider_user_create.py index 9feb546e..08c90f89 100644 --- a/tests/handlers/me/external_provider_user/create/test_me_external_provider_user_create.py +++ b/tests/handlers/me/external_provider_user/create/test_me_external_provider_user_create.py @@ -4,6 +4,7 @@ from me_external_provider_user_create import MeExternalProviderUserCreate from unittest.mock import patch from tests_util import TestsUtil +from record_not_found_error import RecordNotFoundError dynamodb = TestsUtil.get_dynamodb_client() @@ -92,6 +93,7 @@ def test_main_ok(self): user_mock.delete_external_provider_id_cognito_user.return_value = True user_mock.has_user_id.return_value = True user_mock.add_user_profile.return_value = None + user_mock.get_cognito_user_info.side_effect = RecordNotFoundError('Record Not Found') response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() self.assertEqual(response['statusCode'], 200) @@ -108,29 +110,29 @@ def test_main_ok(self): ) def test_already_exist_user_id(self): - event = { - 'body': { - 'user_id': 'existname', - }, - 'requestContext': { - 'authorizer': { - 'claims': { - 'cognito:username': 'Twitter-test-user', + with patch('me_external_provider_user_create.UserUtil') as user_mock: + event = { + 'body': { + 'user_id': 'existname', + }, + 'requestContext': { + 'authorizer': { + 'claims': { + 'cognito:username': 'Twitter-test-user', + } } } } - } - event['body'] = json.dumps(event['body']) + event['body'] = json.dumps(event['body']) - response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() - self.assertEqual(response['statusCode'], 400) + user_mock.get_cognito_user_info.side_effect = RecordNotFoundError('Record Not Found') + + response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() + self.assertEqual(response['statusCode'], 400) def test_already_added_user_id(self): event = { - 'body': { - 'user_id': 'username02', - }, 'requestContext': { 'authorizer': { 'claims': { @@ -140,10 +142,26 @@ def test_already_added_user_id(self): } } - event['body'] = json.dumps(event['body']) + # usersテーブルとcognitoの両方にユーザが存在する場合 + with patch('me_external_provider_user_create.UserUtil') as user_mock: + event['body'] = json.dumps({'user_id': 'username02'}) + user_mock.get_cognito_user_info.return_value = {'Username': 'username02', 'UserAttributes': {}} + response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() + self.assertEqual(response['statusCode'], 400) - response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() - self.assertEqual(response['statusCode'], 400) + # usersテーブルのみにユーザが存在する場合(想定されるデータ不整合のため、そのような場合でも意図したエラーを返すことのテスト) + with patch('me_external_provider_user_create.UserUtil') as user_mock: + event['body'] = json.dumps({'user_id': 'username02'}) + user_mock.get_cognito_user_info.side_effect = RecordNotFoundError('Record Not Found') + response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() + self.assertEqual(response['statusCode'], 400) + + # cognitoのみにユーザが存在する場合(想定されるデータ不整合のため、そのような場合でも意図したエラーを返すことのテスト) + with patch('me_external_provider_user_create.UserUtil') as user_mock: + event['body'] = json.dumps({'user_id': 'onlycognito'}) + user_mock.get_cognito_user_info.return_value = {'Username': 'onlycognito', 'UserAttributes': {}} + response = MeExternalProviderUserCreate(event=event, context="", dynamodb=dynamodb).main() + self.assertEqual(response['statusCode'], 400) def test_invalid_line_user_id(self): event = { From 1c9c1af8baece3bd6a6005b7a7949d1d18da6d81 Mon Sep 17 00:00:00 2001 From: Go Hayakawa Date: Mon, 30 Sep 2019 15:45:09 +0900 Subject: [PATCH 2/2] Fix error handling in external provider user creation --- .../me_external_provider_user_create.py | 121 ++++++++---------- 1 file changed, 55 insertions(+), 66 deletions(-) diff --git a/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py b/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py index 1acc68eb..7368c699 100644 --- a/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py +++ b/src/handlers/me/external_provider_user/create/me_external_provider_user_create.py @@ -2,10 +2,8 @@ import json import os import settings -import logging from lambda_base import LambdaBase from jsonschema import validate, ValidationError -from botocore.exceptions import ClientError from user_util import UserUtil from crypto_util import CryptoUtil from record_not_found_error import RecordNotFoundError @@ -58,71 +56,62 @@ def exec_main_proc(self): elif not already_user_exists: # EXTERNAL_PROVIDERのidで作成したcognitoのユーザーを除去 if UserUtil.delete_external_provider_id_cognito_user(self.cognito, external_provider_user_id): - # user_idでのCognitoユーザーの作成し直し - try: - email = external_provider_user['email'] - hash_data = external_provider_user['password'] - byte_hash_data = hash_data.encode() - decoded_iv = external_provider_user['iv'] - iv = decoded_iv.encode() - backed_password = CryptoUtil.decrypt_password(byte_hash_data, iv) - - backed_temp_password = os.environ['EXTERNAL_PROVIDER_LOGIN_COMMON_TEMP_PASSWORD'] - provider = os.environ['EXTERNAL_PROVIDER_LOGIN_MARK'] - - response = UserUtil.create_external_provider_user( - cognito=self.cognito, - user_id=body['user_id'], - user_pool_id=os.environ['COGNITO_USER_POOL_ID'], - user_pool_app_id=os.environ['COGNITO_USER_POOL_APP_ID'], - email=email, - backed_temp_password=backed_temp_password, - backed_password=backed_password, - provider=provider - ) - - UserUtil.force_non_verified_phone( - cognito=self.cognito, - user_id=body['user_id'] - ) - - UserUtil.wallet_initialization(self.cognito, os.environ['COGNITO_USER_POOL_ID'], body['user_id']) - - # ExternalProviderUsersテーブルにuser_idを追加 - UserUtil.add_user_id_to_external_provider_user( - body['user_id'], - external_provider_users_table, - external_provider_user_id - ) - - # Usersテーブルにユーザーを作成 - UserUtil.add_user_profile( - dynamodb=self.dynamodb, - user_id=body['user_id'], - user_display_name=body['user_id'] - ) - - has_user_id = UserUtil.has_user_id(self.dynamodb, external_provider_user_id) - - return { - 'statusCode': 200, - 'body': json.dumps({ - 'access_token': response['AuthenticationResult']['AccessToken'], - 'last_auth_user': body['user_id'], - 'id_token': response['AuthenticationResult']['IdToken'], - 'refresh_token': response['AuthenticationResult']['RefreshToken'], - 'status': 'login', - 'has_user_id': has_user_id - }) - } - - except ClientError as e: - logging.fatal(e) - return { - 'statusCode': 500, - 'body': json.dumps({'message': 'Internal server error'}) - } + email = external_provider_user['email'] + hash_data = external_provider_user['password'] + byte_hash_data = hash_data.encode() + decoded_iv = external_provider_user['iv'] + iv = decoded_iv.encode() + backed_password = CryptoUtil.decrypt_password(byte_hash_data, iv) + + backed_temp_password = os.environ['EXTERNAL_PROVIDER_LOGIN_COMMON_TEMP_PASSWORD'] + provider = os.environ['EXTERNAL_PROVIDER_LOGIN_MARK'] + + response = UserUtil.create_external_provider_user( + cognito=self.cognito, + user_id=body['user_id'], + user_pool_id=os.environ['COGNITO_USER_POOL_ID'], + user_pool_app_id=os.environ['COGNITO_USER_POOL_APP_ID'], + email=email, + backed_temp_password=backed_temp_password, + backed_password=backed_password, + provider=provider + ) + + UserUtil.force_non_verified_phone( + cognito=self.cognito, + user_id=body['user_id'] + ) + + UserUtil.wallet_initialization(self.cognito, os.environ['COGNITO_USER_POOL_ID'], body['user_id']) + + # ExternalProviderUsersテーブルにuser_idを追加 + UserUtil.add_user_id_to_external_provider_user( + body['user_id'], + external_provider_users_table, + external_provider_user_id + ) + + # Usersテーブルにユーザーを作成 + UserUtil.add_user_profile( + dynamodb=self.dynamodb, + user_id=body['user_id'], + user_display_name=body['user_id'] + ) + + has_user_id = UserUtil.has_user_id(self.dynamodb, external_provider_user_id) + + return { + 'statusCode': 200, + 'body': json.dumps({ + 'access_token': response['AuthenticationResult']['AccessToken'], + 'last_auth_user': body['user_id'], + 'id_token': response['AuthenticationResult']['IdToken'], + 'refresh_token': response['AuthenticationResult']['RefreshToken'], + 'status': 'login', + 'has_user_id': has_user_id + }) + } return { 'statusCode': 500,