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

Fix approval group logging and approval group being bypassed #154

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

dreznicek
Copy link
Contributor

  1. There is a LogDebug Message that is logging the wrong group in the message. It's currently logging the group that the user is a member of and not the approval groups that can approve the command.
  2. The approval logic is being bypassed for users in groups that are assigned a role that has permissions to run a command (BUT, it works if that user is a member of both the group that can execute the command and the approval group assigned)

Description

Fixed the LogDebug line that was showing the Groups that the user belonged in, and not the approval groups.

Next, changed the logic on the Approval Group check to say Hey, are you in the ApprovalGroup? Yeah? Ok, is Peerapproval enabled? Yeah? Then we better get a buddy to approve. Otherwise, you don't need approval, let's execute!

Conversely, Oh you aren't in the Approval Group? Then you definitly need to have approval to run this command.

The reason this logic works is because its already been determined that you have permission to execute this command before the ApprovalNeeded function is even called. Meaning, you wouldn't have gotten here if you didn't, so we're just trying to figure out if:

  1. There is an approval needed for this command?
  2. Are you in the approval group or not?
  3. Is peer approval required?

Related Issue

#153

Motivation and Context

This bug allows any user with permission to execute a command even when the command has an approval configuration setup.

How Has This Been Tested?

We tested by running through the scenarios layed out in the corresponding issue link.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@devblackops
Copy link
Member

Thanks for the fix @dreznicek!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants