Conversation
|
As soon as I'm done with putting out the AI bot fires, I wanted to pick up the slack on #10618 . As a part of that, I wanted to add backport options to ship important updates for older app container image releases (like I did for base images). Is it worth backporting all changes of this PR or limit it to the urgent matter at hand? |
pdurbin
left a comment
There was a problem hiding this comment.
Just some quick feedback from a pass @poikilotherm and I made.
| echo "Revoke the key that allows for creation of builtin users..." | ||
| curl -sS -X DELETE "${DATAVERSE_URL}/api/admin/settings/BuiltinUsers.KEY" | ||
|
|
||
| # TODO: stop using these deprecated database settings. See https://github.com/IQSS/dataverse/pull/11454 |
There was a problem hiding this comment.
I kicked off a thread about this here: https://dataverse.zulipchat.com/#narrow/channel/375812-containers/topic/deprecated.20API.20block.20settings.20.28.2311454.29/near/522132505
| The :doc:`/api/native-api` contains a useful but potentially dangerous set of API endpoints called "admin" that allows you to change system settings, make ordinary users into superusers, and more. The "builtin-users" endpoints let admins do tasks such as creating a local/builtin user account if they know the key defined in :ref:`BuiltinUsers.KEY`. | ||
|
|
||
| By default, most APIs can be operated on remotely and a number of endpoints do not require authentication. The endpoints "admin" and "builtin-users" are limited to localhost out of the box by the settings :ref:`:BlockedApiEndpoints` and :ref:`:BlockedApiPolicy`. | ||
| By default in the code, most of these API endpoints can be operated on remotely and a number of endpoints do not require authentication. However, the endpoints "admin" and "builtin-users" are limited to localhost out of the box by the installer, using the JvmSettings :ref:`API_BLOCKED_ENDPOINTS <dataverse.api.blocked.endpoints>` and :ref:`API_BLOCKED_POLICY <dataverse.api.blocked.policy>`. |
There was a problem hiding this comment.
Is the installer really doing this? I don't see any changes to the installer in this PR.
There was a problem hiding this comment.
Also, do we really want the link to say API_BLOCKED_ENDPOINTS? dataverse.api.blocked.endpoints is a bit less shout-y and what we normally do. 😄
There was a problem hiding this comment.
I didn't change the installer, but I think it was doing this before. I just added "by the installer" because the code defaults are not to block any apis but something in the normal setup process sets those settings - I just called that the "installer" - could say by the normal install process? FWIW: What I found was
dataverse/scripts/api/setup-all.sh
Lines 60 to 61 in 35145e5
dataverse/scripts/api/setup-all.sh
Lines 94 to 95 in 35145e5
There was a problem hiding this comment.
I started a thread in Slack about it. Happy to do a "talk after" at standup as well.
There's a monthly container meeting on Thursday. I'll bring up this PR there as well, the configbaker stuff especially.
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
pdurbin
left a comment
There was a problem hiding this comment.
I haven't really tested this in any meaningful way but I did help improve the docs a bit. I'm sending this to QA.
|
Fix looks good to me. Merging PR.
Created #11553 to track deprecated DB settings in installer. |
What this PR does / why we need it: This PR refactors the filters Dataverse uses to add Cors Headers, handle API version mapping, and handle API blocking.
There are various changes including moving the API-specific code to a JAX-RS ContainerRequestFilter (so it doesn't run for non-API calls), caching the :AllowCors setting to avoid a db call per request, caching the regular expressions used, etc.
It also adds an 'X-Dataverse-unblock-key' header so the key isn't exposed in the URL/browser history and adds a warning if the key is weaker than is allowed for passwords.
FWIW: The refactoring has one potentially useful extension of functionality - because the mapping is done against the @path annotations, it would be possible to block individual calls of the form /api/datasets/{id}/delete (for example) where the URL contains a variable id. We usually block at the class level, e.g. /api/admin so not sure how often this might get used.
Which issue(s) this PR closes:
Special notes for your reviewer: I've made the new JvmSettings read-once - at startup - and left the deprecated db settings dynamic. (The db settings are only used if the JvmSettings don't exist). I did this because there are multiple variants of install/config for classic and docker install that rely on being able to set the db settings and having them take effect immediately. It seemed like more work than worthwhile in this PR to try updating all of the install variants to use the new static JvmSettings.
Suggestions on how to test this: Verify that the existing db settings still work. Add JvmSettings and verify that they take effect in preference to the db settings. (Can check the server.log at startup to see what is in effect.) Verify that the drop, localhost-only, and unblock-key policies all work. Verify that the CORS headers are still applied (unless :AllowCors is false). Delete :AllowCors, set the new JvmSetting origin setting and verify that CORS headers are added as before. Optionally set the other CORS JvmSettings and verify that the headers are modified as expected.
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?: included
Additional documentation: in config guide