-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storage changes #17
Storage changes #17
Conversation
a616fdf
to
61a9b25
Compare
@Jakuje I have a few more changes I want to make, you can start to review the general change if you want, but expect some API changes in the next rebase, so feel free to hold off. |
fb1f325
to
518d910
Compare
I think it is basically ready for review. I am not happy with the need to take a write lock for token for so many operations now (due to the caches needing updates on object fetching which are technically just reads). One idea is to change the object caches in the storage implementations to have themselves locks, so for most operations the locking can be in the caches themselves instead of locking the whole token object. Another option is to move the caches into the session objects, but the reason I moved them in the token is that some non-token objects still need to be available across sessions, and that would make matters complicated still requiring a caching layer at the token level ... |
109b58a
to
5d606ea
Compare
Signed-off-by: Simo Sorce <[email protected]>
In various cases changeing data in the cache would not cause a flush to the json file. Writes are rare so even for the very inefficient json file storage it is ok to just flush (and therefore rewrite the entire file) all the changes as soon as any modification operation happens. Signed-off-by: Simo Sorce <[email protected]>
Signed-off-by: Simo Sorce <[email protected]>
Add an sqlite storage backend and change the existing code to become a memory and a json backend (with the latter using the former as temporary storage). Change token to address caching storage changes, which require a mutable object to update the cache on reads as well. Introduce the conept of CheckedHandle to force correct use of the API. It needed to be split into a "prefetch" call which takes a mutable reference to the storage, and an actual call to get the object that does not. This is because the borrow checkers holds a mutable reference to the objects whenever a reference (even immutable) is returned as a result. This then prevents making further calls to the token object. Once theborrow checker is improved to stop inferring incorrect things about the need to keep mutable references around, this can be changed back to be a slightly saner access pattern. Change test infrastructure to use the files in testdata/ to generate separate test files that are always removed after the test completes. Switch most tests to use the sql storage. Signed-off-by: Simo Sorce <[email protected]>
The secrets would enver be revealead, but the object could confuse an application nonetheless, never attach a handle to them so they can never be seen at the application level. Streamline supporting functions as well. Signed-off-by: Simo Sorce <[email protected]>
@Jakuje I think I addressed all of the issues you raised |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to go!
merged |
Refactor storage to allow multiple backends.
Add a sqlite based storage backend (see more info on the commit).
Rework some PIN operations to simplify and address storage/masking issues.
also
Fixes #23