From bc84a7723a9d54e1dfe2228bbaaa23efd4ced2c3 Mon Sep 17 00:00:00 2001 From: tfreville Date: Wed, 5 May 2021 00:49:56 +0200 Subject: [PATCH 1/2] feat!: Rework code architecture and propose an hello world. - Current code architecture is too heavy for open source project and do not let people easily in if they are not used to it. We decided to refactor it to a simpler base witch can be learned without pre requisites and let you code using your own habits. - Enforce some tools to format code - Add CONTRIBUTING rules and Makefile to ease setup, running && testing This proposition looks a bit heavy for a simple hello world but it should stay clean with more complex usecases. It enforces concern separation as discord part should only filter event before calling correct module entrypoint, dissociate services from modules and allow core feature to stay core ^^ --- Makefile | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 1a15b31..2926379 100644 --- a/Makefile +++ b/Makefile @@ -4,16 +4,19 @@ export ################### # Setup tasks # ################### +install_dev: + pip install -e ".[dev]" + +install: + pip install . + setup_venv: python3.9 -m venv venv . venv/bin/activate - pip install . -setup_dev: setup_venv - pip install -e ".[dev]" +setup_dev: setup_venv install_dev -setup: - pip3 install . +setup: setup_venv install ################### # Testing # From 18a03a643a77ff521d6f7d279210c3e16d848770 Mon Sep 17 00:00:00 2001 From: Titouan Freville Date: Wed, 5 May 2021 18:24:38 +0200 Subject: [PATCH 2/2] feat: Add welcome message As discord server admin, we would like to propose a custom message to welcome any new member. - Add an admin role to discord guild on bot startup - Bot admin can customise message More: - Add bandit check for security issue --- .github/workflows/main.yaml | 23 +++ Makefile | 11 +- README.md | 1 + app/core/roles.py | 27 +++ app/core/sqlalchemy.py | 3 + app/dependencies.py | 1 + app/discord/bot.py | 12 +- app/discord/events.py | 15 ++ app/discord/messages.py | 38 ++-- app/modules/hello.py | 11 -- app/modules/messaging/__init__.py | 3 + app/modules/messaging/_api.py | 57 ++++++ app/modules/messaging/_queries.py | 0 app/modules/messaging/api.py | 47 +++++ app/modules/messaging/usecases.py | 54 ++++++ config.ini | 3 +- setup.py | 16 +- .../{messages_test.py => messages_sample.py} | 0 tests/mocks/discord.py | 38 ++++ tests/mocks/modules/messaging/api_mock.py | 23 +++ tests/modules/messaging/test_usecases.py | 167 ++++++++++++++++++ tests/modules/messaging/test_validator.py | 13 ++ tests/modules/test_hello.py | 33 ---- 23 files changed, 529 insertions(+), 67 deletions(-) create mode 100644 app/core/roles.py create mode 100644 app/core/sqlalchemy.py create mode 100644 app/dependencies.py create mode 100644 app/discord/events.py delete mode 100644 app/modules/hello.py create mode 100644 app/modules/messaging/__init__.py create mode 100644 app/modules/messaging/_api.py create mode 100644 app/modules/messaging/_queries.py create mode 100644 app/modules/messaging/api.py create mode 100644 app/modules/messaging/usecases.py rename tests/discord/{messages_test.py => messages_sample.py} (100%) create mode 100644 tests/mocks/modules/messaging/api_mock.py create mode 100644 tests/modules/messaging/test_usecases.py create mode 100644 tests/modules/messaging/test_validator.py delete mode 100644 tests/modules/test_hello.py diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 12b1545..ad3d22c 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -9,6 +9,7 @@ on: types: [ opened, synchronize, reopened ] paths-ignore: - '**.md' + jobs: lint: name: lint @@ -31,6 +32,28 @@ jobs: - name: lint run: make lint + security: + name: security + runs-on: ubuntu-latest + steps: + - name: checkout + uses: actions/checkout@v2 + + - name: setup python + uses: actions/setup-python@v2 + with: + python-version: '3.9' + + - name: setup env + run: cp .env.sample .env + + - name: install dependencies + run: make setup_dev + + - name: bandit + run: make bandit-ci + + test: name: tests runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 2926379..086b455 100644 --- a/Makefile +++ b/Makefile @@ -29,7 +29,16 @@ lint: mypy main.py app tests cover: - coverage run --source=app -m pytest -x . + coverage run --source=app -m pytest -xv . + +coverage-report: cover + coverage report -m + +bandit: + bandit -r app main.py + +bandit-ci: + bandit -r -ll -ii app main.py test-all: lint cover diff --git a/README.md b/README.md index 4c319b1..aa4eda9 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ ![CI Status](https://github.com/devcherchecollegue-org/devguy-python/actions/workflows/main.yaml/badge.svg?branch=main) [![Coverage Status](https://coveralls.io/repos/github/devcherchecollegue-org/devguy-python/badge.svg?branch=main)](https://coveralls.io/github/devcherchecollegue-org/devguy-python?branch=main) +[![security: bandit](https://img.shields.io/badge/security-bandit-yellow.svg)](https://github.com/PyCQA/bandit) diff --git a/app/core/roles.py b/app/core/roles.py new file mode 100644 index 0000000..5ddc011 --- /dev/null +++ b/app/core/roles.py @@ -0,0 +1,27 @@ +from pydantic import BaseModel +from discord import Guild, colour +from typing import Optional, List + + +class Role(BaseModel): + color: Optional[str] = None + name: str + + async def create(self, guild: Guild): + if self.color: + return guild.create_role( + name=self.name, + colour=colour.Colour(f"0x{self.color}"), + ) + + return guild.create_role(name=self.name) + + +def requires_role( + role: Role, in_roles: List[Role], err: Exception = Exception("forbiden") +): + if all(role.name != candidate.name for candidate in in_roles): + raise err + + +ADMIN = Role(name="devguy_admin") diff --git a/app/core/sqlalchemy.py b/app/core/sqlalchemy.py new file mode 100644 index 0000000..59be703 --- /dev/null +++ b/app/core/sqlalchemy.py @@ -0,0 +1,3 @@ +from sqlalchemy.orm import declarative_base + +Base = declarative_base() diff --git a/app/dependencies.py b/app/dependencies.py new file mode 100644 index 0000000..67edcfe --- /dev/null +++ b/app/dependencies.py @@ -0,0 +1 @@ +deps = {} diff --git a/app/discord/bot.py b/app/discord/bot.py index 7a351c8..0196abe 100644 --- a/app/discord/bot.py +++ b/app/discord/bot.py @@ -1,7 +1,6 @@ from app.discord import client -# flake8: noqa -from app.discord import messages # load messages +from app.discord import messages, events # This file cannot be tested through unit testing cause it requires to start a true discord application. # It could be done through dependencie injection using a mocked discord instance then starting though but @@ -9,14 +8,11 @@ def start(token: str): # pragma: no-cover + events.Events(None) # TODO: inject messenger + messages.Message(None) client.run(token) -@client.event +@client.event # pragma: no-cover async def on_ready(): # pragma: no-cover print(f"We have logged in as {client.user}") - - -@client.event -async def on_connect(): # pragma: no-cover - print("connected") diff --git a/app/discord/events.py b/app/discord/events.py new file mode 100644 index 0000000..25a97fa --- /dev/null +++ b/app/discord/events.py @@ -0,0 +1,15 @@ +from discord import User + +from app.discord import client +from app.modules.messaging import Messenger + + +class Events: + def __init__(self, messenger: Messenger): + self.__messenger = messenger + + @client.event + async def on_member_join(self, member: User): + msg, channel_id = self.__messenger.welcome(member.name) + chan = client.get_channel(channel_id) + await chan.send(msg) diff --git a/app/discord/messages.py b/app/discord/messages.py index 2f686ac..e211f0a 100644 --- a/app/discord/messages.py +++ b/app/discord/messages.py @@ -1,14 +1,30 @@ -from discord import Message -from discord import Client from app.discord import client -from app.modules import hello +from app.modules.messaging import Messenger +from discord import Message + + +class Messages: + def __init__(self, messenger: Messenger, prefix: str = "!devguy"): + self.__messenger = messenger + self.__prefix = prefix + + def __is_cmd(self, message: str): + if message.startswith(self.__prefix): + splitted = message.split(" ") + return True, splitted[0], splitted[1:] + + return False, None + + @client.event + async def on_message(self, message: Message): + is_cmd, cmd, args = self.__is_cmd(message.content) + if not is_cmd: + return + if cmd == "set_as_welcome_channel": + self.__messenger.set_welcome_channel(message.channel.id, message.author) + return -@client.event -async def on_message( - message: Message, - usecase: hello.Usecase = hello.Usecase(), - cli: Client = client, -): - if message.content.startswith("hello"): - await usecase.respond(message, cli.user.name) + if cmd == "set_welcome_message": + self.__messenger.set_welcome_message(" ".join(args), message.author) + return diff --git a/app/modules/hello.py b/app/modules/hello.py deleted file mode 100644 index 09ecdad..0000000 --- a/app/modules/hello.py +++ /dev/null @@ -1,11 +0,0 @@ -from discord import Message - - -class Usecase: - @staticmethod - def respond(message: Message, owner: str): - if message.author.nick == owner: - print("Bot speaking here") - return - - return message.channel.send("Hello!") diff --git a/app/modules/messaging/__init__.py b/app/modules/messaging/__init__.py new file mode 100644 index 0000000..ca78783 --- /dev/null +++ b/app/modules/messaging/__init__.py @@ -0,0 +1,3 @@ +from .usecases import Messenger + +__all__ = ["Messenger"] diff --git a/app/modules/messaging/_api.py b/app/modules/messaging/_api.py new file mode 100644 index 0000000..a4f3b85 --- /dev/null +++ b/app/modules/messaging/_api.py @@ -0,0 +1,57 @@ +from abc import abstractmethod, ABC +from app.core.sqlalchemy import Base +from .api import Kind, Validator, Messages +from string import Formatter +from sqlalchemy.orm import Column, Integer, Varchar, Datetime +from datetime import datetime + + +class _Validator(Validator): + def is_welcome(self, message: str) -> bool: + args = [tup[1] for tup in Formatter().parse(message) if tup[1] is not None] + return args == self.WELCOME_ARGS + + +class _MessageEntity(Base): + __tablename__ = "messages" + + id = Column(Integer, primary_key="true", autoincrement=True) + kind = Column(Integer, nullable=False) + content = Column(Varchar(500), nullable=False) + + created_at = Column(Datetime, nullable=False, default=datetime.utcnow) + last_updated_at = Column(Datetime, nullable=False, default=datetime.utcnow) + + +class _MessageQueries(ABC): + @abstractmethod + def insert(self, message: _MessageEntity): + """Insert message into DB""" + + @abstractmethod + def update(self, message: _MessageEntity): + """Update message into DB""" + + @abstractmethod + def get(self, message_kind: Kind): + """Get current message for provided kind""" + + +class _Messages(Messages): + def __init__(self, querier: _MessageQueries): + self.__query = querier + + def save(self, kind: Kind, content: str): + msg = None + try: + msg = self.__query.get(kind) + except Exception as e: + print(e) + return False + + if not msg: + msg = _MessageEntity(kind=kind.value, content=content) + return self.__query.insert(msg) + + msg.content = content + return self.__query.update(msg) diff --git a/app/modules/messaging/_queries.py b/app/modules/messaging/_queries.py new file mode 100644 index 0000000..e69de29 diff --git a/app/modules/messaging/api.py b/app/modules/messaging/api.py new file mode 100644 index 0000000..a051e98 --- /dev/null +++ b/app/modules/messaging/api.py @@ -0,0 +1,47 @@ +from abc import ABC, abstractmethod +from enum import Enum +from typing import Optional + + +class Kind(Enum): + WELCOME = 0 + + +class Validator(ABC): + WELCOME_ARGS = ["new_member_name"] + + @abstractmethod + def is_welcome(self, message: str) -> bool: + """ + Ensure provided message is a correctly + formatted welcome one. + """ + + +class Messages(ABC): + @abstractmethod + def save(self, kind: Kind, msg: str) -> bool: + """ + Store message with correct kind. + Variables has to be passed using the + python format syntax. + """ + + @abstractmethod + def welcome(self) -> str: + """ + Retrieve stored welcome message if exits. + """ + + +class Channels(ABC): + @abstractmethod + def save(self, kind: Kind, id: int) -> bool: + """ + Store channel id with correct kind. + Those channel will be used for announcement. + """ + + @abstractmethod + def welcome(self) -> Optional[int]: + """Retrieve stored channel for welcome message""" diff --git a/app/modules/messaging/usecases.py b/app/modules/messaging/usecases.py new file mode 100644 index 0000000..35374ca --- /dev/null +++ b/app/modules/messaging/usecases.py @@ -0,0 +1,54 @@ +from .api import Kind, Messages, Validator, Channels +from discord.member import Member +from app.core import roles +from typing import Tuple + + +class Messenger: + class InvalidMessage(Exception): + def __init__(self, kind: Kind): + super().__init__(f"candidate is not a valid {kind.name} message.") + + class ForbidenAction(Exception): + pass + + class NoChannelDefined(Exception): + def __init__(self, kind: Kind): + super().__init__(f"no channel define to announce {kind.name}.") + + def __init__(self, messages: Messages, channels: Channels, validator: Validator): + self.__messages = messages + self.__validator = validator + self.__channels = channels + + def set_welcome_channel(self, channel_id: int, user: Member) -> None: + roles.requires_role(roles.ADMIN, user.roles, self.ForbidenAction) + + if not self.__channels.save(Kind.WELCOME, channel_id): + raise Exception("could not save welcome channel") + + def set_welcome_message(self, message: str, user: Member) -> None: + roles.requires_role(roles.ADMIN, user.roles, self.ForbidenAction) + + if not self.__validator.is_welcome(message): + raise self.InvalidMessage(Kind.WELCOME) + + if not self.__messages.save(Kind.WELCOME, message): + raise Exception("could not save welcome message") + + def welcome(self, new_member_name: str) -> Tuple[str, int]: + """Welcome a new user""" + channel_id = self.__channels.welcome() + if not channel_id: + raise self.NoChannelDefined(Kind.WELCOME) + + msg = self.__messages.welcome() + if not msg: + msg = ( + f"Hello {{{self.__validator.WELCOME_ARGS[0]}}} et bienvenue dans " + "la communauté! Présente toi ma gueule ;) !" + ) + + format_args = {self.__validator.WELCOME_ARGS[0]: new_member_name} + + return (msg.format(**format_args), channel_id) diff --git a/config.ini b/config.ini index 0b93a22..9f349a9 100644 --- a/config.ini +++ b/config.ini @@ -1,2 +1,3 @@ [bot] -token= %(discord_token)s \ No newline at end of file +token= %(discord_token)s +prefix= "!devguy " \ No newline at end of file diff --git a/setup.py b/setup.py index 6f6283c..2b8dc1e 100644 --- a/setup.py +++ b/setup.py @@ -6,8 +6,20 @@ packages=find_packages(), include_package_data=True, zip_safe=False, - install_requires=["discord"], + install_requires=[ + "discord", + "pydantic", + "sqlalchemy", + ], extras_require={ - "dev": ["flake8", "coverage", "mypy", "black", "pytest", "elmock", "pydantic"], + "dev": [ + "black", + "bandit", + "coverage", + "elmock", + "flake8", + "mypy", + "pytest", + ], }, ) diff --git a/tests/discord/messages_test.py b/tests/discord/messages_sample.py similarity index 100% rename from tests/discord/messages_test.py rename to tests/discord/messages_sample.py diff --git a/tests/mocks/discord.py b/tests/mocks/discord.py index b24ab77..ae754fb 100644 --- a/tests/mocks/discord.py +++ b/tests/mocks/discord.py @@ -4,6 +4,11 @@ class MockAuthor(Mock): def __init__(self, nickname: str): self.nick = nickname + super().__init__() + + def reset(self): + self.nick = "" + super().reset() class MockChannel(Mock): @@ -17,8 +22,15 @@ def __init__(self, content: str = "", channel=None, author=None): self.channel = channel self.author = author + super().__init__() + def set_content(self, content: str): self.content = content + return self + + def reset(self): + self.content = "" + super().reset() class MockUser(Mock): @@ -29,3 +41,29 @@ def __init__(self, name: str): class MockClient(Mock): def __init__(self, user): self.user = user + + +class MockRole: + def __init__(self): + self.name = "" + + def set_name(self, name): + self.name = name + return self + + def reset(self): + self.name = "" + + +class MockMembers(Mock): + def __init__(self): + self.roles = [] + super().__init__() + + def set_roles(self, roles): + self.roles = roles + return self + + def reset(self): + self.roles = [] + super().reset() diff --git a/tests/mocks/modules/messaging/api_mock.py b/tests/mocks/modules/messaging/api_mock.py new file mode 100644 index 0000000..04340da --- /dev/null +++ b/tests/mocks/modules/messaging/api_mock.py @@ -0,0 +1,23 @@ +from elmock import Mock +from app.modules.messaging.api import Messages, Kind, Validator, Channels + + +class MockValidator(Mock, Validator): + def is_welcome(self, message: str) -> bool: + return self.execute("is_welcome", message) + + +class MockMessages(Mock, Messages): + def save(self, kind: Kind, msg: str) -> bool: + return self.execute("save", kind, msg) + + def welcome(self) -> str: + return self.execute("welcome") + + +class MockChannels(Mock, Channels): + def save(self, kind: Kind, channel_id: int) -> bool: + return self.execute("save", kind, channel_id) + + def welcome(self) -> int: + return self.execute("welcome") diff --git a/tests/modules/messaging/test_usecases.py b/tests/modules/messaging/test_usecases.py new file mode 100644 index 0000000..97b168a --- /dev/null +++ b/tests/modules/messaging/test_usecases.py @@ -0,0 +1,167 @@ +from app.modules.messaging.api import Kind +from app.modules.messaging import Messenger +from pytest import fixture, fail, raises +from tests.mocks.modules.messaging.api_mock import ( + MockMessages, + MockValidator, + MockChannels, +) +from tests.mocks.discord import MockRole, MockMembers +from app.core import roles + +validator = MockValidator() +messages = MockMessages() +channels = MockChannels() + + +@fixture +def admin_member(): + member_roles = [ + MockRole().set_name("Luna"), + MockRole().set_name("Test"), + MockRole().set_name(roles.ADMIN.name), + ] + member = MockMembers().set_roles(member_roles) + yield member + member.reset() + + +@fixture +def non_admin_member(): + member_roles = [ + MockRole().set_name("Neville"), + MockRole().set_name("Hell"), + ] + member = MockMembers().set_roles(member_roles) + yield member + member.reset() + + +@fixture(autouse=True) +def cleanup(): + yield + + validator.assert_full_filled() + messages.assert_full_filled() + channels.assert_full_filled() + + validator.reset() + messages.reset() + channels.reset() + + +class TestMessenger: + messenger = Messenger(messages=messages, validator=validator, channels=channels) + + class TestSetWelcomeMessage: + def test_valid_message(self, admin_member): + msg = "This is a test message for {}" + + validator.on("is_welcome", msg).returns(True) + messages.on("save", Kind.WELCOME, msg).returns(True) + + try: + TestMessenger.messenger.set_welcome_message(msg, admin_member) + + except (Messenger.ForbidenAction, Messenger.InvalidMessage): + fail("should not raise Forbiden nor Invalid") + except Exception as e: + if str(e) == "could not save welcome message": + fail("should not raise Save error") + + raise e + + def test_raises_forbidden_error_on_non_admin(self, non_admin_member): + msg = "This is a test message for {}" + + with raises(Messenger.ForbidenAction): + TestMessenger.messenger.set_welcome_message( + msg, + non_admin_member, + ) + + def test_raises_on_invalid_message(self, admin_member): + msg = "This is a wront test message for" + + validator.on("is_welcome", msg).returns(False) + + with raises(Messenger.InvalidMessage): + TestMessenger.messenger.set_welcome_message(msg, admin_member) + + def test_raises_on_save_failure(self, admin_member): + msg = "This is a wront test message for" + + validator.on("is_welcome", msg).returns(True) + messages.on("save", Kind.WELCOME, msg).returns(False) + + with raises(Exception): + TestMessenger.messenger.set_welcome_message(msg, admin_member) + + class TestSetWelcomeChannel: + def test_insert(self, admin_member): + channels.on("save", Kind.WELCOME, 10).returns(True) + + try: + TestMessenger.messenger.set_welcome_channel(10, admin_member) + + except (Messenger.ForbidenAction): + fail("should not raise Forbiden nor Invalid") + except Exception as e: + if str(e) == "could not save welcome message": + fail("should not raise Save error") + + raise e + + def test_raises_forbidden_error_on_non_admin(self, non_admin_member): + + with raises(Messenger.ForbidenAction): + TestMessenger.messenger.set_welcome_channel(10, non_admin_member) + + def test_raises_on_save_failure(self, admin_member): + channels.on("save", Kind.WELCOME, 10).returns(False) + + with raises(Exception): + TestMessenger.messenger.set_welcome_channel(10, admin_member) + + class TestGetWelcomeString: + def test_default(self): + usr = "test user" + + messages.on("welcome").returns(None) + channels.on("welcome").returns(10) + + msg, channel = TestMessenger.messenger.welcome(usr) + + assert f"Hello {usr}" in msg + assert channel == 10 + + messages.reset() + messages.on("welcome").returns("") + + msg, channel = TestMessenger.messenger.welcome(usr) + + assert f"Hello {usr}" in msg + assert channel == 10 + + def test_from_saved_message(self): + msg = "Hello {new_member_name} from test" + usr = "test user" + expected = msg.format(new_member_name=usr) + + messages.on("welcome").returns(msg) + channels.on("welcome").returns(10) + + msg, channel = TestMessenger.messenger.welcome(usr) + + assert expected == msg + assert channel == 10 + + def test_raise_when_no_channel_defined(self): + msg = "Hello {new_member_name} from test" + usr = "test user" + + messages.on("welcome").returns(msg) + channels.on("welcome").returns(None) + + with raises(Messenger.NoChannelDefined): + TestMessenger.messenger.welcome(usr) diff --git a/tests/modules/messaging/test_validator.py b/tests/modules/messaging/test_validator.py new file mode 100644 index 0000000..3da5cd6 --- /dev/null +++ b/tests/modules/messaging/test_validator.py @@ -0,0 +1,13 @@ +from app.modules.messaging._api import _Validator + + +class TestValidator: + validator = _Validator() + + def test_validate_correct_welcome_message(self): + assert self.validator.is_welcome("Test for {new_member_name}") is True + + def test_invalidate_incorrect_welcome_message(self): + assert self.validator.is_welcome("Test for {}") is False + assert self.validator.is_welcome("Test message") is False + assert self.validator.is_welcome("Test for {not_ok}") is False diff --git a/tests/modules/test_hello.py b/tests/modules/test_hello.py deleted file mode 100644 index 2bf6af7..0000000 --- a/tests/modules/test_hello.py +++ /dev/null @@ -1,33 +0,0 @@ -from app.modules import hello -from pytest import fixture -from tests.mocks.discord import MockMessage, MockAuthor, MockChannel - -author = MockAuthor("test") -channel = MockChannel() -message = MockMessage(channel=channel, author=author) - - -@fixture -def cleanup(): - yield - - channel.assert_full_filled() - author.assert_full_filled() - message.assert_full_filled() - - channel.reset() - author.reset() - message.reset() - - -class TestRespond: - def test_should_respond_hello(self): - channel.on("send", "Hello!").returns(False) - assert hello.Usecase.respond(message, "ok") is False - - channel.reset() - channel.on("send", "Hello!").returns("Test") - assert hello.Usecase.respond(message, "ok") == "Test" - - def test_should_not_respond_to_bot(self): - assert hello.Usecase.respond(message, "test") is None