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

Add ability to encrypt excel files [SC-SC-203035] #14

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

apichery
Copy link
Contributor

@apichery apichery commented Apr 21, 2024

Important - Requires this PR on the DSS side: https://github.com/dataiku/dip/pull/28735

image

Note: I couldn't find a way to have the "Encryption password" field appear only with DSS 13.0+.
If you know how to do it, here is simple method to plug

def supports_encryption_password_excel(dss_client):
    # Check that the version of DSS is above 13.0 where this feature has been introduced
    try:
        return int(dss_client.get_instance_info().raw["dssVersion"].split(".")[0]) >= 13
    except ValueError:
        return False

@apichery apichery marked this pull request as draft April 22, 2024 14:20
Copy link
Contributor

@liamlynch-data liamlynch-data left a comment

Choose a reason for hiding this comment

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

Looks ok as far as it goes - the only thing is that the when the recipe config is saved the password is not encrypted (unlike everywhere else in DSS thanks to the other PR).

I think the easiest/best way to handle this is to add a preset config in the plugin (no DSS change required I think).

@@ -35,7 +35,7 @@ def attachments_template_dict(attachment_datasets, home_project_key, apply_color
return attachments_dict


def build_attachment_files(attachment_datasets, attachment_type, apply_coloring_excel):
def build_attachment_files(attachment_datasets, attachment_type, apply_coloring_excel, encryption_password_excel):
Copy link
Contributor

Choose a reason for hiding this comment

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

need to add param to docstring

Copy link
Contributor

@liamlynch-data liamlynch-data left a comment

Choose a reason for hiding this comment

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

I'm happy for this one to go in, ideally once we convert it to be able to use presets as done here:
#17

@liamlynch-data liamlynch-data changed the title Add ability to encrypt excel files Add ability to encrypt excel files - SC-203035 Sep 16, 2024
@liamlynch-data liamlynch-data changed the title Add ability to encrypt excel files - SC-203035 Add ability to encrypt excel files [SC-SC-203035] Sep 16, 2024
@liamlynch-data
Copy link
Contributor

We should wait for this feature to continue on this card. This will solve the security issues inherent in this approach without forcing the users to use presets
178823

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.

2 participants