Merge pull request #2810 from alphagov/dont-edit-users-from-other-services

Don’t allow editing of users from other services

app/main/views/manage_users.py

@@ -77,9 +77,7 @@ def invite_user(service_id):
 @user_has_permissions('manage_service')
 def edit_user_permissions(service_id, user_id):
     service_has_email_auth = current_service.has_permission('email_auth')
-    # TODO we should probably using the service id here in the get user
-    # call as well. eg. /user/<user_id>?&service=service_id
-    user = user_api_client.get_user(user_id)
+    user = current_service.get_team_member(user_id)
     user_has_no_mobile_number = user.mobile_number is None
 
     form = PermissionsForm.from_user(user, service_id)
@@ -106,7 +104,7 @@ def edit_user_permissions(service_id, user_id):
 @login_required
 @user_has_permissions('manage_service')
 def remove_user_from_service(service_id, user_id):
-    user = user_api_client.get_user(user_id)
+    user = current_service.get_team_member(user_id)
     form = PermissionsForm.from_user(user, service_id)
 
     if request.method == 'POST':
@@ -135,11 +133,11 @@ def remove_user_from_service(service_id, user_id):
     )
 
 
-@main.route("/services/<service_id>/users/<user_id>/edit-email", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/users/<uuid:user_id>/edit-email", methods=['GET', 'POST'])
 @login_required
 @user_has_permissions('manage_service')
 def edit_user_email(service_id, user_id):
-    user = user_api_client.get_user(user_id)
+    user = current_service.get_team_member(user_id)
     user_email = user.email_address
 
     def _is_email_already_in_use(email):
@@ -163,11 +161,11 @@ def edit_user_email(service_id, user_id):
     )
 
 
-@main.route("/services/<service_id>/users/<user_id>/edit-email/confirm", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/users/<uuid:user_id>/edit-email/confirm", methods=['GET', 'POST'])
 @login_required
 @user_has_permissions('manage_service')
 def confirm_edit_user_email(service_id, user_id):
-    user = user_api_client.get_user(user_id)
+    user = current_service.get_team_member(user_id)
     if 'team_member_email_change' in session:
         new_email = session['team_member_email_change']
     else:
@@ -178,7 +176,7 @@ def confirm_edit_user_email(service_id, user_id):
         ))
     if request.method == 'POST':
         try:
-            user_api_client.update_user_attribute(user_id, email_address=new_email)
+            user_api_client.update_user_attribute(str(user_id), email_address=new_email)
         except HTTPError as e:
             if e.status_code == 403:
                 flash("You don't have permission to edit users emails for this service", 'info')
@@ -202,9 +200,9 @@ def confirm_edit_user_email(service_id, user_id):
     )
 
 
-@main.route("/services/<service_id>/cancel-invited-user/<invited_user_id>", methods=['GET'])
+@main.route("/services/<service_id>/cancel-invited-user/<uuid:invited_user_id>", methods=['GET'])
 @user_has_permissions('manage_service')
 def cancel_invited_user(service_id, invited_user_id):
-    invite_api_client.cancel_invited_user(service_id=service_id, invited_user_id=invited_user_id)
+    current_service.cancel_invite(invited_user_id)
 
     return redirect(url_for('main.manage_users', service_id=service_id))

app/models/service.py

@@ -98,13 +98,18 @@ class Service():
     def has_jobs(self):
         return job_api_client.has_jobs(self.id)
 
+    @cached_property
+    def invited_users(self):
+        return invite_api_client.get_invites_for_service(service_id=self.id)
+
+    @cached_property
+    def active_users(self):
+        return user_api_client.get_users_for_service(service_id=self.id)
+
     @cached_property
     def team_members(self):
         return sorted(
-            (
-                invite_api_client.get_invites_for_service(service_id=self.id) +
-                user_api_client.get_users_for_service(service_id=self.id)
-            ),
+            self.invited_users + self.active_users,
             key=lambda user: user.email_address.lower(),
         )
 
@@ -114,6 +119,23 @@ class Service():
             self.id, 'manage_service'
         ) > 1
 
+    def cancel_invite(self, invited_user_id):
+
+        if str(invited_user_id) not in {user.id for user in self.invited_users}:
+            abort(404)
+
+        return invite_api_client.cancel_invited_user(
+            service_id=self.id,
+            invited_user_id=str(invited_user_id),
+        )
+
+    def get_team_member(self, user_id):
+
+        if str(user_id) not in {user.id for user in self.active_users}:
+            abort(404)
+
+        return user_api_client.get_user(user_id)
+
     @cached_property
     def all_templates(self):