[40] Disallow use of rmw (main)

[FifthPotato]

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.

[korydraughn]

You got it. It's really that easy sometimes :-)

1. What error code return should I use for it? Not sure what's most relevant in this case...

Hmm, did you look through the error code list and find other codes that could work? The error code file is called rodsErrorTable.h.

Two error codes come to mind:

• DEPRECATED_PARAMETER
• SYS_NOT_ALLOWED (I think this is a strong candidate)

2. Should admins still be allowed to use rmw?

No, for the following reasons:

• It's a wildcard
  • Requires extra, perhaps costly steps to get right
  • Could be slow
• We're planning to remove it
  • We want to slowly nudge users/admins away from that subcommand
• Allowing admins to use rmw makes the plugin's job more difficult

We should probably add a few words to the README that state why imeta rmw is not allowed.

Thoughts?

[trel]

irods/irods#6417 was the deprecation in 4.3.0

removal coming in irods/irods#6766, probably 5.0.0

agreed - no special cases for admins please

[trel]

SYS_NOT_ALLOWED seems the most correct, agreed.

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

[FifthPotato]

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

[korydraughn]

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]

You are correct - SYS_NOT_ALLOWED is best.

[trel]

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]

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

[FifthPotato]

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]

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.

[korydraughn]

Just a few more changes and I think we're good.

Please confirm the tests pass after resolving the review comments.

Let us know the results of the tests.

[FifthPotato]

All tests pass.

[korydraughn]

Excellent.

Please squash to taste. No pounds just yet.

[FifthPotato]

Squished and squashed.

[korydraughn]

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

[FifthPotato]

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

[trel]

git-fu

!

[korydraughn]

Looks like your git-fu worked. Nothing appears to have changed.

Good job.

Pound it!

[FifthPotato]

Octothorpe'd!