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/08 16:50:43 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request #1828: Throw exceptions when permissions checks fail
ctubbsii opened a new pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828
Add and throw missing exceptions when permissions checks fail. This
prevents certain operations that the user does not have privileges to
perform from succeeding anyway.
----------------------------------------------------------------
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 #1828: Throw exceptions when permissions checks fail
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828#issuecomment-740803586
> Shouldn't spotbugs have noticed the boolean returned not being used?
It is my understanding that spotbugs doesn't check the return value of every method, only well-known ones whose return values are expected to be used. It's possible that at level 20, it would report it. However, we run spotbugs at maxRank 16, because higher is too spammy, and the value of what it finds is diminished.
----------------------------------------------------------------
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 #1828: Throw exceptions when permissions checks fail
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828#issuecomment-740870064
@Manno15 has backported these fixes to 1.10.1 in #1830 , and has described tests he will add. With two approvals already, I'll merge this now, so the tests can be more easily merged forward from those changes later.
----------------------------------------------------------------
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 #1828: Throw exceptions when permissions checks fail
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828
----------------------------------------------------------------
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 #1828: Throw exceptions when permissions checks fail
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828#issuecomment-740795957
> Why not make the methods throw the exception themselves? It looks like they are already throwing `ThriftSecurityException`.
1. That would be a bigger change,
2. That would be inconsistent with how the methods are used today, and
3. In some cases, we don't want to fail yet... as it's one of several conditions which can be satisfied to perform the operation, so the boolean is needed.
----------------------------------------------------------------
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] milleruntime commented on pull request #1828: Throw exceptions when permissions checks fail
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828#issuecomment-740797724
> > Why not make the methods throw the exception themselves? It looks like they are already throwing `ThriftSecurityException`.
>
> 1. That would be a bigger change,
>
> 2. That would be inconsistent with how the methods are used today, and
>
> 3. In some cases, we don't want to fail yet... as it's one of several conditions which can be satisfied to perform the operation, so the boolean is needed.
OK I see that now. For example, the Auditing needs to know the result so it can be logged.
----------------------------------------------------------------
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] milleruntime commented on pull request #1828: Throw exceptions when permissions checks fail
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1828:
URL: https://github.com/apache/accumulo/pull/1828#issuecomment-740798502
Shouldn't spotbugs have noticed the boolean returned not being used?
----------------------------------------------------------------
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