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.

Sign up for GitHub

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

Jump to bottom

Throw sercurity exceptions when permissions checks fail. Backport to 1.10 #1830

Merged
merged 2 commits into from
Dec 9, 2020
Merged

Throw sercurity exceptions when permissions checks fail. Backport to 1.10 #1830

merged 2 commits into from
Dec 9, 2020

Conversation

Manno15
Copy link
Contributor

@Manno15 Manno15 commented Dec 8, 2020

Based on #1828 in main. This is to back to 1.10. Confirmed the bug did exist on that branch as well.

@Manno15
Copy link
Contributor Author

Manno15 commented Dec 8, 2020

When I get the chance, I plan on adding tests in either ShellIT or ShellServerIT to verify these permissions are checked properly

@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 approved these changes Dec 8, 2020
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

Thanks @Manno15 !

@ctubbsii ctubbsii mentioned this pull request Dec 8, 2020
Merged
@ctubbsii
Copy link
Member

ctubbsii commented Dec 9, 2020 •
edited
Loading

@Manno15 You may want to check that AuditMessageIT passes for you on this branch, because it failed in the main branch until I fixed it in 3b23f6a (also, that might be a good test to update to increase our code coverage for these permissions denials for the flush command, and maybe the set/remove system property commands as well).

@Manno15
Copy link
Contributor Author

Manno15 commented Dec 9, 2020

@ctubbsii I made the changes to AuditMessageIT to get it to run. I will take a look at to see if this is the best spot for the permission denials.

@milleruntime
Copy link
Contributor

Thanks for the quick fix @Manno15 ! Feel free to add the changes to the IT this PR or as a separate PR, whatever is easier.

@Manno15
Copy link
Contributor Author

Manno15 commented Dec 9, 2020

I plan on doing it in a separate pr, I am still debugging some things on it. Is this good to merge? @milleruntime

milleruntime
milleruntime approved these changes Dec 9, 2020
View reviewed changes
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.

Looks good. Sorry I had missed your change to AuditMessageIT

@Manno15 Manno15 merged commit 56142a8 into apache:1.10 Dec 9, 2020
@Manno15 Manno15 mentioned this pull request Dec 9, 2020
Merged
@ctubbsii ctubbsii added this to the 1.10.1 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milleruntime milleruntime milleruntime approved these changes

@ctubbsii ctubbsii ctubbsii approved these changes

Assignees
No one assigned
Labels
blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug.
Projects
None yet
Milestone
1.10.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Manno15 @ctubbsii @milleruntime