From 9a834c97e336e8e4f8b7e1d5516f0b7a91e551ee Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 29 Mar 2024 17:27:15 +0100 Subject: [PATCH 1/6] Settlement: rename variables to make the code more understandable --- ihatemoney/templates/settle_bills.html | 64 ++++++++++++++------------ ihatemoney/web.py | 16 +++---- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/ihatemoney/templates/settle_bills.html b/ihatemoney/templates/settle_bills.html index 67aac337e..f40436217 100644 --- a/ihatemoney/templates/settle_bills.html +++ b/ihatemoney/templates/settle_bills.html @@ -1,33 +1,39 @@ -{% extends "sidebar_table_layout.html" %} - -{% block sidebar %} -
- {{ balance_table(show_weight=False) }} -
-{% endblock %} - - -{% block content %} - - - - {% for bill in bills %} - - - - - +{% extends "sidebar_table_layout.html" %} {% block sidebar %} +
{{ balance_table(show_weight=False) }}
+{% endblock %} {% block content %} +
{{ _("Who pays?") }}{{ _("To whom?") }}{{ _("How much?") }}{{ _("Settled?") }}
{{ bill.ower }}{{ bill.receiver }}{{ bill.amount|currency }} - - -
- {{ ("Settle") }} -
- - -
+ + + + + + + + + + {% for transaction in transactions %} + + + + + {% endfor %} - -
{{ _("Who pays?") }}{{ _("To whom?") }}{{ _("How much?") }}{{ _("Settled?") }}
{{ transaction.ower }}{{ transaction.receiver }}{{ transaction.amount|currency }} + + +
+ {{ ("Settle") }} +
+ + +
+ + {% endblock %} diff --git a/ihatemoney/web.py b/ihatemoney/web.py index 43b04c213..cb1bea56e 100644 --- a/ihatemoney/web.py +++ b/ihatemoney/web.py @@ -852,24 +852,24 @@ def change_lang(lang): @main.route("//settle_bills") def settle_bill(): """Compute the sum each one have to pay to each other and display it""" - bills = g.project.get_transactions_to_settle_bill() - return render_template("settle_bills.html", bills=bills, current_view="settle_bill") + transactions = g.project.get_transactions_to_settle_bill() + return render_template("settle_bills.html", transactions=transactions, current_view="settle_bill") -@main.route("//settle///") -def settle(amount, ower_id, payer_id): - new_reinbursement = Bill( +@main.route("//settle///") +def add_settlement_bill(amount, sender_id, receiver_id): + settlement = Bill( amount=float(amount), date=datetime.datetime.today(), - owers=[Person.query.get(payer_id)], - payer_id=ower_id, + owers=[Person.query.get(receiver_id)], + payer_id=sender_id, project_default_currency=g.project.default_currency, bill_type=BillType.REIMBURSEMENT, what=_("Settlement"), ) session.update() - db.session.add(new_reinbursement) + db.session.add(settlement) db.session.commit() flash(_("Settlement bill has been successfully added"), category="success") From 967266f7dc45a0a56a6a269c12ef01dbf4ad3a89 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 29 Mar 2024 18:44:27 +0100 Subject: [PATCH 2/6] utils: improve error message when form field validation fails --- ihatemoney/utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ihatemoney/utils.py b/ihatemoney/utils.py index 7af4967bc..d39199fd3 100644 --- a/ihatemoney/utils.py +++ b/ihatemoney/utils.py @@ -452,7 +452,9 @@ def format_form_errors(form, prefix): ) else: error_list = "
  • ".join( - str(error) for (field, errors) in form.errors.items() for error in errors + f"{field} {error}" + for (field, errors) in form.errors.items() + for error in errors ) errors = f"
    • {error_list}
    " # I18N: Form error with a list of errors From a5bbaa61f746a8782aba17a12d0103dda3c18448 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 29 Mar 2024 19:43:56 +0100 Subject: [PATCH 3/6] Change settle endpoint to use POST instead of GET This is necessary to avoid XSS. State-changing actions should never be done with a GET. --- ihatemoney/forms.py | 18 ++++++++++++++++ ihatemoney/templates/settle_bills.html | 18 ++++++++++++---- ihatemoney/tests/budget_test.py | 22 ++++++++++--------- ihatemoney/web.py | 29 ++++++++++++++++++++------ 4 files changed, 67 insertions(+), 20 deletions(-) diff --git a/ihatemoney/forms.py b/ihatemoney/forms.py index 0fee97793..485867a28 100644 --- a/ihatemoney/forms.py +++ b/ihatemoney/forms.py @@ -14,6 +14,8 @@ BooleanField, DateField, DecimalField, + HiddenField, + IntegerField, Label, PasswordField, SelectField, @@ -437,6 +439,22 @@ def validate_original_currency(self, field): raise ValidationError(msg) +class HiddenCommaDecimalField(HiddenField, CommaDecimalField): + pass + + +class HiddenIntegerField(HiddenField, IntegerField): + pass + + +class SettlementForm(FlaskForm): + """Used internally for validation, not directly visible to users""" + + amount = HiddenCommaDecimalField("Amount", validators=[DataRequired()]) + sender_id = HiddenIntegerField("Sender", validators=[DataRequired()]) + receiver_id = HiddenIntegerField("Receiver", validators=[DataRequired()]) + + class MemberForm(FlaskForm): name = StringField(_("Name"), validators=[DataRequired()], filters=[strip_filter]) diff --git a/ihatemoney/templates/settle_bills.html b/ihatemoney/templates/settle_bills.html index f40436217..f344c0758 100644 --- a/ihatemoney/templates/settle_bills.html +++ b/ihatemoney/templates/settle_bills.html @@ -18,14 +18,24 @@ {{ transaction.amount|currency }} +
    + {{ settlement_form.csrf_token }} + {{ settlement_form.amount(value=transaction.amount) }} + {{ settlement_form.sender_id(value=transaction.ower.id) }} + {{ settlement_form.receiver_id(value=transaction.receiver.id) }} + +
    -
    {{ ("Settle") }}
    diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py index a3fc813f8..9899aad6d 100644 --- a/ihatemoney/tests/budget_test.py +++ b/ihatemoney/tests/budget_test.py @@ -1358,23 +1358,25 @@ def test_settle_button(self): count = 0 for t in transactions: count += 1 - self.client.get( - "/raclette/settle" - + "/" - + str(t["amount"]) - + "/" - + str(t["ower"].id) - + "/" - + str(t["receiver"].id) + self.client.post( + "/raclette/settle", + data={ + "amount": t["amount"], + "sender_id": t["ower"].id, + "receiver_id": t["receiver"].id, + }, ) temp_transactions = project.get_transactions_to_settle_bill() # test if the one has disappeared assert len(temp_transactions) == len(transactions) - count - # test if theres a new one with bill_type reimbursement + # test if there is a new one with bill_type reimbursement bill = project.get_newest_bill() assert bill.bill_type == models.BillType.REIMBURSEMENT - return + + # There should be no more settlement to do at the end + transactions = project.get_transactions_to_settle_bill() + assert len(transactions) == 0 def test_settle_zero(self): self.post_project("raclette") diff --git a/ihatemoney/web.py b/ihatemoney/web.py index cb1bea56e..23a749c7e 100644 --- a/ihatemoney/web.py +++ b/ihatemoney/web.py @@ -56,6 +56,7 @@ ProjectForm, ProjectFormWithCaptcha, ResetPasswordForm, + SettlementForm, get_billform_for, ) from ihatemoney.history import get_history, get_history_queries, purge_history @@ -853,16 +854,32 @@ def change_lang(lang): def settle_bill(): """Compute the sum each one have to pay to each other and display it""" transactions = g.project.get_transactions_to_settle_bill() - return render_template("settle_bills.html", transactions=transactions, current_view="settle_bill") + settlement_form = SettlementForm() + return render_template( + "settle_bills.html", + transactions=transactions, + settlement_form=settlement_form, + current_view="settle_bill", + ) + + +@main.route("//settle", methods=["POST"]) +def add_settlement_bill(): + """Create a bill to register a settlement""" + form = SettlementForm(id=g.project.id) + if not form.validate(): + flash( + format_form_errors(form, _("Error creating settlement bill")), + category="danger", + ) + return redirect(url_for(".settle_bill")) -@main.route("//settle///") -def add_settlement_bill(amount, sender_id, receiver_id): settlement = Bill( - amount=float(amount), + amount=form.amount.data, date=datetime.datetime.today(), - owers=[Person.query.get(receiver_id)], - payer_id=sender_id, + owers=[Person.query.get(form.receiver_id.data)], + payer_id=form.sender_id.data, project_default_currency=g.project.default_currency, bill_type=BillType.REIMBURSEMENT, what=_("Settlement"), From f9552076c31209adab1b9d1057839132f1316345 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 29 Mar 2024 19:49:40 +0100 Subject: [PATCH 4/6] tests: add more validation against cross-project access --- ihatemoney/tests/budget_test.py | 72 +++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py index 9899aad6d..82e765183 100644 --- a/ihatemoney/tests/budget_test.py +++ b/ihatemoney/tests/budget_test.py @@ -1465,6 +1465,78 @@ def test_access_other_projects(self): # Create and log in as another project self.post_project("tartiflette") + # Add a participant in this second project + self.client.post("/tartiflette/members/add", data={"name": "pirate"}) + pirate = models.Person.query.filter(models.Person.id == 5).one() + assert pirate.name == "pirate" + + # Try to add a new bill in another project + self.client.post( + "/raclette/add", + data={ + "date": "2017-01-01", + "what": "fromage frelaté", + "payer": 2, + "payed_for": [2, 3, 4], + "bill_type": "Expense", + "amount": "100.0", + }, + ) + # Ensure it has not been created + raclette = self.get_project("raclette") + assert raclette.get_bills().count() == 1 + + # Try to add a new bill in our project that references members of another project. + # First with invalid payed_for IDs. + self.client.post( + "/tartiflette/add", + data={ + "date": "2017-01-01", + "what": "soupe", + "payer": 5, + "payed_for": [3], + "bill_type": "Expense", + "amount": "5000.0", + }, + ) + # Ensure it has not been created + piratebill = models.Bill.query.filter(models.Bill.what == "soupe").one_or_none() + assert piratebill is None, "piratebill 1 should not exist" + + # Then with invalid payer ID + self.client.post( + "/tartiflette/add", + data={ + "date": "2017-02-01", + "what": "pain", + "payer": 3, + "payed_for": [5], + "bill_type": "Expense", + "amount": "5000.0", + }, + ) + # Ensure it has not been created + piratebill = models.Bill.query.filter(models.Bill.what == "pain").one_or_none() + assert piratebill is None, "piratebill 2 should not exist" + + # Make sure we can actually create valid bills + self.client.post( + "/tartiflette/add", + data={ + "date": "2017-03-01", + "what": "baguette", + "payer": 5, + "payed_for": [5], + "bill_type": "Expense", + "amount": "5.0", + }, + ) + # Ensure it has been created + okbill = models.Bill.query.filter(models.Bill.what == "baguette").one_or_none() + assert okbill is not None, "Bill baguette should exist" + assert okbill.what == "baguette" + + # Now try to access and modify existing bills modified_bill = { "date": "2018-12-31", "what": "roblochon", From 87112ec9d15e291e365bd0b3049134ecfae008d4 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Sun, 31 Mar 2024 19:50:29 +0200 Subject: [PATCH 5/6] Add security-related test to the new settle endpoint --- ihatemoney/tests/budget_test.py | 18 ++++++++++++++++++ ihatemoney/web.py | 1 + 2 files changed, 19 insertions(+) diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py index 82e765183..30507d945 100644 --- a/ihatemoney/tests/budget_test.py +++ b/ihatemoney/tests/budget_test.py @@ -1630,6 +1630,24 @@ def test_access_other_projects(self): member = models.Person.query.filter(models.Person.id == 1).one_or_none() assert member is None + # test new settle endpoint to add bills with wrong payer / payed_for + self.client.post("/exit") + self.client.post( + "/authenticate", data={"id": "tartiflette", "password": "tartiflette"} + ) + self.client.post( + "/tartiflette/settle", + data={ + "sender_id": 4, + "receiver_id": 5, + "amount": "42.0", + }, + ) + piratebill = models.Bill.query.filter( + models.Bill.bill_type == models.BillType.REIMBURSEMENT + ).one_or_none() + assert piratebill is None, "piratebill 3 should not exist" + @pytest.mark.skip(reason="Currency conversion is broken") def test_currency_switch(self): # A project should be editable diff --git a/ihatemoney/web.py b/ihatemoney/web.py index 23a749c7e..7ba24de10 100644 --- a/ihatemoney/web.py +++ b/ihatemoney/web.py @@ -874,6 +874,7 @@ def add_settlement_bill(): ) return redirect(url_for(".settle_bill")) + # TODO: check that sender and receiver ID are valid and part of this project settlement = Bill( amount=form.amount.data, From 2ec8924e4c765cac088a9826c297ce5a60576248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20M=C3=A9taireau?= Date: Sun, 5 Jan 2025 19:47:53 +0100 Subject: [PATCH 6/6] Test that the users belong the project before settling --- ihatemoney/models.py | 4 ++++ ihatemoney/tests/budget_test.py | 8 ++++---- ihatemoney/web.py | 11 ++++++++--- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/ihatemoney/models.py b/ihatemoney/models.py index c591b85b6..af21994d8 100644 --- a/ihatemoney/models.py +++ b/ihatemoney/models.py @@ -447,6 +447,10 @@ def remove_member(self, member_id): db.session.commit() return person + def has_member(self, member_id): + person = Person.query.get(member_id, self) + return person is not None + def remove_project(self): # We can't import at top level without circular dependencies from ihatemoney.history import purge_history diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py index 30507d945..732535bc9 100644 --- a/ihatemoney/tests/budget_test.py +++ b/ihatemoney/tests/budget_test.py @@ -1470,8 +1470,8 @@ def test_access_other_projects(self): pirate = models.Person.query.filter(models.Person.id == 5).one() assert pirate.name == "pirate" - # Try to add a new bill in another project - self.client.post( + # Try to add a new bill to another project + resp = self.client.post( "/raclette/add", data={ "date": "2017-01-01", @@ -1488,7 +1488,7 @@ def test_access_other_projects(self): # Try to add a new bill in our project that references members of another project. # First with invalid payed_for IDs. - self.client.post( + resp = self.client.post( "/tartiflette/add", data={ "date": "2017-01-01", @@ -1630,7 +1630,7 @@ def test_access_other_projects(self): member = models.Person.query.filter(models.Person.id == 1).one_or_none() assert member is None - # test new settle endpoint to add bills with wrong payer / payed_for + # test new settle endpoint to add bills with wrong ids self.client.post("/exit") self.client.post( "/authenticate", data={"id": "tartiflette", "password": "tartiflette"} diff --git a/ihatemoney/web.py b/ihatemoney/web.py index 7ba24de10..37bd811f8 100644 --- a/ihatemoney/web.py +++ b/ihatemoney/web.py @@ -874,13 +874,18 @@ def add_settlement_bill(): ) return redirect(url_for(".settle_bill")) - # TODO: check that sender and receiver ID are valid and part of this project + # Ensure that the sender and receiver ID are valid and part of this project + receiver_id = form.receiver_id.data + sender_id = form.sender_id.data + + if not g.project.has_member(sender_id): + return redirect(url_for(".settle_bill")) settlement = Bill( amount=form.amount.data, date=datetime.datetime.today(), - owers=[Person.query.get(form.receiver_id.data)], - payer_id=form.sender_id.data, + owers=[Person.query.get(receiver_id, g.project)], + payer_id=sender_id, project_default_currency=g.project.default_currency, bill_type=BillType.REIMBURSEMENT, what=_("Settlement"),