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

SFR-2430: Set flags based on limited access permissions #494

Merged
merged 19 commits into from
Jan 8, 2025

Conversation

Apophenia
Copy link
Contributor

@Apophenia Apophenia commented Dec 30, 2024

Still lacking full testing for the flag logic but ready for re-review!

@kylevillegas93
Copy link
Contributor

@Apophenia is this ready for review?

@kylevillegas93
Copy link
Contributor

Do we need to change the has_part flags for the manifest as well based on the parsed permissions?

@Apophenia
Copy link
Contributor Author

Apophenia commented Jan 8, 2025

Do we need to change the has_part flags for the manifest as well based on the parsed permissions?

I'll check the original Michigan ingest process, but I don't think so. We discussed different approaches, but I believe the way it was implemented was that all manifests are stored in the public bucket, and the manifests contain fulfill links, but those fulfillment links are what require authentication (and they're stored in the private buckets).

@kylevillegas93 kylevillegas93 marked this pull request as ready for review January 8, 2025 16:20

def store_pdf_manifest(self, record: Record):
def store_pdf_manifest(self, record: Record, requires_login: bool=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming but do we want have a default value here and does it make sense for it to be True? Seems like in all cases this function will be called with that param set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the default here - I wanted to consistently fall back to the least permissive option to reduce the likelihood of something being set to open-access because of malformed input data, but I agree that this is not such a case.

Copy link
Contributor

@kylevillegas93 kylevillegas93 left a comment

Choose a reason for hiding this comment

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

Looks good!

@Apophenia Apophenia merged commit 628ae35 into main Jan 8, 2025
1 check passed
@Apophenia Apophenia deleted the limited-access-permissions branch January 8, 2025 20:19
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