Add additional permission checks in AuditMessageIT

[Manno15]

In relation to #1830, I only added an audit check for flushing a table at the moment but I am working on adding additional ones for set/remove system property and any other that is requested.

[ctubbsii]

This is a good start to check the flush command. However, checking the audit message doesn't guarantee the permission is denied (the auditing could be working, but we still allow the action). So, I think we will need checks to ensure the action is denied. The best place to do that is probably PermissionsIT. Can you make those changes in this PR, or would you prefer to open another PR for PermissionsIT?

[Manno15]

I'll take a look at permissionsIT, I can probably just add those to this PR

[Manno15]

@ctubbsii As it stands currently, you only need write permissions to flush a table. Do you think I should add separate permissions for Flush specifically (not fully sure how to do this and make it functional. I did add the permission option though), piggyback off the write permission check to also do a flush of the table, or leave as is and assume that permission is checked correctly since they share permissions?

I also noticed that in PermissionsIT that the test case for system permissions isn't actually there and is left as a to-do I assume. Not sure if there was a good reason why that was left undone.

[ctubbsii]

@ctubbsii As it stands currently, you only need write permissions to flush a table. Do you think I should add separate permissions for Flush specifically (not fully sure how to do this and make it functional. I did add the permission option though), piggyback off the write permission check to also do a flush of the table, or leave as is and assume that permission is checked correctly since they share permissions?

There's no need to change any permissions. The test can simply add a user that lacks the appropriate permission, and can verify that the appropriate AccumuloSecurityException denial is thrown if a flush is attempted.

I also noticed that in PermissionsIT that the test case for system permissions isn't actually there and is left as a to-do I assume. Not sure if there was a good reason why that was left undone.

It probably just wasn't in scope of the original issue. Maybe there's another test that checks those? It can be added to this one, if not.

[Manno15]

There's no need to change any permissions. The test can simply add a user that lacks the appropriate permission and can verify that the appropriate AccumuloSecurityException denial is thrown if a flush is attempted.

That's what the test does overall, was more of a question on where to place it. In its own case, or in the Write case. I decided to go with the latter because the former would require adding its own permission.

[Manno15]

Whenever I try to grant a system permission on the shell I get: [shell.Shell] ERROR: org.apache.accumulo.core.util.BadArgumentException: Unrecognized permission near index 6 grant System.SYSTEM -u test_user
Am I doing something wrong or is that a bug? This is on 1.10. I am trying to test out the permission for changing system properties.

[ctubbsii]

Whenever I try to grant a system permission on the shell I get: [shell.Shell] ERROR: org.apache.accumulo.core.util.BadArgumentException: Unrecognized permission near index 6 grant System.SYSTEM -u test_user
Am I doing something wrong or is that a bug? This is on 1.10. I am trying to test out the permission for changing system properties.

What command are you running? Compare what you are entering with the help information for the command you are running (<command> --help).

[Manno15]

Just the Grant command. grant <perm> -u <user>. Every other perm works as expected, none of the System ones do for me. It fails on the first S in System.

[ctubbsii]

Just the Grant command. grant <perm> -u <user>. Every other perm works as expected, none of the System ones do for me. It fails on the first S in System.

The help documentation for the grant command says to use -s. So, grant -s <systemPerm> -u <user> should work.

[Manno15]

I'll try it tomorrow, but I don't recall seeing that. Maybe I am blind. Thanks for the help.

[Manno15]

@ctubbsii It appears I was blind. The -s makes the command work, can't believe is missed it. Any thoughts on my most recent commits and anything else I should add?

[ctubbsii]

@ctubbsii It appears I was blind. The -s makes the command work, can't believe is missed it. Any thoughts on my most recent commits and anything else I should add?

The PermissionsIT test is hard to parse. I'm trying to take a look now.

[ctubbsii]

The following RPC calls were affected: initiateFlush, waitForFlush, shutdown, shutdownTabletServer, setMasterGoalState, setSystemProperty, and removeSystemProperty. Ideally, we should have test coverage for all of these cases.

[EdColeman]

@Manno15 - are you working adding tests for the other calls? Or would it help if someone also took a look?

[Manno15]

@Manno15 - are you working adding tests for the other calls? Or would it help if someone also took a look?

I am splitting time between this and #1824. I am willing to work on adding these other tests but I welcome help if you are offering it.

The following RPC calls were affected: initiateFlush, waitForFlush, shutdown, shutdownTabletServer, setMasterGoalState, setSystemProperty, and removeSystemProperty. Ideally, we should have test coverage for all of these cases.

Should they be added to this IT? Similar to how I did flush or do you think there is a better location for all of these? I am unsure if permissionsIT is there to test that the permissions work or there to test the underlying function that uses the permission.

[dlmarion]

I'm wondering if a more complete solution here is to create a test for the MasterClientServiceHander (the Master's API) as that is where the SecurityOperation.canFlush method is called.

[ctubbsii]

Should they be added to this IT?

They should be in this PR, but I'm not sure which IT is best suited. I'm sure if you use your best judgment, it will be fine. People can provide help via code reviews if they know of a better place.

[ctubbsii]

I'm wondering if a more complete solution here is to create a test for the MasterClientServiceHander (the Master's API) as that is where the SecurityOperation.canFlush method is called.

Many of those operations already have their own specific end-to-end ITs, as this class represents RPC endpoints. I thought the PermissionsIT was intended to specifically check the permissions of each of these endpoints, so that other tests can focus on testing the end-to-end behavior, but I could be wrong about the scope of the PermissionsIT.

[dlmarion]

From what I can tell, the driver in PermissionsIT are the different Permissions enum values. I was thinking of a test that hits the API directly, like https://github.com/dlmarion/accumulo/blob/manager-api-it/test/src/main/java/org/apache/accumulo/test/functional/ManagerApiIT.java

[Manno15]

I was able to test the flush RPC calls and the set/remove system property call within PermissionsIT but I don't see a way to test shutdown, shutdownTabletServer, and setMasterGoalState. I think implementing something akin to what @dlmarion suggested is best suited to handle that so I will implement it into a new IT

[Manno15]

I was able to handle each RPC call using @dlmarion's suggestion and using his IT as the basis for the one I pushed. Credit goes to him for that, thanks. The only part I am unsure on is if I should keep it in one test or separate each RPC call to their own test within the IT.

Overall, I think this is ready for review and we can discuss which approach we want to go with and I can remove what we don't need/want and add anything that is missing.

[dlmarion]

LGTM

[Manno15]

@dlmarion Do you want me to add you as a co-author if my last commit gets merged? Since it was your idea and you provided the basis for it?

[dlmarion]

I'm not really worried about it, up to you.

[ctubbsii]

I'm in progress for another review of this. I have some cleanup of the tests to shorten it up a bit.

[ctubbsii]

In order to make some improvements to these tests, I backported a JUnit upgrade from #1562 into the 1.10 development branch in d4fd27f

[Manno15]

I like your changes to the test. I know you already merged but it looks good!