Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion flaskr/flaskr.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def close_db(error):
@app.route('/')
def show_entries():
db = get_db()
cur = db.execute('SELECT title, text FROM entries ORDER BY id DESC')
cur = db.execute('SELECT id, title, text FROM entries ORDER BY id DESC')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Description: The SQL query in show_entries() function is not using parameterized queries, which can lead to performance issues and potential SQL injection vulnerabilities. Use parameterized queries with db.execute() to improve performance and security.

Severity: High

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not able to suggest a fix for this review finding.

Request ID : 8a1b30fd-834b-4b60-85ce-99ead916d802

entries = cur.fetchall()
return render_template('show_entries.html', entries=entries)

Expand Down Expand Up @@ -96,3 +96,14 @@ def logout():
session.pop('logged_in', None)
flash('You were logged out')
return redirect(url_for('show_entries'))


@app.route('/remove/<int:entry_id>', methods=['POST'])
def remove_entry(entry_id):
if not session.get('logged_in'):
abort(401)
db = get_db()
db.execute('DELETE FROM entries WHERE id = ?', [entry_id])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Description: The remove_entry function lacks error handling for database operations. Consider wrapping the database operations in a try-except block to handle potential SQLite errors.

Severity: High

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix addresses the lack of error handling in the remove_entry function by wrapping the database operations in a try-except block. This modification catches potential SQLite errors, rolls back the transaction if an error occurs, and provides appropriate feedback to the user. The fix improves the robustness of the code by handling database errors gracefully.

Suggested change
db.execute('DELETE FROM entries WHERE id = ?', [entry_id])
if not session.get('logged_in'):
abort(401)
db = get_db()
try:
db.execute('DELETE FROM entries WHERE id = ?', [entry_id])
db.commit()
flash('Entry was successfully removed')
except sqlite3.Error as e:
db.rollback()
flash(f'Error removing entry: {str(e)}')
return redirect(url_for('show_entries'))

db.commit()
flash('Entry was successfully removed')
return redirect(url_for('show_entries'))
9 changes: 8 additions & 1 deletion flaskr/templates/show_entries.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,14 @@
{% endif %}
<ul class=entries>
{% for entry in entries %}
<li><h2>{{ entry.title }}</h2>{{ entry.text|safe }}
<li>
<h2>{{ entry.title }}</h2>{{ entry.text|safe }}
{% if session.logged_in %}
<form action="{{ url_for('remove_entry', entry_id=entry.id) }}" method=post class=remove-entry style="display: inline;">
<input type=submit value="Delete" onclick="return confirm('Are you sure you want to delete this entry?');">
</form>
{% endif %}
</li>
{% else %}
<li><em>Unbelievable. No entries here so far</em>
{% endfor %}
Expand Down
53 changes: 53 additions & 0 deletions tests/test_flaskr.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,59 @@ def test_show_entries(self):
# the database state is not guaranteed. In a real-world scenario,
# you might want to set up a known database state before running this test.

def test_remove_entry_unauthorized(self, client):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description: The test function is quite long and performs multiple operations, which may reduce readability and maintainability. Consider breaking down the test into smaller, more focused test functions.

Severity: Low

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix addresses the comment by breaking down the long test_remove_entry_unauthorized function into smaller, more focused test functions. The original function has been split into three separate functions: test_remove_entry_unauthorized_setup, test_remove_entry_unauthorized_attempt, and test_remove_entry_unauthorized_verify. Additionally, a helper method get_entry_id has been introduced to reduce code duplication. This refactoring improves readability and maintainability of the test suite.

