Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw exceptions when permissions checks fail #1828

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

ctubbsii
Copy link
Member

@ctubbsii ctubbsii commented Dec 8, 2020

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.

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.
@ctubbsii ctubbsii added blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug. labels Dec 8, 2020
@ctubbsii ctubbsii self-assigned this Dec 8, 2020
Copy link
Contributor

@milleruntime milleruntime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make the methods throw the exception themselves? It looks like they are already throwing ThriftSecurityException.

@ctubbsii
Copy link
Member Author

ctubbsii commented Dec 8, 2020

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.

@milleruntime
Copy link
Contributor

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.

@milleruntime
Copy link
Contributor

Shouldn't spotbugs have noticed the boolean returned not being used?

@ctubbsii
Copy link
Member Author

ctubbsii commented Dec 8, 2020

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.

@ctubbsii
Copy link
Member Author

ctubbsii commented Dec 8, 2020

@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.

@ctubbsii ctubbsii merged commit 58b9de1 into apache:main Dec 8, 2020
@ctubbsii ctubbsii deleted the fix-permission-checks branch December 8, 2020 19:08
asfgit pushed a commit that referenced this pull request Dec 9, 2020
Don't attempt to flush first when cloning, or the audit log message will
show denial on the flush operation, rather than the clone operation
being checked.
asfgit pushed a commit that referenced this pull request Dec 21, 2020
(cherry-picked for 2.0.1)

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.
@ctubbsii ctubbsii added this to the 2.0.1 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants