Skip to content

Comments

GEODE-9740: require DATA:WRITE permission for WRITE redis ops and PUBLISH#7029

Merged
dschneider-pivotal merged 5 commits intoapache:developfrom
dschneider-pivotal:GEODE-9740
Oct 27, 2021
Merged

GEODE-9740: require DATA:WRITE permission for WRITE redis ops and PUBLISH#7029
dschneider-pivotal merged 5 commits intoapache:developfrom
dschneider-pivotal:GEODE-9740

Conversation

@dschneider-pivotal
Copy link
Contributor

@dschneider-pivotal dschneider-pivotal commented Oct 22, 2021

Redis commands with the WRITE flag and the PUBLISH command will require DATA:WRITE.
All other commands will require DATA:READ.

Options

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

@dschneider-pivotal dschneider-pivotal marked this pull request as ready for review October 23, 2021 08:04
@jdeppe-pivotal jdeppe-pivotal added the redis Issues related to the geode-for-redis module label Oct 25, 2021
Copy link
Contributor

@jdeppe-pivotal jdeppe-pivotal left a comment

Choose a reason for hiding this comment

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

Looks good. Could you also add an integration test (maybe to AuthIntegrationTest) that exercises this with read and write commands.

Copy link
Contributor

@upthewaterspout upthewaterspout left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

Looking through the flags in RedisCommandType, I think a few commands might need to be updated? Some commands are marked neither READONLY nor WRITE, so they default to requiring write permission in our scheme. But maybe that's not the right thing?

  • PING
  • ECHO
  • COMMAND
  • INFO
  • LOLWUT
  • SUBSCRIBE, PSUBSCRIBE, UNSUBSCRIBE, ??
  • PUBSUB
  • CLUSTER

That last one - cluster, means I think that a readonly client currently couldn't really work. This might have only worked because the AuthIntegrationTest is not using a clustered client. I think it probably should.

are those with the WRITE flag and the PUBLISH command.
All others need READ permission.
@dschneider-pivotal dschneider-pivotal changed the title GEODE-9740: use DATA:READ permission for read only redis operations GEODE-9740: require DATA:WRITE permission for WRITE redis ops and PUBLISH Oct 26, 2021
Copy link
Contributor

@nonbinaryprogrammer nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

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

looks good but I'd like a couple more tests

@dschneider-pivotal dschneider-pivotal merged commit 0b93946 into apache:develop Oct 27, 2021
@dschneider-pivotal dschneider-pivotal deleted the GEODE-9740 branch October 27, 2021 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

redis Issues related to the geode-for-redis module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants