Let oxide_auth_rocket::OAuthFailure be a #[non_exhaustive] enum

[lfxgroove]

Project Improvement

Currently oxide_auth_rocket::OAuthFailure is a struct with a private inner field Kind which one can retrieve variants from via the oauth() and web() methods. If you want to roll your own error type (since OAuthFailure is not stable) you have to do some cumbersome matching that feels a bit un-rusty:

if let Some(web_failure) = failure.web() {
    //...
} else if let Some(oauth_failure) = failure.oauth() {
    //...
}

Turning the failure type into a non-exhaustive enum would allow users to match on the enum instead of having to call functions while still allowing the project to add new failure modes.

Other context

It just feels like a more ergonomic interface to use.

Tracking pull request

• A pull request does not yet exist, I could create it if this seems like a reasonable request.

[HeroicKatora]

When implementing the type, I wasn't really expecting it to be used for storing an error type. It's mostly a wrapper that implements Responder<'r> and not much else, since it can't implement the trait for the foreign OAuthError type from oxide-auth. I don't think it's actually used in any of the functions or flows. I might misunderstand the problem though. What's the use that requires it to be stored?

[lfxgroove]

I don't really require it to be stored, I just realized that I probably worded the op a bit strangely. I'm currently trying to implement FromRequest to protect resources, in there I've got something like this in the error case when calling execute() for a resource flow:

match protection {
    Ok(_grant) => {
        Outcome::Success(())
    },
    Err(e) => {
        match e {
            Ok(ok) => {
                let resp: RocketResponse = ok.into();
                Outcome::Failure((resp.status(), resp))
            },
            Err(err) => {
                // Logic here is the same as in oxide_auth_rocket/failure.rs, the Responder
                // impl for OAuthFailure
                if let Some(oauth) = err.oauth() {
                    match oauth {
                        OAuthError::BadRequest | OAuthError::DenySilently => {
                            Outcome::Failure((Status::BadRequest, RocketResponse::new()))
                        },
                        OAuthError::PrimitiveError => {
                            Outcome::Failure((Status::InternalServerError, RocketResponse::new()))
                        }
                    }
                } else if let Some(_web) = err.web() {
                    Outcome::Failure((Status::BadRequest, RocketResponse::new()))
                } else {
                    unreachable!();
                }
            },
        }
    }
}

To me it would make more sense to match on err instead of using a chain of if let in the Err(Err(..)) case, e.g:

match err {
    OAuthFailure::OAuth(oauth_err) => {},
    OAuthFailure::Web(web_err) => {},
    _ => unreachable!(), // or some other kind of handling
}

Does that make sense?

I guess a follow-up question is if there's a better way to handle this in general, currently I've just copied the implementation details from the Responder impl for oxide_auth_rocket/failure.rs since I'm not really sure how to use a Responder to create a Outcome, the status code is what's tripping me up. I'll dig a bit into this and see if I can't figure something out.

[HeroicKatora]

If you want to treat use a different error that's fine. You only have to implement the Endpoint trait with the one you intend to use. Then the type of err in Err(err) => is different and you don't have that problem. As for the Responder trait impl you'll want to consult the documentation of rocket.

[lfxgroove]

I see, thanks for the info!

I guess you do not want to replace oxide_auth_rocket::OAuthFailure with the enum oxide_auth_rocket::Kind to enable matching instead of calling web() and oauth()?

[HeroicKatora]

Hm, I hadn't considered that #[non_exhaustive] was not actually available when I first implemented this type 🙂 So changing it to Kind and adding the attribute would be a more modern alternative. I'm inclined to agree with that replacement.