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

#11076 Allow User with View Unpublished Dataset Perms to access Preview URL without becoming Preview URL User #11080

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

sekmiller
Copy link
Contributor

@sekmiller sekmiller commented Dec 10, 2024

What this PR does / why we need it:
Going back to 2020 there was a known issue that as a logged in user with permission on the draft version of a dataset, if you accessed a preview url while logged in you would be logged out.

Which issue(s) this PR closes:

Special notes for your reviewer:
Added a test to the Private URL init method

Suggestions on how to test this:
Access Preview URL while logged in with View Unpublished Version Permissions. You should remain logged in and see the full draft version.

If you access a Preview URL while logged in as someone without View Unpublished, you will be logged out (be changed to a Preview URL user)

If not logged in, Preview URL access should be unchanged.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No

Is there a release notes update needed for this change?:
Edge case - probably not worth noting

Additional documentation:

@sekmiller sekmiller added Size: 3 A percentage of a sprint. 2.1 hours. FY25 Sprint 12 FY25 Sprint 12 (2024-12-04 - 2024-12-18) labels Dec 10, 2024
@sekmiller sekmiller added this to the 6.5 milestone Dec 10, 2024

This comment has been minimized.

@ofahimIQSS
Copy link
Contributor

ofahimIQSS commented Dec 10, 2024

the continuous-integration/jenkins/pr-merge is failing

ERROR: Ansible run terminated abnormally, failing build.
GitHub has been notified of this commit’s build result
Finished: FAILURE

https://jenkins.dataverse.org/blue/organizations/jenkins/IQSS-Dataverse-Develop-PR/detail/PR-11080/2/pipeline

@qqmyers
Copy link
Member

qqmyers commented Dec 10, 2024

The deleteFile test is flaky (as reported to the core team last week) - depending on the order of returned files which appears to now be random. I restarted the build for now and will look into fixing the test.

@pdurbin pdurbin self-assigned this Dec 10, 2024
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@ofahimIQSS @sekmiller and I have been discussing.

From my perspective, the bug is not completely fixed in that some users will still be logged out (the ones who don't have permission to the unpublished dataset, which will be most users), but at least the person creating the Preview URL (has permission already, that is) will no longer be logged out. It's definitely an improvement. And Omer likes this better than what's in develop. Approved.

@pdurbin pdurbin removed their assignment Dec 10, 2024
@ofahimIQSS
Copy link
Contributor

An observation I had during testing which is an edge case -

Screen.Recording.2024-12-10.at.1.33.05.PM.mov

@qqmyers
Copy link
Member

qqmyers commented Dec 10, 2024

I think your edge case is #11047

@ofahimIQSS
Copy link
Contributor

I think your edge case is #11047

Indeed it is!

@ofahimIQSS ofahimIQSS self-assigned this Dec 10, 2024
@sekmiller
Copy link
Contributor Author

This should also fix #11047 - the refresh of the confirmation popup needed some attention.

Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:11076-access-previewUrl-with-perms
ghcr.io/gdcc/configbaker:11076-access-previewUrl-with-perms

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@ofahimIQSS
Copy link
Contributor

ofahimIQSS commented Dec 10, 2024

This should also fix #11047 - the refresh of the confirmation popup needed some attention.

Fix looks good - No issues to report back. Merging PR and proceeding with regression testing v6.5

Screen.Recording.2024-12-10.at.2.55.15.PM.mov

@ofahimIQSS ofahimIQSS merged commit 6e3a250 into develop Dec 10, 2024
10 of 11 checks passed
@ofahimIQSS ofahimIQSS deleted the 11076-access-previewUrl-with-perms branch December 10, 2024 20:01
@ofahimIQSS ofahimIQSS removed their assignment Dec 10, 2024
@pdurbin pdurbin mentioned this pull request Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 12 FY25 Sprint 12 (2024-12-04 - 2024-12-18) Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: Done 🧹
4 participants