Skip to content
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

Handle complex metadata permission storage #58

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

qwofford
Copy link
Collaborator

See #39

@qwofford qwofford marked this pull request as draft July 19, 2023 21:08
@DanielRJohnson DanielRJohnson linked an issue Jul 20, 2023 that may be closed by this pull request
@DanielRJohnson DanielRJohnson linked an issue Jul 31, 2023 that may be closed by this pull request
@qwofford
Copy link
Collaborator Author

qwofford commented Nov 6, 2023

The permissions metadata tracker should:

  1. By default, give an error on artifact_handler put requests that have column permissions metadata which union to a set of size > one.
  2. Given a param indicating "scary mode", save files out, one for each unique permissions configuration in the union of the columns permissions metadata. Preserve posix file security information in resulting files. Unit tests should validate permissions are are preserved for information involved. The user should have to explicitly select "y" to save data.
  3. Given a param indicating "very scary mode", save all data to the same file and do so with many loud warnings including a report of all the permissions data that were squashed during this operation, and what they were squashed to. The user should have to explicitly select "y" to save data.

The DSI documentation for back-end drivers should be extended to explain this behavior.

In a maximally conservative workflow, we would never save data, but always read data using plugins before incorporating query functionality.

Coordinate with @kchilleri on this, and feel free to create sub-issues as needed.

@DanielRJohnson
Copy link
Collaborator

Updated this branch to catch up with main and to implement the functionalities requested.

Some implementation notes:

  • The "very scary" permissions squashing defaults to UID-GID-660. I figured this was sensible, but can be changed.
  • The Bueno plugin does not have an obvious way to fit in with the column-wise permissions model since each row comes from a different file. Not sure what is best on this front, I've just left a TODO and set the permissions to self.filenames[0]'s permissions.
  • Tests are passing except SystemKernel and GUFI tests that I can't run at the moment.
  • Pylama is happy except for a "too complex" warning for adding two conditionals to Terminal.artifact_handler. This would require a bigger restructure to fix, and didn't think it was necessary at the moment.

No rush to review, holidays are coming up! :)

@DanielRJohnson DanielRJohnson self-assigned this Dec 18, 2023
@DanielRJohnson
Copy link
Collaborator

@qwofford @kchilleri I can't add y'all as reviewers since Quincy is the owner or the PR and Krishna isn't listed, but see above ^

@qwofford
Copy link
Collaborator Author

Note that we should squash these commits because I made a mess of the git history while resolving conflicts.

@qwofford
Copy link
Collaborator Author

qwofford commented Dec 19, 2023

Looking excellent as usual. I'm adding some documentation for this new capability. When you @DanielRJohnson have an opportunity please take a look at what I wrote and we can discuss after the holiday:

https://lanl.github.io/dsi/permissions.html

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.

Environment plugin permission defaults Flexible file permissions handling at DSI Core Terminal
2 participants