From 293735eca715c7cc5221e551e5eb41f92b6abd0f Mon Sep 17 00:00:00 2001 From: 0livd Date: Mon, 23 Oct 2017 23:03:44 +0200 Subject: [PATCH] Make authentication logic simpler and safer (#270) * Fixed exposed password in session The project password was set in clear text in the session cookie. The cookie payload is only base64 encoded so it must not be used to store private information. The password is simply replaced by a boolean. * Simplify authentication logic --- CHANGELOG.rst | 1 + ihatemoney/tests/tests.py | 4 +-- ihatemoney/web.py | 61 +++++++++++++++++---------------------- 3 files changed, 29 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 27c6cbd8..c2dba843 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,7 @@ Changed ======= - Logged admin can see any project (#262) +- Simpler and safer authentication logic (#270) - Better install doc (#275) Added diff --git a/ihatemoney/tests/tests.py b/ihatemoney/tests/tests.py index 36ca6fc2..6c0ccb9f 100644 --- a/ihatemoney/tests/tests.py +++ b/ihatemoney/tests/tests.py @@ -181,7 +181,7 @@ class BudgetTestCase(IhatemoneyTestCase): }) # session is updated - self.assertEqual(session['raclette'], 'party') + self.assertTrue(session['raclette']) # project is created self.assertEqual(len(models.Project.query.all()), 1) @@ -373,7 +373,7 @@ class BudgetTestCase(IhatemoneyTestCase): self.assertNotIn("Authentication", resp.data.decode('utf-8')) self.assertIn('raclette', session) - self.assertEqual(session['raclette'], 'raclette') + self.assertTrue(session['raclette']) # logout should wipe the session out c.get("/exit") diff --git a/ihatemoney/web.py b/ihatemoney/web.py index c7a2f22b..92b7ddc4 100644 --- a/ihatemoney/web.py +++ b/ihatemoney/web.py @@ -105,7 +105,7 @@ def pull_project(endpoint, values): project_id=project_id)) is_admin = session.get('is_admin') - if (project.id in session and session[project.id] == project.password) or is_admin: + if session.get(project.id) or is_admin: # add project into kwargs and call the original function g.project = project else: @@ -159,43 +159,34 @@ def authenticate(project_id=None): msg = _("You need to enter a project identifier") form.errors["id"] = [msg] return render_template("authenticate.html", form=form) - else: - project = Project.query.get(project_id) - create_project = False # We don't want to create the project by default + project = Project.query.get(project_id) if not project: - # But if the user try to connect to an unexisting project, we will + # If the user try to connect to an unexisting project, we will # propose him a link to the creation form. - if request.method == "POST": - form.validate() - else: - create_project = project_id + return render_template("authenticate.html", form=form, create_project=project_id) - else: - # if credentials are already in session, redirect - if project_id in session and project.password == session[project_id]: - setattr(g, 'project', project) - return redirect(url_for(".list_bills")) + # if credentials are already in session, redirect + if session.get(project_id): + setattr(g, 'project', project) + return redirect(url_for(".list_bills")) - # else process the form - if request.method == "POST": - if form.validate(): - if not form.password.data == project.password: - msg = _("This private code is not the right one") - form.errors['password'] = [msg] - else: - # maintain a list of visited projects - if "projects" not in session: - session["projects"] = [] - # add the project on the top of the list - session["projects"].insert(0, (project_id, project.name)) - session[project_id] = form.password.data - session.update() - setattr(g, 'project', project) - return redirect(url_for(".list_bills")) + if request.method == "POST" and form.validate(): + if not form.password.data == project.password: + msg = _("This private code is not the right one") + form.errors['password'] = [msg] + return render_template("authenticate.html", form=form) + # maintain a list of visited projects + if "projects" not in session: + session["projects"] = [] + # add the project on the top of the list + session["projects"].insert(0, (project_id, project.name)) + session[project_id] = True + session.update() + setattr(g, 'project', project) + return redirect(url_for(".list_bills")) - return render_template("authenticate.html", form=form, - create_project=create_project) + return render_template("authenticate.html", form=form) @main.route("/") @@ -233,7 +224,7 @@ def create_project(): db.session.commit() # create the session object (authenticate) - session[project.id] = project.password + session[project.id] = True session.update() # send reminder email @@ -290,8 +281,8 @@ def edit_project(): if request.method == "POST": if edit_form.validate(): project = edit_form.update(g.project) + db.session.add(project) db.session.commit() - session[project.id] = project.password return redirect(url_for(".list_bills")) @@ -359,7 +350,7 @@ def demo(): contact_email="demo@notmyidea.org") db.session.add(project) db.session.commit() - session[project.id] = project.password + session[project.id] = True return redirect(url_for(".list_bills", project_id=project.id))