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

Make errors more distinguishable #24

Closed
wants to merge 1 commit into from

Conversation

keunwoo
Copy link

@keunwoo keunwoo commented Jul 10, 2015

Prior to this commit, this library raised errors either mostly using
errors.New() or directly passing through error values from underlying
libraries. This made it difficult for clients to respond correctly to
the errors that were returned.

This becomes particularly problematic when securecookie is used together
with gorilla/sessions. From an operations standpoint, you often want to
log different errors when the client simply provides an invalid auth
cookie, versus an I/O error fetching data from the session store. The
former probably indicates an expired timestamp or similar client error;
the latter indicates a possible failure in a backend database.

This commit introduces a public Error type, which is now returned
consistently on all errors, as well as an ErrorType enum, which can be
used to distinguish between implementation errors (UsageError and
InternalError) and failed validation of user input (DecodeError).

See also discussion on pull request #9:
#9

Some interface comments on API functions have been clarified and updated
to harmonize with the new error interfaces.

Prior to this commit, this library raised errors either mostly using
errors.New() or directly passing through error values from underlying
libraries.  This made it difficult for clients to respond correctly to
the errors that were returned.

This becomes particularly problematic when securecookie is used together
with gorilla/sessions.  From an operations standpoint, you often want to
log different errors when the client simply provides an invalid auth
cookie, versus an I/O error fetching data from the session store.  The
former probably indicates an expired timestamp or similar client error;
the latter indicates a possible failure in a backend database.

This commit introduces a public Error type, which is now returned
consistently on all errors, as well as an ErrorType enum, which can be
used to distinguish between implementation errors (UsageError and
InternalError) and failed validation of user input (DecodeError).

See also discussion on pull request gorilla#9:
gorilla#9

Some interface comments on API functions have been clarified and updated
to harmonize with the new error interfaces.
@elithrar
Copy link
Contributor

Playing Devil's advocate for a moment—have you considered consolidating on a securecookie.Error interface instead? This is how the net package (amongst others) in the std. library deal with it and it may therefore be more familiar to package users.

Example:

type Error interface {
    error
    // Suggestions/examples only.
    Decode() bool
    Usage() bool
}

type KeyError struct {
    Err error
}

func (ke KeyError) Error() string {
    return ke.Err.Error()
}

func (ke KeyError) Decode() bool {
    return false
}

func (ke KeyError) Usage() bool {
    return true
}

// Other implementations of our securecookie.Error interface as appropriate

// verifyMac verifies that a message authentication code (MAC) is valid.
func verifyMac(h hash.Hash, value []byte, mac []byte) error {
    mac2 := createMac(h, value)
    if subtle.ConstantTimeCompare(mac, mac2) == 1 {
        return nil
    }
    // Wrap the error with a type describing it before returning it.
    return &VerificationError{err}

Package users then check the error type. If they care about the details they can inspect them further:

err := SomeFuncThatCallsEncode()
if err != nil {
switch err.(type):
case securecookie.Error:
    // We now know that we can call err.Usage() or err.Decode() to determine the root
    // cause of the error, if needed. 
default:
    // package users can type switch only on the errors they want to handle
    // as a "special case". All other errors can fall back to a default - 
    // e.g. just raising a blanket HTTP 500 and logging the error internally.
    // Alternatively, they can choose not to do anything and just handle them with the 
    // usual if err != nil { // do something generic } approach and be none the wiser.
}

... or alternatively:

err := s.Encode("mycookie", val)
if err != nil {
    if ue, ok := err.(securecookie.Error); ok {
        // We can check if it's a usage error via ue.Usage()
    }
        // handle the general case
    }
}

The IsErrorType function could be retained as a helper function for its type assertion. I otherwise definitely like the work you've done and agree that distinguishing between errors is important, especially for a package like this with a lot of "failure modes" (user error, en/de -coding errors, etc.).

@keunwoo
Copy link
Author

keunwoo commented Jul 10, 2015

I honestly don't have a strong preference between an Error interface and the version in the last patchset. I was just posting this to move the discussion forward beyond the stale patch in PR #9. For what it's worth, the vast majority of error types in the Go stdlib are structs, not interfaces; net.Error is the exception, not the rule. OTOH, I think the interface version will indeed be slightly more concise for clients than bitmasking ErrorType, as in the last patchset I uploaded. I'll put together a patchset that implements your suggestion and post it in the next 1-2 business days, so that we can compare them.

keunwoo pushed a commit to keunwoo/securecookie that referenced this pull request Jul 20, 2015
Prior to this commit, this library raised errors either mostly using
errors.New() or directly passing through error values from underlying
libraries.  This made it difficult for clients to respond correctly to
the errors that were returned.

This becomes particularly problematic when securecookie is used together
with gorilla/sessions.  From an operations standpoint, you often want to
log different errors when the client simply provides an invalid auth
cookie, versus an I/O error fetching data from the session store.  The
former probably indicates an expired timestamp or similar client error;
the latter indicates a possible failure in a backend database.

This commit introduces a public Error interface, which is now returned
consistently on all errors, and can be used to distinguish between
implementation errors (IsUsage() and IsInternal()) and failed validation
of user input (IsDecode()).

See also discussion on pull requests gorilla#9 and gorilla#24:
gorilla#9
gorilla#24

Some interface comments on other API functions have been clarified and
updated to harmonize with the new error interfaces.
@elithrar
Copy link
Contributor

elithrar commented Aug 6, 2015

Closing due to #28 being merged in.

@elithrar elithrar closed this Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants