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

Only show thumbnails if user has viewAccessCopies permissions #1630

Merged
merged 5 commits into from
Dec 5, 2023

Conversation

lfarrell
Copy link
Contributor

Copy link
Member

@bbpennel bbpennel left a comment

Choose a reason for hiding this comment

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

This looks good over all, just one part I had questions about

@@ -107,6 +109,16 @@ public boolean hasDatastreamAccess(AccessGroupSet principals, DatastreamType dat
return accessControlService.hasAccess(metadata.getPid(), principals, permission);
}

public boolean hasThumbnailPreviewAccess(AccessGroupSet principals, DatastreamType datastream,
Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep this as public, we should add javadoc. Its only used internal to the permissions helper though at the moment, so it'd also be reasonable to make it private.

So this is roughly the same as hasDatastreamAccess but without the check to see if the datastream exists, right? In that case, I don't think we need to check for both the LARGE and SMALL thumbnail in hasThumbnailAccess, since they have the same permission and it doesn't verify if it exists. We could probably also simplify things a bit and remove the datastream input parameter, since internally we can figure out what the thumbnail datastream(s) are.

@bbpennel bbpennel merged commit 44b2d5a into main Dec 5, 2023
2 checks passed
@bbpennel bbpennel deleted the BXC-4315 branch December 5, 2023 14:12
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