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
signed URLs with private url users, autogenerate API token #10098
signed URLs with private url users, autogenerate API token #10098
Conversation
and get valid token if expired/missing (IQSS#10045)
Does this PR fix the following issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release note is a little confusing. Otherwise, the code is looking good. I haven't tested it but API tests are passing.
if (user instanceof AuthenticatedUser) { | ||
apiToken = getValidApiTokenForAuthenticatedUser((AuthenticatedUser) user); | ||
} else if (user instanceof PrivateUrlUser) { | ||
PrivateUrlUser privateUrlUser = (PrivateUrlUser) user; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Netbeans is suggesting the fancy new Java 14 instanceof automatic casting feature ( https://blogs.oracle.com/javamagazine/post/pattern-matching-for-instanceof-in-java-14 ), but I'll leave it alone.
@@ -12,7 +12,7 @@ | |||
*/ | |||
public class PrivateUrlUser implements User { | |||
|
|||
public static final String PREFIX = "#"; | |||
public static final String PREFIX = "!"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why I picked #
but I think it's fine to change it (I'm glad there's a flyway script).
if(!userId.startsWith(PrivateUrlUser.PREFIX)) { | ||
targetUser = authSvc.getAuthenticatedUser(userId); | ||
userApiToken = authSvc.findApiTokenByUser((AuthenticatedUser)targetUser); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off. I'll push a fix.
@@ -0,0 +1,5 @@ | |||
A new version of the standard Dataverse Previewers from https://github/com/gdcc/dataverse-previewers is available. The new version supports the use of signedUrls rather than API keys when previewing restricted files (including files in draft dataset versions). Upgrading is highly recommended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by this. When Dataverse 6.1 is released, will Dataverse Previewers 1.4 be released?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is my intent. Feel free to hedge.
Has this new PR been added yet? I can't find it. |
@qqmyers can you please provide a tool and manifest to test with? I tried simply modifying the existing text previewer but it doesn't work. Here's the manifest I used:
What I see in the browser: |
The new previewers aren't out yet, so using the manual method described in how to test would be the best option. It looks like you have a valid config - the response you get has the base64 encoded callback param, which, when decoded, shows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine, including creating an API token. Merging.
What this PR does / why we need it: This PR addresses issues related to using signedUrls as a PrivateUrlUser and, as part of refactoring/reducing duplicate code, also adds generating an API token if needed to launch a config tool.
For PrivateUrlUsers, signedUrls weren't working for a couple reasons:
Which issue(s) this PR closes:
Special notes for your reviewer:
Re: the PrivateUrlUser PREFIX - I chose to change this to '!' rather than use a character that has to be escaped (given that other rounds of escaping/unescaping may be happening). The only place this PREFIX was recorded is in the roleassignment table, and a flyway update script is given. I don't think these were exposed anywhere - perhaps in some api call? Alternately, the username used for SignedUrl purposes could be different than what is used in the db, but that seemed to be less straight-forward. (One could go without a PREFIX in signedUrl usernames, but it seems useful to be able to distinguish PrivateUrlUsers without having to see if the rest of the name can be parsed as a datasetId.)
W.r.t. #10045/autogenerating api keys - we do this in some places now, This PR adds it for dataset-level config tools. With this PR enabling signedUrls for PrivateUrlUsers and a planned PR to convert the existing Previewers to use signedUrls, it seems like were close to a model where we could keep the API key hidden unless the user wanted to use it explicitly. That may be a good way to address the concerns in #9898.
Suggestions on how to test this:
Configure a file previewer to use signedUrls and verify that the signedUrls provided work on a draft file (or restricted file) for a normal user and for a PrivateUrlUser, both in the file page and when launched as a separate page.
The signedUrls can be verified manually or by using one of the previewers now enabled for signedUrls (new PR being added to https://github.com/gdcc/dataverse-previewers). Manual verification involves looking at the URL used to launch the previewer in the browser console/network tab, getting the callback parameter, base64 decoding it, using that signedUrl in the browser to retrieve the params and allowedApiCall signedUrls and then verify that they also work. (I can do a quick walkthrough for anyone.)
Re: #10045 - this is needed for TurboCurator and using the TurboCurator configuration currently set up on the beta machine would be easiest (i.e. use the same configuration json as their to register the TurboCurator tool on your test instance.) As with previewers, manually verifying that the signedURLs returned work would be enough. (afaik, TurboCurator is not yet publicly visible.)
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: I've included some notes - not sure what rises to the level of being called out. I also went ahead and made the notes reference new Previewers that can use signed Urls. While those aren't in this PR and aren't part of a 6.1 release per se, they will work with 6.1 (and the PrivateUrl fix here means that 6.1 is the first version where they will work for PrivateUrl users. They should work for normal authenticated users in 5.14/6.0 as well.)
Additional documentation:
https://dataverse-guide--10098.org.readthedocs.build/en/10098/api/external-tools.html