14
\$\begingroup\$

I'm learning OOPs design principles and design patterns. As an exercise, I came up with this model for multiple user authentication using OOPs. I plan to build this on top of a Flask server, but I've just mocked the behavior for now.

Please let me know your thoughts about the implementation design. Thanks in advance.

User Model:

class User:
    def __init__(self, name: str = None, ph_no: str = None, username: str = None, password: str = None):
        self.username = username
        self.password = Utils.encrypt(password)
        self.ph_no = ph_no
        self.name = name

        Utils.persist_in_db(self)

    def update_password(self, old_password: str, new_password: str):
        if Utils.encrypt(old_password) == self.password:
            self.password = Utils.encrypt(new_password)
        else:
            raise PasswordNotCorrectException()

    def authenticate(self, password) -> bool:
        return self.password == Utils.encrypt(password)

    def __repr__(self):
        return f'<User: {OrderedDict(self.__dict__)}>'

Core logic:

import threading
import time
from login_service import LoginService
from models.user import User


class BMSApp:
    def __init__(self):
        self.mock()

    @staticmethod
    def mock():
        user1 = UserThread(name='Rick',
                             username='ricky',
                             ph_no='xxxxx',
                             password='password1234')

        user2 = UserThread(name='Morty',
                             username='morty',
                             ph_no='xxxx',
                             password='lifeisawesome')

        user1.start()
        user2.start()

        user1.join()
        user2.join()


class UserThread(threading.Thread):
    def __init__(self, **kwargs):
        super().__init__()
        self.kwargs = kwargs

    def run(self) -> None:
        BookingStrategy.execute(**self.kwargs)


class BookingStrategy:
    @staticmethod
    def execute(**kwargs):
        login_result = LoginService.login(**kwargs)
        if login_result[0]:
            print(f"Login succeeded for user {login_result[1]}")
        else:
            print(f"Login failed for user {login_result[1]}")

Mock Login service:

class LoginService:
    @staticmethod
    def login(**kwargs):
        #         TODO: implement proper login functionality
        user = User(**kwargs)
        return random.choice([(True, user), (False, user)])
\$\endgroup\$
  • \$\begingroup\$ Independent of whether this is good OOP, your password update function seems to expect that Utils,encrypt() is self-reversible. That would be a horrible choice for password safety! All serious password storage mechanisms go to great lengths to ensure passwords can not simply be decrypted. \$\endgroup\$ – Hans-Martin Mosner Dec 2 '19 at 5:54
  • \$\begingroup\$ Sorry, I meant to write Utils.encrypt(old_password) == self.password. Fixed it now. \$\endgroup\$ – Andrew Scott Dec 2 '19 at 7:12
  • \$\begingroup\$ That still does not consider salting. And the name encrypt() implies that there is a matching decrypt() function, and if there is, you're still not doing it right. \$\endgroup\$ – Hans-Martin Mosner Dec 2 '19 at 8:11
  • 1
    \$\begingroup\$ Considering there are answers posted now, please don't edit your code further or it will be considered invalidation of the current answers. The response to such will be a rollback of the edit. Consider posting a follow-up question linking back to this one if you have updated code that you'd like to have reviewed. \$\endgroup\$ – Mast Dec 2 '19 at 9:38
  • 1
    \$\begingroup\$ Would recommend you to read this book github.com/cosmicpython/book \$\endgroup\$ – Ignacio Vergara Kausel Dec 2 '19 at 10:00
8
\$\begingroup\$

First of all, I am not at all sure it is good idea to learn OOP by re-implementing security-sensitive feature for a web-framework, which already has support for such things. Please, at least consider using Flask-Security, Flask-Login, Flask-Principal unless you plan a total rewrite of all those. Also, I see plain text passwords in use - most of the time a security risk.

That said, I can find the following problems:

  1. Persistence service is too intrusive and its logic is not clear. For example, you are creating a user just to login: Why do you want to persist anything someone tries to enter as a login attempt? Consider using some ORM, where you can control object's persistence, or come up with something better than object dealing with its persistence.

    Those aspects better be invisible in the model. In Python, it's probably best to use a DTO (data-transfer object, received from the login form, usually a dict) until identity is verified.

  2. I do not really understand why you are using threads. In the Flask environment one can use a session as an abstraction, so dealing at thread level is strange and can make your code too dependent on a specific implementation, preventing porting it to multiprocessing, async/greenlets and the like.

There are some naming problems (what is ph_no?).

In my opinion the best place for OOP in user authentication is (in addition to User class) a possibility to define different authentication strategies (and inject dependencies like Utils.encrypt that way). Then, if a more complex system is in question - role-based authorization, etc, etc. The code you presented above is not geared towards those goals. Instead, it deals with low-level concerns which are already solved in the framework.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Encrypted, not plaintext. Although it should be hashed. Esp. since OP is treating it as a hash, ie. he is not decrypting the password. \$\endgroup\$ – Taemyr Dec 2 '19 at 9:11
  • \$\begingroup\$ Hmmm... Question was changed after my answer. I believe it was Utils.decrypt - so plain text password could be obtained. \$\endgroup\$ – Roman Susi Dec 2 '19 at 16:32
2
\$\begingroup\$

In general objects that the parameters of the constructors have dependencies, for example username with password with name, should be readonly in order to avoid changes during the execution, in python basically is make that parameters private and have a decorator that only allows read them. You will avoid issues such has create an object and after that change the username, or even the password.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.