Merge pull request #321 from browserstack/security/enigma-races

security(races): serialize approval transitions with select_for_update + idempotency guard (F-027/028/029)

Author: vedharish

Access/accessrequest_helper.py

@@ -632,64 +632,74 @@ def is_request_valid(request_id, access_mapping):
 def accept_user_access_requests(auth_user, request_id):
     """ Grant for individual user access requests """
     json_response = {}
-    access_mapping = UserAccessMapping.get_access_request(request_id)
-    if not is_request_valid(request_id, access_mapping):
-        json_response["error"] = USER_REQUEST_IN_PROCESS_ERR_MSG.format(
-            request_id=request_id,
-        )
-        return json_response
-
-    requester = access_mapping.user_identity.user
-    if auth_user.user == requester:
-        json_response["error"] = SELF_APPROVAL_ERROR_MSG
-        return json_response
+    try:
+        with transaction.atomic():
+            # F-027: lock the mapping row so the validity/approver-state check and
+            # the processing() status write are atomic. Without the lock two
+            # concurrent approvers can both pass the check and double-dispatch the
+            # grant task. The Celery dispatch inside run_accept_request_task now
+            # only fires for the single request that wins the lock.
+            access_mapping = (
+                UserAccessMapping.objects.select_for_update()
+                .filter(request_id=request_id)
+                .first()
+            )
+            if not is_request_valid(request_id, access_mapping):
+                json_response["error"] = USER_REQUEST_IN_PROCESS_ERR_MSG.format(
+                    request_id=request_id,
+                )
+                return json_response
 
-    access_label = access_mapping.access.access_label
+            requester = access_mapping.user_identity.user
+            if auth_user.user == requester:
+                json_response["error"] = SELF_APPROVAL_ERROR_MSG
+                return json_response
 
-    try:
-        permissions = _get_approver_permissions(
-            access_mapping.access.access_tag, access_label
-        )
-        approver_permissions = permissions["approver_permissions"]
-        if not helpers.check_user_permissions(
-            auth_user, list(approver_permissions.values())
-        ):
-            logger.debug(USER_REQUEST_PERMISSION_DENIED_ERR_MSG)
-            json_response["error"] = USER_REQUEST_PERMISSION_DENIED_ERR_MSG
-            return json_response
+            access_label = access_mapping.access.access_label
 
-        is_primary_approver = (
-            access_mapping.is_pending()
-            and auth_user.user.has_permission(approver_permissions["1"])
-        )
-        is_secondary_approver = (
-            access_mapping.is_secondary_pending()
-            and auth_user.user.has_permission(approver_permissions["2"])
-        )
+            permissions = _get_approver_permissions(
+                access_mapping.access.access_tag, access_label
+            )
+            approver_permissions = permissions["approver_permissions"]
+            if not helpers.check_user_permissions(
+                auth_user, list(approver_permissions.values())
+            ):
+                logger.debug(USER_REQUEST_PERMISSION_DENIED_ERR_MSG)
+                json_response["error"] = USER_REQUEST_PERMISSION_DENIED_ERR_MSG
+                return json_response
 
