Python: separation of concern User Model and User DB

Question

I am working on a small side-project since a couple of weeks, I am using Flask but trying to use as few libraries as possible and no ORMs (for learning purposes.

I am currently working on the User service on my application and currently stuck in the registration email confirmation part regarding my design.

Since I am not using any ORM, I wrote those classes:

• UserRepository: interact with the user table in the database (add, get, delete user and save)
• UserService: actions on a User (registration, deactivate user, update stats)
• User: representation of a User entity, no methods, methods are in the UserService (most of them take a User as a parameter expect the one to create a new User)

Current issue:

When a new user register, I send a registration email with a URL link to activate the account. I am using URLSafeTimedSerializer to do that and dump the user.email to generate the account confirmation URL. The email is sent and when the user click on the link, I am able to decrypt the user.email using URLSafeTimedSerializer.loads(). But with my current design, I am not quite confident how I am going to retrieve the user.email. Maybe the UserService class should take the UserRepository as an argument?

user/views.py

user = Blueprint('user', __name__, template_folder='templates')
# ----------
# STUCK HERE 
# ----------
@user.route('/activate/<activation_token>', methods=["GET"])
def activate(activation_token):
 private_key = current_app.config['SECRET_KEY']
 s = URLSafeTimedSerializer(private_key)
 user_email = s.loads(activation_token)
 # now I need to check if that user is already confirmed, if not I have to change current User confirmed value from from False to True
 # I should probably create a new UserService instance and create a method get_user_by_email(email) ?
 def generate_registration_token(email):
 private_key = current_app.config['SECRET_KEY']
 s = URLSafeTimedSerializer(private_key)
 u = s.dumps(email)
 return u
def send_registration_email(email):
 from prepsmarter.blueprints.user.tasks import deliver_contact_email
 token = generate_registration_token(email)
 host = current_app.config['HOST']
 activation_url = f"{host}/activate/{token}"
 try:
 deliver_contact_email(email, activation_url)
 return "ok"
 except Exception as e:
 return str(e)
 
@user.route('/register')
def login():
 return render_template('register.html')
@user.route('/new-user',methods = ['POST'])
def register_user():
 form_email = request.form.get('email')
 form_password = request.form.get('psw')
 form_password_repeat = request.form.get('psw-repeat')
 registration_form = RegistrationForm(form_email, form_password, form_password_repeat).validate_registration()
 if registration_form:
 new_user = UserService().register_user(form_email, form_password)
 user_repository = UserRepository(conn, 'users')
 user_repository.add_user(new_user)
 user_repository.save()
 send_registration_email(new_user.email)
 return "new user created" #will probably change return statements later on
 return "new uer not created" #will probably change return statements later on

user/models.py (User class)

class User():
 def __init__(self, email, password, registration_date, active, sign_in_count, current_sign_in_on, last_sign_in_on):
 self.email = email
 self.password = password
 self.registration_date = registration_date
 self.active = active
 self.confirmed = False

user/services.py (UserRepository)

class UserRepository():
 def __init__(self, conn, table):
 self.conn = conn
 self.table = table 
 
 #select exists(select 1 from users where email='[email protected]')
 def add_user(self, user):
 sql = "INSERT INTO users (email, password, is_active, sign_in_count, current_sign_in_on, last_sign_in_on) VALUES (%s, %s, %s, %s, %s, %s)"
 cursor = self.conn.cursor()
 # the is_active column in the DB is a tinyint(1). True = 1 and False = 0
 if user.active == True:
 is_active = 1
 is_active = 0
 cursor.execute(sql, ( user.email, user.password, is_active, user.sign_in_count, user.current_sign_in_on, user.last_sign_in_on))
 resp = cursor.fetchall()
 return resp
 
 def delete_user(self):
 return ""
 
 def get_user(self):
 return ""
 
 def save(self):
 self.conn.commit()
 

user/services.py (UserService)

class UserService():
 
 def register_user(self,
 email,
 password):
 sign_in_count = 1
 today_date = datetime.today().strftime('%Y-%m-%d')
 active = True
 new_user = User(email, password, today_date, active,
 sign_in_count, today_date, today_date)
 return new_user 
 
 def desactivate_user(self, User):
 if User.active == False:
 print(f"User {User.email} is already inactive")
 User.active = False
 
 def reactive_user(self, User):
 if User.active == True:
 print(f"User {User.email} is already active")
 User.active = True
 
 def is_active(self, User):
 return User.is_active

• Reinderien: Your formatting seems off - as written Python would fail to interpret this. Please double-check your indentation.
• Reinderien: @Peilonrayz I suspect that generate_registration_token is still off
• Peilonrayz: @Reinderien Right you are! Pierre you're going to have to double check generate_registration_token's indentation. Unfortunately we aren't allowed to edit the indentation, like I did for UserService.

Answer 1

It's unstated, so I have to assume that you're using itsdangerous and its implementation of URLSafeTimedSerializer .

You ask:

with my current design, I am not quite confident how I am going to retrieve the user.email

I'm not sure where the confusion lies. You say you already have the email, from this call:

user_email = s.loads(activation_token)

So maybe you're actually looking for a way to query the database by user email. Given your current patterns this would be done via your UserRepository class, possibly get_user - which currently looks to be an unimplemented stub.

Your repository class, at least in theory, should be fine; though your self.table is unused and I don't see a use for it even if all of your stubs were implemented.

add_user doesn't make sense. You send an INSERT, don't include a RETURNING clause, and then run a fetchall(). What do you expect this to return? You haven't indicated what your database is, nor even what your conn is, but I doubt that this is going to work.

UserService seems like a lot of boilerplate that can go away entirely. register_user can move those defaults to column-level default declarations right in the database. Even if you were to keep your default values in the application code, you should not be calling strftime when assigning a value to User.registration_date; leave it as a real date object.

I'd also be surprised if your database interaction library (that you haven't specified) isn't able to deal with booleans directly, and similarly for the database below it. Specifically, modern database like PostgreSQL do not require conversion to a 0/1 nor representation as a smallint, and can use true, false and boolean. The difference seems minor but better communicates your intent.

Everything else in the service class is trivial and doesn't offer any value over doing the boolean operations in place. The prints should be replaced with logging calls, and/or converted to meaningful HTTP response messages. desactivate is spelled deactivate.

I don't see any evidence that the password is getting hashed or salted when it goes to the database. Please ensure that this is the case.

from prepsmarter.blueprints.user.tasks import deliver_contact_email

deserves to be at the top of your file and not in a method.

The return value from send_registration_email is problematic. Returning an "ok" string or a string-ified exception goes counter to proper Python exception handling. It's less surprising to simply return None (i.e. not return at all), and if an exception happens, the caller needs to handle it. As written, the except type you've shown is too broad.

new uer is likely a typo.