Skip to content

Commit cf03052

Browse files
authored
Merge pull request #7118 from roed-math/psycodict-security-prep
Say what the website needs from psycodict, rather than assuming it
2 parents b5e1176 + c3fb0de commit cf03052

7 files changed

Lines changed: 155 additions & 11 deletions

File tree

.github/workflows/generate-matrix.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"lmfdb/tests/test_dynamic_knowls.py lmfdb/tests/test_root.py lmfdb/hecke_algebras/test_hecke_algebras.py lmfdb/tests/test_homepage.py lmfdb/elliptic_curves/test_ell_curves.py lmfdb/elliptic_curves/test_browse_page.py",
66
"lmfdb/sato_tate_groups/test_st.py lmfdb/hilbert_modular_forms/test_hmf.py lmfdb/tests/test_spelling.py lmfdb/tests/test_template_namespace.py lmfdb/tests/test_acknowlegments.py lmfdb/tests/test_tensor_products.py",
77
"lmfdb/cluster_pictures/test_clusterpicture.py lmfdb/local_fields/test_localfields.py lmfdb/ecnf/test_ecnf.py lmfdb/ecnf/test_isog_class.py lmfdb/api/test_api.py lmfdb/characters/test_characters.py",
8-
"lmfdb/users/test_users.py lmfdb/lattice/test_lattice.py lmfdb/maass_forms/test_maass.py lmfdb/higher_genus_w_automorphisms/test_hgcwa.py lmfdb/belyi/test_belyi.py lmfdb/hypergm/test_hgm.py lmfdb/tests/test_utils.py",
8+
"lmfdb/users/test_users.py lmfdb/lattice/test_lattice.py lmfdb/maass_forms/test_maass.py lmfdb/higher_genus_w_automorphisms/test_hgcwa.py lmfdb/belyi/test_belyi.py lmfdb/hypergm/test_hgm.py lmfdb/tests/test_utils.py lmfdb/tests/test_connection_reset.py",
99
"lmfdb/artin_representations/test_artin_representation.py lmfdb/genus2_curves/test_genus2_curves.py",
1010
"lmfdb/classical_modular_forms/test_cmf.py lmfdb/classical_modular_forms/test_cmf2.py",
1111
"lmfdb/lfunctions/test_lfunctions.py",

.github/workflows/matrix_includes.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,12 @@
6060
"server": "devmirror"
6161
},
6262
{
63-
"files": "lmfdb/users/test_users.py lmfdb/lattice/test_lattice.py lmfdb/maass_forms/test_maass.py lmfdb/higher_genus_w_automorphisms/test_hgcwa.py lmfdb/belyi/test_belyi.py lmfdb/hypergm/test_hgm.py lmfdb/tests/test_utils.py",
63+
"files": "lmfdb/users/test_users.py lmfdb/lattice/test_lattice.py lmfdb/maass_forms/test_maass.py lmfdb/higher_genus_w_automorphisms/test_hgcwa.py lmfdb/belyi/test_belyi.py lmfdb/hypergm/test_hgm.py lmfdb/tests/test_utils.py lmfdb/tests/test_connection_reset.py",
6464
"folders": "belyi higher_genus_w_automorphisms hypergm lattice maass_forms tests users",
6565
"server": "proddb"
6666
},
6767
{
68-
"files": "lmfdb/users/test_users.py lmfdb/lattice/test_lattice.py lmfdb/maass_forms/test_maass.py lmfdb/higher_genus_w_automorphisms/test_hgcwa.py lmfdb/belyi/test_belyi.py lmfdb/hypergm/test_hgm.py lmfdb/tests/test_utils.py",
68+
"files": "lmfdb/users/test_users.py lmfdb/lattice/test_lattice.py lmfdb/maass_forms/test_maass.py lmfdb/higher_genus_w_automorphisms/test_hgcwa.py lmfdb/belyi/test_belyi.py lmfdb/hypergm/test_hgm.py lmfdb/tests/test_utils.py lmfdb/tests/test_connection_reset.py",
6969
"folders": "belyi higher_genus_w_automorphisms hypergm lattice maass_forms tests users",
7070
"server": "devmirror"
7171
},

.github/workflows/python-package.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ jobs:
9090
- name: checking that we didn't miss any test files
9191
shell: bash -l {0}
9292
# If this fails you need to update the file list above and file count
93-
run: test $(find lmfdb -name 'test_*.py' -or -name '*_test.py' | wc -l) -eq 43
93+
run: test $(find lmfdb -name 'test_*.py' -or -name '*_test.py' | wc -l) -eq 44
9494

9595
- name: Config LMFDB to run tests against proddb
9696
if: matrix.files != 'lint' && matrix.server == 'proddb'

lmfdb/knowledge/knowl.py

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,13 +144,35 @@ class KnowlBackend(PostgresBase):
144144

145145
def __init__(self):
146146
PostgresBase.__init__(self, 'db_knowl', db)
147-
self._rw_knowldb = (getattr(db, "_can_read_write_knowls", None) or db.can_read_write_knowls)()
147+
self._refresh_connection_capabilities()
148148
# we cache knowl titles for 10s
149149
self.caching_time = 10
150150
self.cached_titles_timestamp = 0
151151
self.cached_defines_timestamp = 0
152152
self.cached_titles = {}
153153

154+
def _refresh_connection_capabilities(self):
155+
"""
156+
Ask the database what the current session may do with the knowls.
157+
158+
Fails closed: nothing is editable until the session has said so.
159+
"""
160+
self._rw_knowldb = False
161+
checker = getattr(db, "_can_read_write_knowls", None) or db.can_read_write_knowls
162+
self._rw_knowldb = checker()
163+
164+
def _connection_reset(self):
165+
"""
166+
Called by psycodict once it has replaced the connection this object
167+
uses (roed314/psycodict#135); a no-op in psycodict without that hook.
168+
169+
Whether knowls may be edited describes the session rather than this
170+
object, and a replacement connection can reach a standby, or a role
171+
whose privileges have changed, so the cached answer has to be asked
172+
again rather than carried over.
173+
"""
174+
self._refresh_connection_capabilities()
175+
154176
def _safe_execute(self, query, values=None):
155177
# Every 20 minutes we reload the knowl database on production
156178
# using a dump from beta. If this query is run during the time

lmfdb/lmfdb_database.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,15 @@
1212
from psycodict.searchtable import PostgresSearchTable
1313
from psycodict.statstable import PostgresStatsTable
1414

15+
try:
16+
from psycodict.grants import LMFDBGrantPolicy
17+
except ImportError:
18+
# psycodict before roed314/psycodict#137, which granted these privileges
19+
# unconditionally and so has nothing to be told about them. Delete this
20+
# fallback once the psycodict requirement is pinned past that PR.
21+
LMFDBGrantPolicy = None
22+
23+
1524
def overrides(super_class):
1625
def overrider(method):
1726
super_method = getattr(super_class, method.__name__)
@@ -483,6 +492,15 @@ def __init__(self, config=None, **kwargs):
483492
config = ConfigWrapper(config)
484493
# else: config is already a Configuration object, use it as-is
485494

495+
if LMFDBGrantPolicy is not None:
496+
# psycodict's default policy grants nothing, so a table it creates
497+
# -- or swaps in during a reload -- would be readable only by its
498+
# owner. This policy is what psycodict used to do: SELECT to lmfdb
499+
# and webserver, INSERT to webserver on counts and stats. It warns
500+
# rather than raises when a role is missing, since a development
501+
# database usually has neither; a deployment that must have both
502+
# can pass LMFDBGrantPolicy(missing_role="error") instead.
503+
kwargs.setdefault("grant_policy", LMFDBGrantPolicy())
486504
PostgresDatabase.__init__(self, config, **kwargs)
487505
self.is_verifying = False # set to true when importing lmfdb.verify
488506
self.__editor = config.logging_options.get("editor", "")
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
# Tests for the state the knowl and user backends derive from the database
2+
# session, which psycodict asks them to refresh when it replaces a connection.
3+
#
4+
# A real failover is not something a test suite can arrange, so these tests
5+
# stand in for one: they change what the database reports about the session and
6+
# call the hook psycodict would call, which is the part of the mechanism that
7+
# lives in the LMFDB.
8+
9+
import unittest
10+
11+
from lmfdb import db
12+
from lmfdb.knowledge.knowl import knowldb
13+
from lmfdb.users.pwdmanager import userdb
14+
15+
16+
class ConnectionResetTest(unittest.TestCase):
17+
def _set_capability(self, attr, value):
18+
"""
19+
Make the database report ``value`` for one of its capability flags,
20+
returning what it reported before.
21+
"""
22+
old = getattr(db, attr)
23+
setattr(db, attr, value)
24+
return old
25+
26+
def test_knowl_backend_follows_the_session(self):
27+
old = self._set_capability("_read_and_write_knowls", False)
28+
try:
29+
knowldb._connection_reset()
30+
assert not knowldb.can_read_write_knowls(), \
31+
"knowl editing stayed enabled after the session lost the privilege"
32+
33+
self._set_capability("_read_and_write_knowls", True)
34+
knowldb._connection_reset()
35+
assert knowldb.can_read_write_knowls(), \
36+
"knowl editing stayed disabled after the session regained the privilege"
37+
finally:
38+
self._set_capability("_read_and_write_knowls", old)
39+
knowldb._connection_reset()
40+
41+
def test_user_backend_follows_the_session(self):
42+
old = self._set_capability("_read_and_write_userdb", False)
43+
try:
44+
userdb._connection_reset()
45+
assert not userdb.can_read_write_userdb(), \
46+
"userdb stayed writable after the session lost the privilege"
47+
# Failing closed means the columns the session could not confirm
48+
# are not left behind either.
49+
assert userdb._cols == userdb._username_full_name
50+
51+
self._set_capability("_read_and_write_userdb", True)
52+
userdb._connection_reset()
53+
assert userdb.can_read_write_userdb(), \
54+
"userdb stayed read-only after the session regained the privilege"
55+
finally:
56+
self._set_capability("_read_and_write_userdb", old)
57+
userdb._connection_reset()
58+
59+
def test_grant_policy_is_the_lmfdb_one(self):
60+
# Only meaningful once psycodict has policies at all; before that it
61+
# grants these same privileges on its own.
62+
try:
63+
from psycodict.grants import LMFDBGrantPolicy
64+
except ImportError:
65+
self.skipTest("this psycodict has no grant policies")
66+
assert db.grant_policy == LMFDBGrantPolicy(), \
67+
"the database is not granting what the website needs on the tables it creates"

lmfdb/users/pwdmanager.py

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,52 @@ def __init__(self):
2323
PostgresBase.__init__(self, 'db_users', db)
2424
# never narrow down the rmin-rmax range, only increase it!
2525
self.rmin, self.rmax = -10000, 10000
26-
self._rw_userdb = (getattr(db, "_can_read_write_userdb", None) or db.can_read_write_userdb)()
2726
#TODO use this instead of hardcoded columns names
2827
#with identifiers
2928
self._username_full_name = ["username", "full_name"]
30-
if self._rw_userdb:
31-
cur = self._execute(SQL("SELECT column_name FROM information_schema.columns WHERE table_schema = %s AND table_name = %s"), ['userdb', 'users'])
32-
self._cols = [rec[0] for rec in cur]
33-
else:
34-
self._cols = self._username_full_name
29+
self._refresh_connection_capabilities()
30+
31+
def _refresh_connection_capabilities(self):
32+
"""
33+
Ask the database what the current session may do with userdb.users,
34+
and which columns it has.
35+
36+
This fails closed: the restricted answers are installed first and are
37+
only widened once the session has been found able to read and write.
38+
The query is issued on ``self.conn`` rather than through ``_execute``
39+
because this also runs while psycodict is recovering from a failure
40+
inside ``_execute``, which it would otherwise re-enter.
41+
"""
42+
self._rw_userdb = False
43+
self._cols = self._username_full_name
44+
45+
checker = getattr(db, "_can_read_write_userdb", None) or db.can_read_write_userdb
46+
if not checker():
47+
return
48+
cur = self.conn.cursor()
49+
try:
50+
cur.execute(SQL("SELECT column_name FROM information_schema.columns WHERE table_schema = %s AND table_name = %s"), ['userdb', 'users'])
51+
cols = [rec[0] for rec in cur.fetchall()]
52+
self.conn.commit()
53+
except Exception:
54+
self.conn.rollback()
55+
raise
56+
finally:
57+
cur.close()
58+
self._cols = cols
59+
self._rw_userdb = True
60+
61+
def _connection_reset(self):
62+
"""
63+
Called by psycodict once it has replaced the connection this object
64+
uses (roed314/psycodict#135); a no-op in psycodict without that hook.
65+
66+
Whether userdb may be written to describes the session rather than this
67+
object, and a replacement connection can reach a standby, or a role
68+
whose privileges have changed, so the cached answers have to be asked
69+
again rather than carried over.
70+
"""
71+
self._refresh_connection_capabilities()
3572

3673
def can_read_write_userdb(self):
3774
return self._rw_userdb

0 commit comments

Comments
 (0)