piskvorky / sqlitedict

Persistent dict, backed by sqlite3 and pickle, multithread-safe.
Apache License 2.0
1.17k stars 131 forks source link

SqliteDict read-only mode should not create table #127

Closed hholst80 closed 3 years ago

hholst80 commented 3 years ago

The current behavior of SqliteDict is to always create the table it should operate on. Would a PR be accepted to change this behavior, for mode='r', to not to create a table?

The behavior in sqlitedict right now is as follows:

$ python3
>>> from sqlitedict import SqliteDict
>>> db = SqliteDict('sqlite3.db', 'nonexistant-table', mode='r')
>>> <ctrl-d>
$ sqlite3 sqlite3.db
sqlite> .tables
nonexistant-table
sqlite> .quit
$
piskvorky commented 3 years ago

Hm, interesting question. I have no preference either way – how did this come up for you? What's your motivation, use-case where this makes a difference?

CC @mpenkov thoughts?

mpenkov commented 3 years ago

Yeah, I think it wouldn't be a bad idea, because it's quite unexpected to modify something by opening it for reading.

I think the expected behavior in the above case would be something like a ValueError being raised if the table does not exist.

hholst80 commented 3 years ago

I would like to keep things fairly consistent with how things work right now. Raising an exception would be a new workflow.

I think it would be reasonable for .commit to raise an exception on a read-only db but it should be OK to open a SqliteDict without problem even if the table does not exist.

>>> d=SqliteDict('foo.db', flag='r')
>>> d['foo']='bar'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.9/site-packages/sqlitedict.py", line 249, in __setitem__
    raise RuntimeError('Refusing to write to read-only SqliteDict')
RuntimeError: Refusing to write to read-only SqliteDict
>>> d.commit()
>>> d
SqliteDict(foo.db)
>>> dict(d)
{}
>>> 
hholst80 commented 3 years ago

Hm, interesting question. I have no preference either way – how did this come up for you? What's your motivation, use-case where this makes a difference?

CC @mpenkov thoughts?

The use case is somewhat bespoke but IMO not unreasonable. I am using SqliteDict to store user account data and I am bolting on an authentication workflow onto this now. The table name is basically the username and the dict is all the user account data for that user. In the authentication workflow the auth service checks the passwd field in the user account details (stored as a SqliteDict). However, if the username does not exist I do not want the db to change. So, I either have to store valid usernames in a separate db or allow rouge usernames (that is, sqlite tables) to be created by the auth service.

  1. Internet botnet tries to login as "admin", "root" etc.
  2. reverse proxy calls out to auth service
  3. auth service probes SqliteDict('accounts.db', username, flag='r') for the passwd field (pw hash)
  4. If they match, allow request, otherwise deny.

The problem right now is that in step 3 the auth service will populate the db with random username that people are trying to login as. Valid or invalid.

piskvorky commented 3 years ago

Thanks, that makes sense. How about a separate table (not DB) with valid usernames? …As long as you can keep it in sync with the corresponding user tables (atomic operations).

.commit to raise an exception on a read-only db but it should be OK to open a SqliteDict without problem even if the table does not exist.

Isn't that what happens now? I don't follow.

hholst80 commented 3 years ago

Isn't that what happens now? I don't follow.

Per my example it does not look like .commit raises an exception on a read-only db. It does however raise an exception on assignment. I am not sure that I agree that the semantics are right. IMO it should raise an exception on commit and not on assignment. If autocommit is true it should raise an exception on assignment, as that should implicitly commit.

This is however unrelated to the issue here with the implicit table create.