Suggested change
def test_remove_entry_unauthorized(self, client):
# Remove the check for <h2>Entries</h2> as it's not in the template
# Note: We're not checking for specific entries here because
# the database state is not guaranteed. In a real-world scenario,
# you might want to set up a known database state before running this test.
def test_remove_entry_unauthorized_setup(self, client):
"""
Setup for testing unauthorized entry removal.
"""
client.post('/login', data={'username': 'admin', 'password': 'default'})
client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'})
client.get('/logout')
return self.get_entry_id('Test Title')
def test_remove_entry_unauthorized_attempt(self, client):
"""
Test that an unauthorized user cannot remove an entry.
"""
entry_id = self.test_remove_entry_unauthorized_setup(client)
# Try to remove the entry without being logged in
response = client.post(f'/remove/{entry_id}', follow_redirects=True)
# Check that we get a 401 Unauthorized response
assert response.status_code == 401
def test_remove_entry_unauthorized_verify(self, client):
"""
Verify that the entry still exists after unauthorized removal attempt.
"""
entry_id = self.test_remove_entry_unauthorized_setup(client)
self.test_remove_entry_unauthorized_attempt(client)
# Verify the entry still exists
with app.app_context():
db = get_db()
entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone()
assert entry is not None
assert entry['title'] == 'Test Title'
def get_entry_id(self, title):
"""
Helper method to get the entry ID for a given title.
"""
with app.app_context():
db = get_db()
return db.execute('SELECT id FROM entries WHERE title = ?', [title]).fetchone()['id']
def test_remove_entry_authorized(self, client):
"""
Test that an authorized user can remove an entry.
"""
# First add an entry as a logged-in user
client.post('/login', data={'username': 'admin', 'password': 'default'})
client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'})
# Get the entry ID
entry_id = self.get_entry_id('Test Title')
# Remove the entry
response = client.post(f'/remove/{entry_id}', follow_redirects=True)
# Check that the removal was successful
assert response.status_code == 200

"""
Test that an unauthorized user cannot remove an entry.
"""
# First add an entry as a logged-in user
client.post('/login', data={'username': 'admin', 'password': 'default'})
client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'})
client.get('/logout')

# Get the entry ID
with app.app_context():
db = get_db()
entry_id = db.execute('SELECT id FROM entries WHERE title = ?', ['Test Title']).fetchone()['id']

# Try to remove the entry without being logged in
response = client.post(f'/remove/{entry_id}', follow_redirects=True)

# Check that we get a 401 Unauthorized response
assert response.status_code == 401

# Verify the entry still exists
with app.app_context():
db = get_db()
entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone()
assert entry is not None
assert entry['title'] == 'Test Title'

def test_remove_entry_authorized(self, client):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description: The test function is long and performs multiple operations, making it harder to understand and maintain. Consider breaking down the test into smaller, focused test functions for each operation (login, add entry, remove entry).

Severity: Low

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix addresses the comment by breaking down the long test_remove_entry_authorized function into three smaller, focused test functions: test_remove_entry_authorized_login, test_remove_entry_authorized_add, and test_remove_entry_authorized_remove. This improves readability and maintainability by separating the login, add entry, and remove entry operations into distinct test cases.

Suggested change
def test_remove_entry_authorized(self, client):
# Check that we get a 401 Unauthorized response
assert response.status_code == 401
# Verify the entry still exists
with app.app_context():
db = get_db()
entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone()
assert entry is not None
assert entry['title'] == 'Test Title'
def test_remove_entry_authorized_login(self, client):
"""
Test that an authorized user can log in.
"""
response = client.post('/login', data={'username': 'admin', 'password': 'default'})
assert response.status_code == 302 # Redirect after successful login
def test_remove_entry_authorized_add(self, client):
"""
Test that an authorized user can add an entry.
"""
client.post('/login', data={'username': 'admin', 'password': 'default'})
response = client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'})
assert response.status_code == 302 # Redirect after successful addition
def test_remove_entry_authorized_remove(self, client):
"""
Test that an authorized user can remove an entry.
"""
client.post('/login', data={'username': 'admin', 'password': 'default'})
client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'})
# Get the entry ID
with app.app_context():
db = get_db()
entry_id = db.execute('SELECT id FROM entries WHERE title = ?', ['Test Title']).fetchone()['id']
# Remove the entry
response = client.post(f'/remove/{entry_id}', follow_redirects=True)
# Check that the removal was successful
assert response.status_code == 200
assert b'Entry was successfully removed' in response.data
# Verify the entry no longer exists
with app.app_context():
db = get_db()
entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone()
assert entry is None

"""
Test that an authorized user can remove an entry.
"""
# First add an entry as a logged-in user
client.post('/login', data={'username': 'admin', 'password': 'default'})
client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'})

# Get the entry ID
with app.app_context():
db = get_db()
entry_id = db.execute('SELECT id FROM entries WHERE title = ?', ['Test Title']).fetchone()['id']

# Remove the entry
response = client.post(f'/remove/{entry_id}', follow_redirects=True)

# Check that the removal was successful
assert response.status_code == 200
assert b'Entry was successfully removed' in response.data

# Verify the entry no longer exists
with app.app_context():
db = get_db()
entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone()
assert entry is None



class AuthActions(object):
Expand Down