Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Forbidden error #187

Merged
merged 1 commit into from
Sep 23, 2016
Merged

Forbidden error #187

merged 1 commit into from
Sep 23, 2016

Conversation

groob
Copy link
Contributor

@groob groob commented Sep 17, 2016

This code is getting pretty ugly, I'll have to find a better way.

The biggest issue right now, is that on a PATCH request, there could be multiple fields that are denied for different reasons. Validation errors have a similar issue.
Typically, you want to bail right away if an error occurs, and when we log that error it's fine.
Here, I want to validate the whole input and build up a list of failed properties and reasons.

Maybe I'm not thinking about it the right way.

JSON output

{
  "message": "Permission Denied",
  "errors": [
    {
      "name": "enabled",
      "reason": "must be an admin"
    },
    {
      "name": "email",
      "reason": "no write permissions on user"
    }
  ]
}

@groob
Copy link
Contributor Author

groob commented Sep 20, 2016

Maybe something like this? https://github.com/hashicorp/go-multierror

@groob
Copy link
Contributor Author

groob commented Sep 20, 2016

@groob
Copy link
Contributor Author

groob commented Sep 20, 2016

Good discussion of the issue here pkg/errors#15

@groob groob removed the In Progress label Sep 22, 2016
allow batching of permission errors
refactor some of the error handling in encodeError
clean up some of the error messages
@groob groob assigned zwass and marpaia and unassigned groob Sep 23, 2016
@@ -17,7 +17,7 @@ func authenticated(next endpoint.Endpoint) endpoint.Endpoint {
return nil, err
}
if !vc.IsLoggedIn() {
return nil, forbiddenError{message: "must be logged in"}
return nil, authError{reason: "must be logged in", clientReason: "must be logged in"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is auth error and all the rest are permission errors because auth errors only apply to the authentication process whereas authorization errors and represented by permissionError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. One ends up in a 401 and the other in a 403 on the client.
The user will get an authentication error if they haven't logged in or their credentials are invalid, and a permission error if they're already auth'd but not allowed to do something.

@marpaia marpaia assigned groob and unassigned marpaia Sep 23, 2016
@groob groob merged commit 1d55969 into kolide:master Sep 23, 2016
@groob groob deleted the forbidden_error branch September 23, 2016 02:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants