abort on update to protected properties, some refactoring

Author: npalaska

lib/pbench/server/api/resources/users_api.py

@@ -2,7 +2,7 @@
 from flask import request, jsonify, make_response
 from flask_restful import Resource, abort
 from flask_bcrypt import check_password_hash
-from email_validator import validate_email, EmailNotValidError
+from email_validator import EmailNotValidError
 from sqlalchemy.exc import SQLAlchemyError, IntegrityError
 from configparser import NoOptionError
 from pbench.server.database.models.users import User
@@ -119,40 +119,19 @@ def post(self):
                 400, message="Missing lastName field",
             )
 
-        # validate the email field
-        try:
-            valid = validate_email(email_id)
-            email = valid.email
-        except EmailNotValidError:
-            self.logger.warning("Invalid email {}", email_id)
-            abort(400, message=f"Invalid email: {email_id}")
-
-        # check if user already exist
-        try:
-            user = User.query(username=user_data.get("username"))
-        except Exception:
-            self.logger.exception("Exception while querying user")
-            abort(500, message="INTERNAL ERROR")
-
-        if user:
-            self.logger.warning(
-                "A user tried to re-register. Username: {}", user.username
-            )
-            abort(403, message="A user with that name already exists.")
-
         try:
             user = User(
                 username=username,
                 password=password,
                 first_name=first_name,
                 last_name=last_name,
-                email=email,
+                email=email_id,
             )
 
             # insert the user
             user.add()
             self.logger.info(
-                "New user registered, username: {}, email: {}", username, email
+                "New user registered, username: {}, email: {}", username, email_id
             )
 
             response_object = {
@@ -162,6 +141,9 @@ def post(self):
             response = jsonify(response_object)
             response.status_code = 201
             return make_response(response, 201)
+        except EmailNotValidError:
+            self.logger.warning("Invalid email {}", email_id)
+            abort(400, message=f"Invalid email: {email_id}")
         except Exception:
             self.logger.exception("Exception while registering a user")
             abort(500, message="INTERNAL ERROR")
@@ -385,17 +367,27 @@ def get(self, username):
             abort(500, message="INTERNAL ERROR")
 
         # TODO: Check if the user has the right privileges
-        if verified or user.is_admin():
+        if verified:
+            return_data = user.get_json()
             response_object = {
                 "status": "success",
-                "data": {
-                    "email": user.email,
-                    "first_name": user.first_name,
-                    "last_name": user.last_name,
-                    "registered_on": user.registered_on,
-                },
+                "data": return_data,
             }
             return make_response(jsonify(response_object), 200)
+        elif user.is_admin():
+            try:
+                target_user = User.query(username=username)
+                return_data = target_user.get_json()
+                response_object = {
+                    "status": "success",
+                    "data": return_data,
+                }
+                return make_response(jsonify(response_object), 200)
+            except Exception:
+                self.logger.exception(
+                    "Exception occurred while querying the user. Username: {}", username
+                )
+                abort(500, message="INTERNAL ERROR")
         else:
             self.logger.warning(
                 "Username retrieved from the auth token {} is different from the username provided",
@@ -448,23 +440,28 @@ def put(self, username):
 
         # TODO: Check if the user has the right privileges
         if verified:
-            try:
-                # Log if the user payload contain fields that are either non-updatabale or
-                # are not present in the user db.
-                non_updatable = list(
-                    set(post_data.keys()) - set(User.__table__.columns.keys())
+            # Check if the user payload contain fields that are either non-updatabale or
+            # are not present in the user db. If any key in the payload does not match
+            # with the column name we will abort the update request.
+            non_updatable = list(
+                set(post_data.keys()) - set(User.__table__.columns.keys())
+            )
+            if non_updatable:
+                self.logger.warning(
+                    "User trying to update fields that are not present in the user database. Fields: {}",
+                    non_updatable,
                 )
-                if "registered_on" in post_data.keys():
-                    non_updatable.append("registered_on")
-                if non_updatable:
-                    self.logger.warning(
-                        "User trying to update fields that are either non-updatable or does not present in the user database. Fields: {}",
-                        non_updatable,
-                    )
+                abort(400, message="Invalid fields in update request payload")
+            try:
                 # We will update the user object with the keys and values provided in the request payload.
-                # THe keys need to match with the column names in the user model. However, if any key in
-                # the payload does not match with the column name we just skip that field.
+                # THe keys need to match with the column names in the user model.
                 user.update(**post_data)
+            except ValueError:
+                self.logger.warning(
+                    "Either provided values to update the user does not adhere to the user model "
+                    "datatype or user attempted to update the protected properties."
+                )
+                abort(400, message="Invalid fields in update request payload")
             except Exception:
                 self.logger.exception("Exception occurred during updating user object")
                 abort(500, message="INTERNAL ERROR")
@@ -477,15 +474,10 @@ def put(self, username):
                 403, message="Authentication token does not belong to the current user"
             )
 
+        return_data = user.get_json()
         response_object = {
             "status": "success",
-            "data": {
-                "username": user.username,
-                "email": user.email,
-                "first_name": user.first_name,
-                "last_name": user.last_name,
-                "registered_on": user.registered_on,
-            },
+            "data": return_data,
         }
         return make_response(jsonify(response_object), 200)
 

lib/pbench/server/database/models/users.py

@@ -1,8 +1,10 @@
 import datetime
 from flask_bcrypt import generate_password_hash
+from email_validator import validate_email
 from sqlalchemy import Column, Integer, String, DateTime, LargeBinary
 from pbench.server.database.database import Database
 from sqlalchemy.orm import relationship
+from sqlalchemy.orm import validates
 
 
 class User(Database.Base):
@@ -31,17 +33,27 @@ def __init__(self, **kwargs):
     def __str__(self):
         return f"User, id: {self.id}, username: {self.username}"
 
+    def get_json(self):
+        return {
+            "username": self.username,
+            "email": self.email,
+            "first_name": self.first_name,
+            "last_name": self.last_name,
+            "registered_on": self.registered_on,
+        }
+
     @staticmethod
-    def query(**kwargs):
+    def query(id=None, username=None, email=None):
         # Currently we would only query with single argument. Argument need to be either username/id/email
         try:
-            key, value = next(iter(kwargs.items()))
-            if key == "username":
-                user = Database.db_session.query(User).filter_by(username=value).first()
-            elif key == "id":
-                user = Database.db_session.query(User).filter_by(id=value).first()
-            elif key == "email":
-                user = Database.db_session.query(User).filter_by(email=value).first()
+            if username:
+                user = (
+                    Database.db_session.query(User).filter_by(username=username).first()
+                )
+            elif id:
+                user = Database.db_session.query(User).filter_by(id=id).first()
+            elif email:
+                user = Database.db_session.query(User).filter_by(email=email).first()
             else:
                 user = None
         except Exception as e:
@@ -60,6 +72,25 @@ def add(self):
             Database.db_session.rollback()
             raise e
 
+    # Prevent update on "registered_on" field
+    @validates("registered_on")
+    def evaluate_registered_on(self, key, value):
+        if self.registered_on:  # Field already exists
+            if self.registered_on.strftime("%Y-%m-%d %H:%M:%S.%f") != value:
+                raise ValueError("registered_on cannot be modified.")
+        return value
+
+    # validate the email field
+    @validates("email")
+    def evaluate_email(self, key, value):
+        try:
+            valid = validate_email(value)
+            email = valid.email
+        except Exception as e:
+            raise e
+        else:
+            return email
+
     def update(self, **kwargs):
         """
         Update the current user object with given keyword arguments
@@ -72,9 +103,6 @@ def update(self, **kwargs):
                     Database.db_session.add(value)
                 elif key == "password":
                     setattr(self, key, generate_password_hash(value))
-                # Prevent update on "registered_on" field
-                elif key == "registered_on":
-                    continue
                 else:
                     setattr(self, key, value)
             Database.db_session.commit()

lib/pbench/test/unit/server/test_user_auth.py

@@ -161,34 +161,26 @@ def test_user_relogin(client, server_config):
             assert response.content_type == "application/json"
             assert response.status_code == 200
 
-            # Immediate re-login with auth header
+            # Re-login with auth header
+            time.sleep(1)
             response = client.post(
                 f"{server_config.rest_uri}/login",
                 headers=dict(Authorization="Bearer " + data["auth_token"]),
                 json={"username": "user", "password": "12345"},
             )
             data = response.json
-            assert data["message"] == "Retry login after some time"
-            assert response.status_code == 403
+            assert data["message"] == "Successfully logged in."
+            assert response.status_code == 200
 
-            # Re-login after a second
+            # Re-login without auth header
             time.sleep(1)
             response = client.post(
                 f"{server_config.rest_uri}/login",
                 json={"username": "user", "password": "12345"},
             )
             data = response.json
-            assert data["auth_token"]
-            assert data["status"] == "success"
-
-            # Immediate re-login without auth header
-            response = client.post(
-                f"{server_config.rest_uri}/login",
-                json={"username": "user", "password": "12345"},
-            )
-            data = response.json
-            assert data["message"] == "Retry login after some time"
-            assert response.status_code == 409
+            assert data["message"] == "Successfully logged in."
+            assert response.status_code == 200
 
     @staticmethod
     def test_user_login_with_wrong_password(client, server_config):
@@ -261,6 +253,8 @@ def test_get_user(client, server_config):
             assert data["status"] == "success"
             assert data["data"] is not None
             assert data["data"]["email"] == "[email protected]"
+            assert data["data"]["username"] == "username"
+            assert data["data"]["first_name"] == "firstname"
             assert response.status_code == 200
 
     @staticmethod
@@ -291,19 +285,20 @@ def test_update_user(client, server_config):
                 headers=dict(Authorization="Bearer " + data_login["auth_token"]),
             )
             data = response.json
-            assert data["status"] == "success"
-            assert data["data"] is not None
-            assert data["data"]["first_name"] == "newname"
-            # registered_on is not updatable
-            assert data["data"]["registered_on"] != new_registration_time
-            assert response.status_code == 200
+            assert response.status_code == 400
+            assert data["message"] == "Invalid fields in update request payload"
 
             # Test password update
             response = client.put(
                 f"{server_config.rest_uri}/user/username",
                 json={"password": "newpass"},
                 headers=dict(Authorization="Bearer " + data_login["auth_token"]),
             )
+            data = response.json
+            assert response.status_code == 200
+            assert data["status"] == "success"
+            assert data["data"]["first_name"] == "firstname"
+            assert data["data"]["email"] == "[email protected]"
             time.sleep(1)
             response = login_user(client, server_config, "username", "newpass")
             data_login = response.json