You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Mike Yoder <my...@cloudera.com> on 2014/10/28 18:35:29 UTC
Review Request 27290: SENTRY-507 more getConfigVal banning
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27290/
-----------------------------------------------------------
Review request for sentry.
Repository: sentry
Description
-------
SENTRY-507 adding two more items to banned config vals in getConfigVal()
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 6de9992556d176aec546b93d928e33e8aa2f2ab4
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 23bd7653c94f6337b5cbbc927b39fc811f275482
Diff: https://reviews.apache.org/r/27290/diff/
Testing
-------
Unit testing; added new tests for additional items.
Thanks,
Mike Yoder
Re: Review Request 27290: SENTRY-507 more getConfigVal banning
Posted by Mike Yoder <my...@cloudera.com>.
> On Oct. 28, 2014, 6:20 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 609
> > <https://reviews.apache.org/r/27290/diff/1/?file=735499#file735499line609>
> >
> > I could see how this could grow in the future, consider splitting this out to its own function (with a comment on the purpose) so you just have:
> >
> > if (isBannedConfigVal(attr)) {
> > throw ...
> > }
I think an entirely new function is a bit of overkill... but I did address this, sort of, by splitting out the regexes into a "requiredPattern" and an "excludePattern" String. Now we just issue two Pattern.matches(), and if the excludePattern grows we just make the string bigger.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27290/#review58842
-----------------------------------------------------------
On Oct. 28, 2014, 5:35 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27290/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 5:35 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-507 adding two more items to banned config vals in getConfigVal()
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 6de9992556d176aec546b93d928e33e8aa2f2ab4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 23bd7653c94f6337b5cbbc927b39fc811f275482
>
> Diff: https://reviews.apache.org/r/27290/diff/
>
>
> Testing
> -------
>
> Unit testing; added new tests for additional items.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27290: SENTRY-507 more getConfigVal banning
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27290/#review58842
-----------------------------------------------------------
Ship it!
If Hive doesn't have these checks you might consider filing a JIRA against them make the HMS more secure.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/27290/#comment99996>
I could see how this could grow in the future, consider splitting this out to its own function (with a comment on the purpose) so you just have:
if (isBannedConfigVal(attr)) {
throw ...
}
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java
<https://reviews.apache.org/r/27290/#comment99997>
nit: rename to checkBannedConfigVal()
- Lenni Kuff
On Oct. 28, 2014, 5:35 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27290/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 5:35 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-507 adding two more items to banned config vals in getConfigVal()
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 6de9992556d176aec546b93d928e33e8aa2f2ab4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 23bd7653c94f6337b5cbbc927b39fc811f275482
>
> Diff: https://reviews.apache.org/r/27290/diff/
>
>
> Testing
> -------
>
> Unit testing; added new tests for additional items.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27290: SENTRY-507 more getConfigVal banning
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27290/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 9:55 p.m.)
Review request for sentry.
Changes
-------
Feedback based on review comments
Repository: sentry
Description
-------
SENTRY-507 adding two more items to banned config vals in getConfigVal()
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 6de9992
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 23bd765
Diff: https://reviews.apache.org/r/27290/diff/
Testing
-------
Unit testing; added new tests for additional items.
Thanks,
Mike Yoder