You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2021/10/27 21:23:59 UTC
[GitHub] [phoenix] ss77892 opened a new pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
ss77892 opened a new pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340
… views.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ss77892 commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
ss77892 commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r782493631
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/PermissionNSEnabledIT.java
##########
@@ -48,6 +56,43 @@ public PermissionNSEnabledIT() throws Exception {
public static synchronized void doSetup() throws Exception {
BasePermissionsIT.initCluster(true);
}
+ private AccessTestAction createMappedView(final String schemaName, final String tableName) throws SQLException {
Review comment:
Actually, the problem with the regexp is a bit different. It's the dot symbol. When we check the user permission for the table 'TEST.TEST' we accidentally get the permission for 'TEST:TEST' and would not check whether the table names match or not. So in the fix, we would remove the namespace problem by making the regex string have the namespace (so it would be 'default:TEST.TEST'), but I just realized that in theory if we don't have namespace mapping enabled and there are tables 'TEST.TEST' and 'TESTATEST' with a user that has access to TESTATEST than he/she would have access the 'TEST.TEST'. So to avoid that we also must escape the dot in the table name. Would write a test case and if it fails will file a separate bug.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ss77892 commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
ss77892 commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r738702676
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java
##########
@@ -481,7 +481,8 @@ public void preIndexUpdate(ObserverContext<PhoenixMetaDataControllerEnvironment>
for (MasterObserver service : getAccessControllers()) {
// Use AccessControlClient API's if the accessController is an instance of org.apache.hadoop.hbase.security.access.AccessController
if (service.getClass().getName().equals(org.apache.hadoop.hbase.security.access.AccessController.class.getName())) {
- userPermissions.addAll(AccessControlClient.getUserPermissions(connection, tableName.getNameAsString()));
Review comment:
Yeah. That was an interesting finding. We are using HBase AccessController to check whether we have permissions. The funny thing is that it uses regexp inside and it may incorrectly work with the Phoenix tables. For example, the user has access to the TEST schema but doesn't have access to the 'default' one. trying to access default:TEST.TEST table the access will be granted because he already has access to TEST:TEST. We have to check the full (including the namespace) table name to avoid such 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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ss77892 commented on pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
ss77892 commented on pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#issuecomment-954137004
retest
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] dbwong commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
dbwong commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r738694255
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java
##########
@@ -481,7 +481,8 @@ public void preIndexUpdate(ObserverContext<PhoenixMetaDataControllerEnvironment>
for (MasterObserver service : getAccessControllers()) {
// Use AccessControlClient API's if the accessController is an instance of org.apache.hadoop.hbase.security.access.AccessController
if (service.getClass().getName().equals(org.apache.hadoop.hbase.security.access.AccessController.class.getName())) {
- userPermissions.addAll(AccessControlClient.getUserPermissions(connection, tableName.getNameAsString()));
Review comment:
Can you explain this change a bit? This appears to be changing the user permissions where as the namespace handling appears to be in 486? Would this change I'm not too familiar with this class would this cause any change in accessibility between this patch and previously?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] dbwong commented on pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
dbwong commented on pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#issuecomment-953358725
Thanks for the submission. I think the changes look okay but can you please add a test to ensure this doesn't regress in the future?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
stoty commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r781779059
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/PermissionNSEnabledIT.java
##########
@@ -48,6 +56,43 @@ public PermissionNSEnabledIT() throws Exception {
public static synchronized void doSetup() throws Exception {
BasePermissionsIT.initCluster(true);
}
+ private AccessTestAction createMappedView(final String schemaName, final String tableName) throws SQLException {
Review comment:
Are you sure ?
While we are not mapping the Phoenix schema to a the HBase namespaces, we prepend it to the Hbase table name directly, I think that the regex confustion can still happens the same.
i.e. the `tablename` regex will still match `anyschema.tablename,` and even `anyschema.tablenamefoo` , only maybe on a different codepath in HBase.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#issuecomment-1007182501
This got me thinking that we should in fact
use `"^"+tableName.getNameWithNamespaceInclAsString()+"$"`
to avoid a similar stituation, where we have permission for `ns:prefixTable,` and we do not add the required new permission for `ns:prefixTableFoo` because we THINK we already have it.
I think I'm going to open am HBase ticket about the horrific API design (if one doesn't exist yet).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
stoty commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r780071025
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/PermissionNSEnabledIT.java
##########
@@ -48,6 +56,43 @@ public PermissionNSEnabledIT() throws Exception {
public static synchronized void doSetup() throws Exception {
BasePermissionsIT.initCluster(true);
}
+ private AccessTestAction createMappedView(final String schemaName, final String tableName) throws SQLException {
Review comment:
We should run this test for the NS Disabled case as well.
I.e. I think this should be in the parent class.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#issuecomment-1007182501
This got me thinking that we should in fact
use `"^"+tableName.getNameWithNamespaceInclAsString()+"$"`
to avoid a similar stituation, where we have permission for `ns:prefixTable,` and we do not add the required new permission for `ns:prefixTableFoo` because we THINK we already have it.
I think I'm going to open am HBase ticket about the horrific API design (if one doesn't exist yet).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
stoty commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r780071025
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/PermissionNSEnabledIT.java
##########
@@ -48,6 +56,43 @@ public PermissionNSEnabledIT() throws Exception {
public static synchronized void doSetup() throws Exception {
BasePermissionsIT.initCluster(true);
}
+ private AccessTestAction createMappedView(final String schemaName, final String tableName) throws SQLException {
Review comment:
We should run this test for the NS Disabled case as well.
I.e. I think this should be in the parent class.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ss77892 commented on a change in pull request #1340: PHOENIX-6579 ACL check doesn't honor the namespace mapping for mapped…
Posted by GitBox <gi...@apache.org>.
ss77892 commented on a change in pull request #1340:
URL: https://github.com/apache/phoenix/pull/1340#discussion_r781761088
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/PermissionNSEnabledIT.java
##########
@@ -48,6 +56,43 @@ public PermissionNSEnabledIT() throws Exception {
public static synchronized void doSetup() throws Exception {
BasePermissionsIT.initCluster(true);
}
+ private AccessTestAction createMappedView(final String schemaName, final String tableName) throws SQLException {
Review comment:
Well, didn't realize it from the beginning, but with NS disabled this test case doesn't make sense. We have a single namespace 'default' and all system and user tables are in there.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org