-        if not (is_primary_approver or is_secondary_approver):
-            logger.debug(USER_REQUEST_PERMISSION_DENIED_ERR_MSG)
-            json_response["error"] = USER_REQUEST_PERMISSION_DENIED_ERR_MSG
-            return json_response
-        if is_primary_approver and "2" in approver_permissions:
-            access_mapping.approver_1 = auth_user.user
-            access_mapping.update_access_status("SecondaryPending")
-            json_response["msg"] = USER_REQUEST_SECONDARY_PENDING_MSG.format(
-                request_id=request_id, approved_by=auth_user.username
+            is_primary_approver = (
+                access_mapping.is_pending()
+                and auth_user.user.has_permission(approver_permissions["1"])
             )
-            logger.debug(
-                USER_REQUEST_SECONDARY_PENDING_MSG.format(
+            is_secondary_approver = (
+                access_mapping.is_secondary_pending()
+                and auth_user.user.has_permission(approver_permissions["2"])
+            )
+
+            if not (is_primary_approver or is_secondary_approver):
+                logger.debug(USER_REQUEST_PERMISSION_DENIED_ERR_MSG)
+                json_response["error"] = USER_REQUEST_PERMISSION_DENIED_ERR_MSG
+                return json_response
+            if is_primary_approver and "2" in approver_permissions:
+                access_mapping.approver_1 = auth_user.user
+                access_mapping.update_access_status("SecondaryPending")
+                json_response["msg"] = USER_REQUEST_SECONDARY_PENDING_MSG.format(
                     request_id=request_id, approved_by=auth_user.username
                 )
-            )
-        else:
-            json_response = run_accept_request_task(
-                is_primary_approver,
-                access_mapping,
-                auth_user=auth_user,
-                request_id=request_id,
-                access_label=access_label,
-            )
+                logger.debug(
+                    USER_REQUEST_SECONDARY_PENDING_MSG.format(
+                        request_id=request_id, approved_by=auth_user.username
+                    )
+                )
+            else:
+                json_response = run_accept_request_task(
+                    is_primary_approver,
+                    access_mapping,
+                    auth_user=auth_user,
+                    request_id=request_id,
+                    access_label=access_label,
+                )
     except Exception as exception:
         return process_error_response(exception)
 

Access/background_task_manager.py

@@ -56,6 +56,18 @@ def background_task(func, *args):
 )
 def run_access_grant(request_id):
     user_access_mapping = UserAccessMapping.get_access_request(request_id=request_id)
+    # F-029: idempotency guard. autoretry_for=(Exception,) can re-run this task;
+    # if the request is already approved, do not call the access module again
+    # (which would create duplicate external grants).
+    if user_access_mapping is None:
+        logger.warning("run_access_grant: request %s not found; skipping", request_id)
+        return False
+    if user_access_mapping.is_approved():
+        logger.info(
+            "run_access_grant: request %s already approved; skipping re-grant",
+            request_id,
+        )
+        return True
     access_tag = user_access_mapping.access.access_tag
     user = user_access_mapping.user_identity.user
     approver = user_access_mapping.approver_1.user

Access/group_helper.py

@@ -547,57 +547,63 @@ def is_user_in_group(user_email, group_members_email):
 
 
 def accept_member(auth_user, requestId, shouldRender=True):
+    context = {}
     try:
-        membership = MembershipV2.get_membership(membership_id=requestId)
-        if not membership:
-            logger.error("Error request not found OR Invalid request type")
-            context = {}
-            context["error"] = REQUEST_NOT_FOUND_ERROR + str(e)
-            return context
+        with transaction.atomic():
+            # F-028: lock the membership row so the is_pending() check and the
+            # approve() write are atomic. Without the lock two concurrent
+            # approvers can both pass is_pending() and double-grant the group.
+            membership = (
+                MembershipV2.objects.select_for_update()
+                .filter(membership_id=requestId)
+                .first()
+            )
+            if not membership:
+                logger.error("Error request not found OR Invalid request type")
+                context["error"] = REQUEST_NOT_FOUND_ERROR
+                return context
+            if not membership.is_pending():
+                logger.warning(
+                    "An Already Approved/Declined/Processing Request was accessed by - "
+                    + auth_user.username
+                )
+                context["error"] = REQUEST_PROCESSED_BY.format(
+                    requestId=requestId, user=membership.approver.user.username
+                )
+                return context
+            if membership.is_self_approval(approver=auth_user.user):
+                context["error"] = SELF_APPROVAL_ERROR
+                return context
+
+            context["msg"] = REQUEST_PROCESSING.format(requestId=requestId)
+            membership.approve(auth_user.user)
+            group = membership.group
+            user = membership.user
+            user_mappings_list = views_helper.generate_user_mappings(
+                user, group, membership
+            )
+
+        # Lock released after commit. External grants and notifications must run
+        # outside the transaction so we never hold a row lock across them.
+        execute_group_access(user_mappings_list)
+
+        notifications.send_membership_accepted_notification(
+            user=user, group=group, membership=membership
+        )
+        logger.debug(
+            "Process has been started for the Approval of request - "
+            + requestId
+            + " - Approver="
+            + auth_user.username
+        )
+        return context
     except Exception as e:
+        logger.exception("Error while accepting group membership: %s", str(e))
         context["error"] = {
             "error_msg": INTERNAL_ERROR_MESSAGE["error_msg"],
             "msg": INTERNAL_ERROR_MESSAGE["msg"],
         }
-
-    try:
-        if not membership.is_pending():
-            logger.warning(
-                "An Already Approved/Declined/Processing Request was accessed by - "
-                + auth_user.username
-            )
-            context = {}
-            context["error"] = REQUEST_PROCESSED_BY.format(
-                requestId=requestId, user=membership.approver.user.username
-            )
-            return context
-        elif membership.is_self_approval(approver=auth_user.user):
-            context = {}
-            context["error"] = SELF_APPROVAL_ERROR
-            return context
-        else:
-            context = {}
-            context["msg"] = REQUEST_PROCESSING.format(requestId=requestId)
-            with transaction.atomic():
-                membership.approve(auth_user.user)
-                group = membership.group
-                user = membership.user
-                user_mappings_list = views_helper.generate_user_mappings(
-                    user, group, membership
-                )
-
-            execute_group_access(user_mappings_list)
-
-            notifications.send_membership_accepted_notification(
-                user=user, group=group, membership=membership
-            )
-            logger.debug(
-                "Process has been started for the Approval of request - "
-                + requestId
-                + " - Approver="
-                + auth_user.username
-            )
-            return context
+        return context
     except Exception as e:
         logger.exception(e)
         logger.error("Error in Accept of New Member in Group request.")