DynamoDB improvements #221
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
harry-anderson
requested review from
michelemin and
obelisk
and removed request for
michelemin
harry-anderson
commented
Nov 19, 2025
harry-anderson
commented
Nov 19, 2025
obelisk
previously approved these changes
Nov 25, 2025
Owner
|
@michelemin For one more review |
michelemin
requested changes
Nov 25, 2025
Collaborator
michelemin
left a comment
michelemin
left a comment
There was a problem hiding this comment.
Mostly nits and typos
| /// Configured readers - maps a table name to a list of rules that are allowed to READ data | ||
| r: HashMap<String, HashSet<String>>, | ||
| /// Reserved tables - list of 'reserved' table names which rules cannot access | ||
| /// For the purpose of preventing rules rule accessing 'storage' tables in |
Collaborator
There was a problem hiding this comment.
Typo - "preventing rules rule accessing"
Collaborator
There was a problem hiding this comment.
Actually I think the entire second line should be removed. The first line seems sufficient.
| /// Configured readers - maps a table name to a list of rules that are allowed to READ data | ||
| r: HashMap<String, HashSet<String>>, | ||
| /// Reserved tables - list of 'reserved' table names which rules cannot access | ||
| /// For the purpose of preventing rules rule accessing 'storage' tables in |
|
|
||
| #[derive(PartialEq, PartialOrd)] | ||
| #[derive(PartialEq, PartialOrd, Debug)] | ||
| /// Represents an access scope for a rule has to modify a DynamoDB table |
Collaborator
There was a problem hiding this comment.
Nit
Suggested change
| /// Represents an access scope for a rule has to modify a DynamoDB table | |
| /// Represents an access scope that a rule has to modify a DynamoDB table |
Comment on lines
109
to
110
| /// Checks if module can perform given give action | ||
| /// Modules are registerd as as read (R) or write (RW) under self. |
Collaborator
There was a problem hiding this comment.
Typos
Suggested change
| /// Checks if module can perform given give action | |
| /// Modules are registerd as as read (R) or write (RW) under self. | |
| /// Checks if module can perform a given action | |
| /// Modules are registered as as read (R) or write (RW) under self. |
Comment on lines
+154
to
+164
| // check if read access is configured for this table | ||
| if let Some(table_readers) = self.r.get(table_name) { | ||
| // check if this module has read access to this table | ||
| if table_readers.contains(&module.to_string()) { | ||
| warn!( | ||
| "[{module}] trying to [write] but only has [read] permission for dynamodb table [{table_name}]" | ||
| ); | ||
| return Err(ApiError::BadRequest); | ||
| } | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
I would put this block after checking for Write access.
Otherwise, imagine that a module appears both in the r and rw sections for a table: we would log a warning but then everything would be fine.
So I'd say we first check the happy path: if something goes wrong, we dig deeper and try to understand what happened.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improvements to the dynamodb api for rules:
reserved_tablesconfig to prevent rules from accessing 'reserved_tables' i.e dynamodb table used by the storage API