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

Restrcting the AccessPolicy options allowed during the project setup #2204

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

Rutvikrj26
Copy link
Contributor

Why this is required:

  • HDN does not support Open as one of the access policies, and hence ideally, it should not show up as one of the options allowed for the user to choose during the set-up of the project.

What this PR does:

  • Since the AccessPolicy names are strictly typed at a lot of places, and the access is based on the Integer comparisons, it is hard to remove the OPEN option as a part of the AccessPolicy class.
  • This PR modifies the AccessMetadataForm to restrict the choices available based on the environment variable ALLOWED_ACCESS_POLICIES, which has a default value set as the 4 available options, if the environment variable is not specified.

@bemoody
Copy link
Collaborator

bemoody commented Mar 13, 2024

Use config, don't use os.environ.

This setting kind of goes together with ENABLE_FILE_DOWNLOADS_OPTION; I would list these two settings in the same place and perhaps name the new setting ENABLED_ACCESS_POLICIES (though "allowed" is also fine.)

I think it'd be better not to list the possible values in the physionet/settings/base.py.

Also the 'replace("_", " ").title()' (duplicating the logic of AccessPolicy.choices) doesn't really belong in the form class.

So it could be something more like:

    if settings.ALLOWED_ACCESS_POLICIES:
        self.fields['access_policy'].choices = [
            (value, label) for (value, label) in AccessPolicy.choices()
            if AccessPolicy(value).name in settings.ALLOWED_ACCESS_POLICIES
        ]

Or maybe it would be cleaner to define a method AccessPolicy.allowed_choices instead. (I don't think we want to change the behavior of AccessPolicy.choices because those choices are baked into migrations.)

@Rutvikrj26
Copy link
Contributor Author

Heyy @bemoody
Thanks for reviewing!

I've updated the code based on your suggestions except one pending change which I would like to understand the reason better before implementing.

I think it'd be better not to list the possible values in the physionet/settings/base.py.

Wouldn't it be better to have that as a default value? The though was whichever deployment of Physionet wants to configure this setting, can just choose to add the environment variable, making it optional.
Let me know your thoughts.

@tompollard
Copy link
Member

We should also either: (1) Work out where we are going to document configurable settings or (2) Add the ALLOWED_ACCESS_POLICIES variable to the .env.example file.

@tompollard
Copy link
Member

Thanks @Rutvikrj26. I added a couple of suggestions above. Please address them in a new PR if necessary!

@tompollard tompollard merged commit 2e4e0cb into MIT-LCP:dev Mar 26, 2024
8 checks passed
tompollard added a commit that referenced this pull request Apr 19, 2024
This PR is a followup to #2204 where the access policies are getting
restricted. This adds the configuration setting to the .env.example with
usage instructions and it's effects on the platform.
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.

None yet

3 participants