You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/12/09 17:01:02 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #1832: Add additional permission checks in AuditMessageIT
Manno15 opened a new pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744617613
> @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 with it 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742524904
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744706158
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744630413
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745304373
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745300510
LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745351543
I'm in progress for another review of this. I have some cleanup of the tests to shorten it up a bit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#discussion_r539510581
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -482,6 +482,10 @@ public void testDeniedAudits() throws AccumuloSecurityException, AccumuloExcepti
auditConnector.tableOperations().deleteRows(OLD_TEST_TABLE_NAME, new Text("myRow"),
new Text("myRow~"));
} catch (AccumuloSecurityException ex) {}
+ try {
+ auditConnector.tableOperations().flush(OLD_TEST_TABLE_NAME, new Text("start"),
+ new Text("end"), false);
Review comment:
Normally, a range would be specified so that the start sorts before the end, but 's' sorts after 'e'. You should change it to match `myRow` and `myRow~` like the previous line, instead, so that way this will still work, even if we later add additional argument checks to make sure the start sorts before the end.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742816050
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#discussion_r539538978
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -482,6 +482,10 @@ public void testDeniedAudits() throws AccumuloSecurityException, AccumuloExcepti
auditConnector.tableOperations().deleteRows(OLD_TEST_TABLE_NAME, new Text("myRow"),
new Text("myRow~"));
} catch (AccumuloSecurityException ex) {}
+ try {
+ auditConnector.tableOperations().flush(OLD_TEST_TABLE_NAME, new Text("start"),
+ new Text("end"), false);
Review comment:
Funny thing is, that is actually what I had originally but I changed it to mimic the parameters names for whatever reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745264947
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742818339
I'll try it tomorrow, but I don't recall seeing that. Maybe I am blind.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742811570
> 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`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744590487
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745224050
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744626193
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744626672
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742817682
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-746114793
I like your changes to the test. I know you already merged but it looks good!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742555044
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745323445
I'm not really worried about it, up to you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744617613
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-745520126
In order to make some improvements to these tests, I backported a JUnit upgrade from #1562 into the 1.10 development branch in d4fd27f32dc2611a23f67b1d3e8dafd8ee05a1cb
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744612038
@Manno15 - are you working adding tests for the other calls? Or would it help if someone also took a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-744610558
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742818339
I'll try it tomorrow, but I don't recall seeing that. Maybe I am blind. Thanks for the help.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-741959478
I'll take a look at permissionsIT, I can probably just add those to this PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742521904
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-742533150
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1832: Add additional permission checks in AuditMessageIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1832:
URL: https://github.com/apache/accumulo/pull/1832#issuecomment-743183369
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org