Skip to content

Conversation

@DomGarguilo
Copy link
Member

This PR:

  • removes tests or portions of tests that switch proxy users
  • misc. improvements to other test logic, names or logging

Copy link

@Manno15 Manno15 left a comment

Choose a reason for hiding this comment

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

Overall the changes look good. Was it discussed somewhere else as to why these tests are removed versus fixing?

@DomGarguilo
Copy link
Member Author

Overall the changes look good. Was it discussed somewhere else as to why these tests are removed versus fixing?

Yea good point, sorry. There has been an effort to remove the use of multiple users in the proxy to reduce complexity. Most of that was taken care of in #59

@DomGarguilo DomGarguilo merged commit ccda683 into apache:main Jan 30, 2023
@DomGarguilo DomGarguilo deleted the fixSwitchUsersTests branch January 30, 2023 20:23
@ctubbsii
Copy link
Member

ctubbsii commented Feb 6, 2023

Overall the changes look good. Was it discussed somewhere else as to why these tests are removed versus fixing?

Yea good point, sorry. There has been an effort to remove the use of multiple users in the proxy to reduce complexity. Most of that was taken care of in #59

I think the previous comment by @Manno15 still had a point. Some of those tests probably needed fixing, rather than removal. For example, the permissions test could be changed so that the permissions are added/removed by the root user using the minicluster API directly, and the Proxy instance could be used to check the regular user's permissions are appropriately enabled/restricted.

Similarly, the conditional writer test was doing some checks with the authorizations, which could have been manipulated in minicluster. Multiple proxy instances for separate users (using separate proxy config files for each separate user) would have also worked.

@DomGarguilo
Copy link
Member Author

For example, the permissions test could be changed so that the permissions are added/removed by the root user using the minicluster API directly, and the Proxy instance could be used to check the regular user's permissions are appropriately enabled/restricted.

@ctubbsii, what do you mean by "regular user" here? In the tests when using the minicluster, the only user that is created/used is the root user.

@ctubbsii
Copy link
Member

ctubbsii commented Feb 9, 2023

For example, the permissions test could be changed so that the permissions are added/removed by the root user using the minicluster API directly, and the Proxy instance could be used to check the regular user's permissions are appropriately enabled/restricted.

@ctubbsii, what do you mean by "regular user" here? In the tests when using the minicluster, the only user that is created/used is the root user.

Look for lines that say client.createLocalUser. Those created an unprivileged user whose permissions were manipulated to verify that the permissions worked as expected in the proxy. There are a few things that were being verified by those tests:

  1. The proxy's ability to create a user
  2. The proxy's ability to switch users
  3. The proxy's ability to execute the grant/revoke permission APIs
  4. Verifying the new user's permissions were enforced when performing the operation whose permission was granted/revoked

We still want to make sure we have proxy test coverage for 1 and 3. We could rely on Accumulo's own enforcement testing for 4, since the proxy now only has one user... and there's no risk of it using the wrong user's permissions now, but if we want the same coverage as before, we'll need to do some alternative to item 2, which we no longer need to test, as it is not supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants