Skip to content

Comments

feat: encrypt datasets - add routes and write on db#17

Merged
Manuthor merged 40 commits intofindex_v6from
reuse_cli
Dec 5, 2024
Merged

feat: encrypt datasets - add routes and write on db#17
Manuthor merged 40 commits intofindex_v6from
reuse_cli

Conversation

@Manuthor
Copy link
Collaborator

@Manuthor Manuthor commented Nov 15, 2024

Doc:

  • Add README.md
  • Add doc for public_documentation repo

CI:

  • add build for Windows, macos and Docker

Server:

  • add list Permissions route
  • add routes to save/get/delete encrypted dataset
  • split Database trait in 3 (Findex, Permissions and Datasets)

CLI:

@Manuthor Manuthor force-pushed the reuse_cli branch 5 times, most recently from 6fdb46e to d4466b0 Compare November 18, 2024 10:17
Copy link
Contributor

@tbrezot tbrezot left a comment

Choose a reason for hiding this comment

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

Great work, I will need more time to fully understand it, though. Maybe a deeper review can be performed upon first release.

Comment on lines +20 to +27
impl Display for Uuids {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
for uuid in &self.uuids {
writeln!(f, "UUID: {uuid}")?;
}
Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We already talked about this, but encapsulating data in ad-hoc wrappers for the same of automatic Display logging really bugs me. Is there really no way to use an automatic Debug mode instead? I agree it wouldn't be as pretty, but the overhead of these wrappers is really big (79LoC here, 134LoC for the encrypted entries... and it is contaminating).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we already talked about this. I didn't find a better way to have neat logs where data are not raw u8 arrays. IMHO, Debug default trait is insufficient if we plan to debug code later. Debugging could be done by someone else. (The Display trait impl' themselves do not take 79LoC and 134LoC)

Copy link
Contributor

Choose a reason for hiding this comment

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

the Display implementation isn't, but if you don't need it, then the wrappers are not used and the whole uuids.rs and encrypted_entries.rs files and surely others become useless.

* fix: windows tests

* fix: clean permissions redis writing in server side

* fix: make dataset redis writing sync

* fix(redis): delete datasets

* test: make consistent the Redis writing. Use pipe everywhere except transaction on permissions creation

* fix: increase server waiting for redis to start

* fix: move cli conf in client
@Manuthor Manuthor marked this pull request as ready for review December 4, 2024 07:53
@Manuthor Manuthor requested a review from HatemMn December 4, 2024 07:53
Copy link
Collaborator

@HatemMn HatemMn left a comment

Choose a reason for hiding this comment

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

Amazing work !

A generic comment : there are many TODO / todo(manu) all around the code, maybe should be cleaned ? I didn't know if some of them are still up to date or no

";

pub(crate) struct Redis {
pub(crate) client: Client,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should store this, is it used anywhere ? We don't need it in cloudproof v6 and v7 too

it can be declared inline ( line 34 to be exact ) as it won't be used again anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, in that case, I would need an upsert script to prevent concurrent permission writes

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure I understand, but LUA scripts are run in an atomic manner. Do we need another LUA script ? I might work on that, if needed

Another alternative is marking it as issue where you show me where the script should be and where you mention my PR

Then close this, and I will try to do it on my own branch

Copy link
Contributor

@tbrezot tbrezot left a comment

Choose a reason for hiding this comment

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

Will read all that in #15 once merged.

@Manuthor Manuthor merged commit 61ee1be into findex_v6 Dec 5, 2024
@Manuthor Manuthor deleted the reuse_cli branch December 5, 2024 12:09
Manuthor added a commit that referenced this pull request Dec 9, 2024
* feat: add findex impl - client and server sides

* feat: add CSV datasets support in CLI

* fix: rename IndexAction to AddAction and flatten Findex actions

* Update .pre-commit-config.yaml

Co-authored-by: Hatem M'naouer <19950216+HatemMn@users.noreply.github.com>

* fix(auth0): use proper findex-server token

* test: add JWT authentication tests

* feat: add Delete Findex operation

* fix: remove Secret for now

* fix: prepare sqlite impl

* feat: add authorization access mechanism

* feat: revoke role

* docs: cli access

* chore: rename FindexClient to RestClient

* fix: permissions mechanism (#16)

* ci: fix

* test: authentication failing

* test:  ignore auth tests

* test: re-enable test server auth tests

* test: re-enable client auth tests

* test: disable client auth tests

* test: do not clear db

* chore: rename access to permission

* refactor: check_permission

* test: try mt tests

* chore: remove all lints

* test: remove dependency_on_unit_never_type_fallback

* test: make default server port to 6666

* fix: lint

* feat: encrypt datasets - add routes and write on db (#17)

* feat: encrypt datasets - add routes and write on db

* build: docker image

* fix: permissions on datasets (add/del/get)

* chore: split database traits

* fix: reuse config_utils

* feat: add list_permissions

* fix: strip error conversions

* ci: merge workflows between debug and release

* docs: add main README

* docs: add mkdocs

* docs: small changes in mkdocs

* chore: reduce verbosity

* ci: include windows build by default

* docs: fix menu

* fix(config): use only toml format (client and server)

* test: index a 3MB dataset

* fix(redis): use pipe everywhere Redis needs to read/write

* feat: support docker on arm cpu

* ci: build ARM docker only on nightly build

* fix: remove pandoc files

* docs: add CLI usage

* fix: use develop branch from http_client_server repo

* ci(docker build): remove useless arg

* fix: PR review

* fix(only docs): PR review

* test: add/delete/get datasets

* fix: Redis writing concurrency (on permissions) and Windows tests (#19)

* fix: windows tests

* fix: clean permissions redis writing in server side

* fix: make dataset redis writing sync

* fix(redis): delete datasets

* test: make consistent the Redis writing. Use pipe everywhere except transaction on permissions creation

* fix: increase server waiting for redis to start

* fix: move cli conf in client

* feat: use Serializable trait

* docs: add rust doc on database traits

* fix: avoid a user to lookup who are the more powerful users

* fix: database parameters not optional but with default values

* fix: reenable cli auth tests

* fix: remove useless r2d2 redis-feature and FindexRestClientResultHelper::context

* fix: removing useless clippy::blocks_in_conditions

* fix: remove uneeded RUSTSECs from deny.toml

* ci: fix Github actions warnings

* ci: fix Github actions warnings - download-artifact to v4

* fix: remove some todos

* fix: remove ahash feature from redis crate

* fix: remove useless .rustfmt.toml

* fix: Cargo.toml cleanup

* fix: bad links

* fix: clean useless comments in rust doc

* fix: in datasets actions, use &[Uuid] instead of Vec<Uuid>

* fix: uniformize in clap action all process() to run()

* fix: remove console

* fix: PR review

---------

Co-authored-by: Hatem M'naouer <19950216+HatemMn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants