search

irods / irods_rule_engine_plugin_metadata_guard

BSD 3-Clause "New" or "Revised" License
0 stars 7 forks source link

[40] Disallow use of rmw (main) #67

Closed FifthPotato closed 7 months ago

FifthPotato commented 7 months ago

It almost is that easy?

Pertinent questions:

  1. What error code return should I use for it? Not sure what's most relevant in this case...
  2. Should admins still be allowed to use rmw?

New tests for this still pending.

trel commented 7 months ago

https://github.com/irods/irods/issues/6417 was the deprecation in 4.3.0

removal coming in https://github.com/irods/irods/issues/6766, probably 5.0.0

agreed - no special cases for admins please

trel commented 7 months ago

SYS_NOT_ALLOWED seems the most correct, agreed.

Alternatively, we ship DEPRECATED_PARAMETER until 5.0.0, then ship SYS_NOT_ALLOWED?

FifthPotato commented 7 months ago

Found out that addw doesn't seem to bypass metadata_guard, so it doesn't need disabling.

korydraughn commented 7 months ago

SYS_NOT_ALLOWED seems the most correct, agreed.

Alternatively, we ship DEPRECATED_PARAMETER until 5.0.0, then ship SYS_NOT_ALLOWED?

I get what you're saying, but thinking about it more, disallowing an action and returning a deprecation error code feels wrong. Invoking a deprecated action normally results in the action being allowed at the expense of a warning or log message. For that reason, I think SYS_NOT_ALLOWED is the better choice.

I checked the iRODS server code and we only return DEPRECATED_PARAMETER in the rError stack. We never return that as the top-level API operation return code.

trel commented 7 months ago

You are correct - SYS_NOT_ALLOWED is best.

trel commented 7 months ago

Found out that addw doesn't seem to bypass metadata_guard, so it doesn't need disabling.

Let's make sure we have a test that shows we understand this behavior and have expected results and reasoning.

korydraughn commented 7 months ago

Please mark outdated comments as resolved if they have been handled.

FifthPotato commented 7 months ago

Investigated why rmw can bypass the plugin but addw can't: it just has to do with what inputs cause what. In rmw, you can specify an attribute that doesn't technically start with irods:: (or whatever protected prefix) but will still affect things in protected prefixes (e.g. imeta rmw -d whatever 'irods:%' '%' technically doesn't start with irods::), but I don't think addw supports wildcards in the attribute name.

trel commented 7 months ago

but I don't think addw supports wildcards in the attribute name.

correct - for addw, the wildcards match the target item name (data object, collection, user, resource).

we need to make sure a non-admin user who does have permission to add metadata on an item cannot add a particular AVU that would encroach on a metadata-guarded namespace...

i think right now the addw is failing because the user just doesn't even have permission to add any metadata to the target item... regardless of metadata_guard.

FifthPotato commented 7 months ago

All tests pass.

korydraughn commented 7 months ago

Excellent.

Please squash to taste. No pounds just yet.

FifthPotato commented 7 months ago

Squished and squashed.

korydraughn commented 7 months ago

Let's squash the changes for rmw into one commit and the test for addw into a separate commit.

FifthPotato commented 7 months ago

Resquished and resquashed. Had to learn a small bit of git-fu for that one.

trel commented 7 months ago

git-fu

!

FifthPotato commented 7 months ago

Octothorpe'd!