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

Executor crashes when failing to authorize against a rule mentioning multiple permissions #758

Closed
christophermaier opened this issue Jun 3, 2016 · 0 comments
Assignees
Labels
Milestone

Comments

@christophermaier
Copy link
Collaborator

christophermaier commented Jun 3, 2016

When failing to authenticate against a rule that mentions multiple permissions, Cog will crash when rendering the error message. All proper authorization checks have taken place, but no output is returned to the user.

The offending code is at

def render({:denied, {%Ast.Rule{}=rule, current_invocation}}),
do: "Sorry, you aren't allowed to execute '#{current_invocation}' :(\n You will need the '#{rule.permission_selector.perms.value}' permission to run this command."

An example of a rule that will trigger this condition is:

when command is twitter:tweet with option[as] == "support" must have twitter:tweet and site:support

If your user does not have both permissions, executing

twitter:tweet --as=support "blah blah blah"

will crash the executor process.

The error rendering code was written assuming a rule would only mention a single permission. This specific failure is caused because the AST node in question is now a Piper.Permissions.Ast.ConditionalExpr, and it has no :perms field.

@christophermaier christophermaier added this to the Cog 0.8.0 milestone Jun 3, 2016
kevsmith pushed a commit that referenced this issue Jun 9, 2016
This commit uses the newly added Permissions.Ast.Rule.permissions_used/1
function to obtain the list of permissions mentioned in a access
rule to generate a better access denied error message. This also
prevents Cog from crashing when rules mention more than one permission.

This isn't an ideal solution as we're not telling the user exactly which
specific permissions they're lacking but it is a step in the right
direction and vastly better than crashing.

50% fix for #758
@kevsmith kevsmith self-assigned this Jun 9, 2016
@kevsmith kevsmith closed this as completed Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants