From 91ffe4658607390284dfd662541011e745c3c18e Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 13 Jul 2021 09:15:30 -0300 Subject: [PATCH] GetCurrentUser Fix possible race condition ( user creation ) Change-Id: Icd0e63a14c602b02c4f351b9459ca009103dc316 Signed-off-by: smarcet --- app/Models/OAuth2/ResourceServerContext.php | 161 +++++++++--------- app/Services/Model/Imp/MemberService.php | 24 ++- app/Services/Model/Imp/SummitOrderService.php | 68 +++++--- .../SummitRegistrationInvitationService.php | 33 ++-- .../Model/Imp/SummitSelectionPlanService.php | 1 + .../Utils/DoctrineTransactionService.php | 4 +- tests/ResourceServerContextTest.php | 40 +++++ 7 files changed, 200 insertions(+), 131 deletions(-) create mode 100644 tests/ResourceServerContextTest.php diff --git a/app/Models/OAuth2/ResourceServerContext.php b/app/Models/OAuth2/ResourceServerContext.php index 8353c763..945ab400 100644 --- a/app/Models/OAuth2/ResourceServerContext.php +++ b/app/Models/OAuth2/ResourceServerContext.php @@ -42,30 +42,22 @@ final class ResourceServerContext implements IResourceServerContext */ private $member_service; - /** - * @var IGroupRepository - */ - private $group_repository; - /** * ResourceServerContext constructor. - * @param IGroupRepository $group_repository * @param IMemberRepository $member_repository * @param IMemberService $member_service * @param ITransactionService $tx_service */ public function __construct ( - IGroupRepository $group_repository, IMemberRepository $member_repository, IMemberService $member_service, ITransactionService $tx_service ) { $this->member_repository = $member_repository; - $this->group_repository = $group_repository; - $this->member_service = $member_service; - $this->tx_service = $tx_service; + $this->member_service = $member_service; + $this->tx_service = $tx_service; } /** @@ -154,77 +146,57 @@ final class ResourceServerContext implements IResourceServerContext return $this->getAuthContextVar('application_type'); } - private function getAuthContextVar(string $varName){ + private function getAuthContextVar(string $varName) + { return isset($this->auth_context[$varName]) ? $this->auth_context[$varName] : null; } public function getCurrentUser(bool $synch_groups = true): ?Member { - return $this->tx_service->transaction(function() use($synch_groups) { - $member = null; - // legacy test, for new IDP version this value came on null - $id = $this->getCurrentUserExternalId(); - if(!is_null($id) && !empty($id)){ - $member = $this->member_repository->getByExternalId(intval($id)); - if(!is_null($member)) { - return $synch_groups ? $this->checkGroups($member) : $member; - } - } + $member = null; - // is null - if(is_null($member)){ - // try to get by external id - $id = $this->getCurrentUserId(); + // try to get by external id + $user_external_id = $this->getAuthContextVar('user_id'); + $user_first_name = $this->getAuthContextVar('user_first_name'); + $user_last_name = $this->getAuthContextVar('user_last_name'); + $user_email = $this->getAuthContextVar('user_email'); + $user_email_verified = boolval($this->getAuthContextVar('user_email_verified')); - if(is_null($id)) { - return null; - } - $member = $this->member_repository->getByExternalId(intval($id)); + if (is_null($user_external_id)) { + return null; + } - if(!is_null($member)){ - $user_first_name = $this->getAuthContextVar('user_first_name'); - $user_last_name = $this->getAuthContextVar('user_last_name'); - $user_email = $this->getAuthContextVar('user_email'); - $user_email_verified = boolval($this->getAuthContextVar('user_email_verified')); + // first we check by external id + $member = $this->tx_service->transaction(function () use ($user_external_id) { + return $this->member_repository->getByExternalIdExclusiveLock(intval($user_external_id)); + }); - if(!empty($user_email)) - $member->setEmail($user_email); - if(!empty($user_first_name)) - $member->setFirstName($user_first_name); - if(!empty($user_last_name)) - $member->setLastName($user_last_name); - - $member->setEmailVerified($user_email_verified); - return $synch_groups ? $this->checkGroups($member) : $member; - } - } - - if(is_null($member)) { + if (is_null($member)) { + // then by primary email + $member = $this->tx_service->transaction(function () use ($user_email) { // we assume that is new idp version and claims already exists on context - $user_external_id = $this->getAuthContextVar('user_id'); - $user_first_name = $this->getAuthContextVar('user_first_name'); - $user_last_name = $this->getAuthContextVar('user_last_name'); - $user_email = $this->getAuthContextVar('user_email'); - $user_email_verified = boolval($this->getAuthContextVar('user_email_verified')); + $user_email = $this->getAuthContextVar('user_email'); // at last resort try to get by email Log::debug(sprintf("ResourceServerContext::getCurrentUser getting user by email %s", $user_email)); - $member = $this->member_repository->getByEmail($user_email); + return $this->member_repository->getByEmailExclusiveLock($user_email); + }); + } - if (is_null($member)) {// user exist on IDP but not in our local DB, proceed to create it - Log::debug - ( - sprintf - ( - "ResourceServerContext::getCurrentUser creating user email %s user_external_id %s fname %s lname %s", - $user_email, - $user_external_id, - $user_first_name, - $user_last_name - ) - ); - - $member = $this->member_service->registerExternalUser + if (is_null($member)) {// user exist on IDP but not in our local DB, proceed to create it + Log::debug + ( + sprintf + ( + "ResourceServerContext::getCurrentUser creating user email %s user_external_id %s fname %s lname %s", + $user_email, + $user_external_id, + $user_first_name, + $user_last_name + ) + ); + try { + $member = $this->member_service->registerExternalUser ( new ExternalUserDTO ( @@ -236,19 +208,40 @@ final class ResourceServerContext implements IResourceServerContext $user_email_verified ) ); - } - - if(!empty($user_email)) - $member->setEmail($user_email); - if(!empty($user_first_name)) - $member->setFirstName($user_first_name); - if(!empty($user_last_name)) - $member->setLastName($user_last_name); - - $member->setEmailVerified(boolval($user_email_verified)); - $member->setUserExternalId($user_external_id); - + } catch (\Exception $ex) { + Log::warning($ex); + // race condition lost + $member = $this->tx_service->transaction(function () use ($user_external_id) { + return $this->member_repository->getByExternalIdExclusiveLock(intval($user_external_id)); + }); } + } + + if (is_null($member)) { + Log::warning(sprintf("ResourceServerContext::getCurrentUser user not found %s (%s).", $user_external_id, $user_email)); + return null; + } + + return $this->tx_service->transaction(function () use + ( + $member, + $user_email, + $user_first_name, + $user_last_name, + $user_external_id, + $user_email_verified, + $synch_groups + ) { + // update member fields + if (!empty($user_email)) + $member->setEmail($user_email); + if (!empty($user_first_name)) + $member->setFirstName($user_first_name); + if (!empty($user_last_name)) + $member->setLastName($user_last_name); + + $member->setUserExternalId($user_external_id); + $member->setEmailVerified($user_email_verified); return $synch_groups ? $this->checkGroups($member) : $member; }); @@ -258,15 +251,16 @@ final class ResourceServerContext implements IResourceServerContext * @param Member $member * @return Member */ - private function checkGroups(Member $member):Member{ + private function checkGroups(Member $member): Member + { Log::debug(sprintf("ResourceServerContext::checkGroups member %s %s", $member->getId(), $member->getEmail())); // check groups $groups = []; - foreach ($this->getCurrentUserGroups() as $idpGroup){ + foreach ($this->getCurrentUserGroups() as $idpGroup) { Log::debug(sprintf("ResourceServerContext::checkGroups member %s %s group %s", $member->getId(), $member->getEmail(), json_encode($idpGroup))); $slug = $idpGroup['slug'] ?? ''; Log::debug(sprintf("ResourceServerContext::checkGroups member %s %s group slug %s", $member->getId(), $member->getEmail(), $slug)); - if(empty($slug)){ + if (empty($slug)) { continue; } $groups[] = trim($slug); @@ -274,13 +268,14 @@ final class ResourceServerContext implements IResourceServerContext } return $this->member_service->synchronizeGroups($member, $groups); } + /** * @return array */ public function getCurrentUserGroups(): array { $res = $this->getAuthContextVar('user_groups'); - if(is_null($res)){ + if (is_null($res)) { Log::debug("ResourceServerContext::getCurrentUserGroups is null"); return []; } diff --git a/app/Services/Model/Imp/MemberService.php b/app/Services/Model/Imp/MemberService.php index 415a240a..6cb6a737 100644 --- a/app/Services/Model/Imp/MemberService.php +++ b/app/Services/Model/Imp/MemberService.php @@ -20,6 +20,7 @@ use App\Services\Model\dto\ExternalUserDTO; use DateTime; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Log; +use LaravelDoctrine\ORM\Facades\Registry; use libs\utils\ICacheService; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; @@ -276,6 +277,7 @@ final class MemberService public function registerExternalUser(ExternalUserDTO $userDTO): Member { return $this->tx_service->transaction(function () use ($userDTO) { + Log::debug ( sprintf @@ -288,15 +290,19 @@ final class MemberService ) ); - $member = new Member(); - $member->setActive($userDTO->isActive()); - $member->setEmailVerified( $userDTO->isEmailVerified()); - $member->setEmail($userDTO->getEmail()); - $member->setFirstName( $userDTO->getFirstName()); - $member->setLastName($userDTO->getLastName()); - $member->setUserExternalId($userDTO->getId()); - $this->member_repository->add($member, true); - Event::fire(new NewMember($member->getId())); + $member = $this->member_repository->getByExternalIdExclusiveLock($userDTO->getId()); + + if(is_null($member)) { + $member = new Member(); + $member->setUserExternalId($userDTO->getId()); + $member->setActive($userDTO->isActive()); + $member->setEmailVerified($userDTO->isEmailVerified()); + $member->setEmail($userDTO->getEmail()); + $member->setFirstName($userDTO->getFirstName()); + $member->setLastName($userDTO->getLastName()); + $this->member_repository->add($member, true); + Event::fire(new NewMember($member->getId())); + } return $member; }); } diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 242df195..797b7da3 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -1260,6 +1260,7 @@ final class SummitOrderService public function reSendOrderEmail(int $order_id):SummitOrder { return $this->tx_service->transaction(function () use ($order_id) { + $order = $this->order_repository->getByIdExclusiveLock($order_id); if (is_null($order) || !$order instanceof SummitOrder) @@ -1324,20 +1325,28 @@ final class SummitOrderService $ownerEmail ) ); - - // we have an user on idp - $member = $this->member_service->registerExternalUser - ( - new ExternalUserDTO + $external_id = $user['id']; + try { + // we have an user on idp + $member = $this->member_service->registerExternalUser ( - $user['id'], - $user['email'], - $user['first_name'], - $user['last_name'], - boolval($user['active']), - boolval($user['email_verified']) - ) - ); + new ExternalUserDTO + ( + $external_id, + $user['email'], + $user['first_name'], + $user['last_name'], + boolval($user['active']), + boolval($user['email_verified']) + ) + ); + } + catch (\Exception $ex){ + Log::warning($ex); + // race condition lost + $member = $this->member_repository->getByExternalIdExclusiveLock(intval($external_id)); + $order = $this->order_repository->getByIdExclusiveLock($order_id); + } // add the order to newly created member $member->addSummitRegistrationOrder($order); @@ -3367,18 +3376,27 @@ final class SummitOrderService ); // we have an user on idp - $member = $this->member_service->registerExternalUser - ( - new ExternalUserDTO - ( - $user['id'], - $user['email'], - $user['first_name'], - $user['last_name'], - boolval($user['active']), - boolval($user['email_verified']) - ) - ); + $external_id = $user['id']; + try { + $member = $this->member_service->registerExternalUser + ( + new ExternalUserDTO + ( + $external_id, + $user['email'], + $user['first_name'], + $user['last_name'], + boolval($user['active']), + boolval($user['email_verified']) + ) + ); + } + catch (\Exception $ex){ + // race condition lost, try to get it + Log::warning($ex); + $member = $this->member_repository->getByExternalIdExclusiveLock(intval($external_id)); + $order = $this->order_repository->getByIdExclusiveLock($orderId); + } // add the order to newly created member $member->addSummitRegistrationOrder($order); } diff --git a/app/Services/Model/Imp/SummitRegistrationInvitationService.php b/app/Services/Model/Imp/SummitRegistrationInvitationService.php index 1f868a5e..40229a8d 100644 --- a/app/Services/Model/Imp/SummitRegistrationInvitationService.php +++ b/app/Services/Model/Imp/SummitRegistrationInvitationService.php @@ -238,20 +238,29 @@ final class SummitRegistrationInvitationService $email ) ); + $external_id = $user['id']; + try { - // we have an user on idp - $member = $this->member_service->registerExternalUser - ( - new ExternalUserDTO + // we have an user on idp + $member = $this->member_service->registerExternalUser ( - $user['id'], - $user['email'], - $user['first_name'], - $user['last_name'], - boolval($user['active']), - boolval($user['email_verified']) - ) - ); + new ExternalUserDTO + ( + $external_id, + $user['email'], + $user['first_name'], + $user['last_name'], + boolval($user['active']), + boolval($user['email_verified']) + ) + ); + } + catch (\Exception $ex){ + Log::warning($ex); + // race condition lost + $member = $this->member_repository->getByExternalIdExclusiveLock(intval($external_id)); + $invitation = $this->invitation_repository->getByIdExclusiveLock($invitation->getId()); + } } } diff --git a/app/Services/Model/Imp/SummitSelectionPlanService.php b/app/Services/Model/Imp/SummitSelectionPlanService.php index 5f915781..9f5498bd 100644 --- a/app/Services/Model/Imp/SummitSelectionPlanService.php +++ b/app/Services/Model/Imp/SummitSelectionPlanService.php @@ -306,6 +306,7 @@ final class SummitSelectionPlanService return $this->tx_service->transaction(function() use($summit, $selection_plan_id, $presentation_id, $payload){ $current_member = $this->resource_server_ctx->getCurrentUser(); + if(is_null($current_member)) throw new AuthzException("User not Found"); diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php index 8716544b..0e73389a 100644 --- a/app/Services/Utils/DoctrineTransactionService.php +++ b/app/Services/Utils/DoctrineTransactionService.php @@ -78,6 +78,7 @@ final class DoctrineTransactionService implements ITransactionService // new entity manager $con = $em->getConnection(); } + $con->setTransactionIsolation($isolationLevel); $con->beginTransaction(); // suspend auto-commit $result = $callback($this); @@ -99,9 +100,8 @@ final class DoctrineTransactionService implements ITransactionService } catch (Exception $ex) { Log::warning("rolling back transaction"); Log::warning($ex); - $em->close(); - Log::warning("DoctrineTransactionService::transaction con->rollBack"); $con->rollBack(); + $em->close(); throw $ex; } } diff --git a/tests/ResourceServerContextTest.php b/tests/ResourceServerContextTest.php new file mode 100644 index 00000000..c1b68394 --- /dev/null +++ b/tests/ResourceServerContextTest.php @@ -0,0 +1,40 @@ +setAuthorizationContext($context); + + $member = $ctx->getCurrentUser(true); + } +} \ No newline at end of file