-
-
Notifications
You must be signed in to change notification settings - Fork 722
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
Add Authorization header filters #62
Comments
Playing around with this a bit more it seems one way to go would be to make rejections an enum so that those status codes that imply certain headers (eg 401 Unauthorized) can carry the data needed for these headers (eg scheme and realm). I am also thinking of how to return HTTP problems which is the same use case but with the additional need to be even more flexible. I am happy to code up a suggestion, but would appreciate a hint on your preferences what path to take. |
I believe something like this would be awesome! I've been spending time on typed headers, as I expect them to be very useful in warp, and believe they can help with these filters. |
Working on a new approach based on typed headers. Comments welcome: Next step is to allow for a callback in basic() that validates credentials and returns realm. |
Here is an updated version of an authorization filter Very much WIP, though. @seanmonstar Comments very much welcome, specifically:
As a side note: warp is a lovely piece of code and very enjoyable to work with, thanks a lot. |
Interestingly enough, simplifying what I did gravitates towards focussing on the notion and specifications of Auth. schemes as opposed to just credentials in the headers package. Primarily to bind Credentials and Challenge of a scheme together. (Which makes sense, because they are defined as a pair by the specifications. I'll explore that further. |
Since there was some interest recently, I am referencing my current state of work here so you can follow along. The authorization filter is here And the necessary additions to headers are found in The filter usage example is in I am not yet satisfied with how all this plays out, but making it orders of magnitude simpler would in my opinion only be possible be applying strong opinionation towards HTTP authorization and stripping some aspects. I want to steer clear of that. |
I've only just glanced at your implementation, but I'd suggest changing the |
Yeah, definitely :-) I recall that I did not use Option because that did not work with some generics down the line. But I can try again, maybe I meanwhile removed that original cause. |
Any progress on this? |
@bbigras I am sorry - I got absorbed by clients' projects and have no time left at the moment for extra work. Feel free to take over. |
It seems @algermissen has deleted their repository with their work-in-progress. Is there any way to do this without modifying warp? I'm trying to filter by IP address in my case, and there are no examples of the canonical way to do this type of filter in warp. |
@mcginty OMG - sorry. I did not realize the fork contained changes. Damn. Not sure what the plans are in warp, I got distracted from it. I looked locally on my machine but no luck in finding the branch. I'd rather also understand what the roadmap plans are regarding auth. |
@algermissen it happens! thanks for responding at least! |
I think you may find the example of custom rejection useful: Combine that with |
I have been looking over the current state of this effort in #751 and #716 along with what is still available from this conversation. From what I can tell, it seems like the data structures that should be provided in the filter should probably be the typed header inner value's A primary question I have is if we want this filter to be suger for what is currently available via the warp::any().and(warp::header2("Authorization")).map(|header: Authorization<Basic>| {
// check header.0.password() here
}); While this isn't entirely what is provided in previous efforts because those gracefully handle the challenge response, I feel like a provided implementation could be even more useful by providing a filter than can be passed into Maybe I am misunderstanding the usefulness of this approach though, so as a talking point I have put together a module based on implementation of The basic example looks like this: #![deny(warnings)]
use headers::authorization::Basic;
use warp::auth::{basic, Authorizer};
use warp::Filter;
use std::{
collections::HashMap,
sync::{Arc, RwLock},
};
struct Auth {
users: Arc<RwLock<HashMap<&'static str, &'static str>>>,
}
impl Authorizer for Auth {
fn basic(&self, basic: &Basic) -> Result<(), ()> {
let lock = self.users.read().map_err(|_e| {
println!("Locked RwLock from same thread twice....");
})?;
if let Some(pw) = lock.get(basic.username()) {
if *pw == basic.password() {
return Ok(());
}
}
Err(())
}
}
#[tokio::main]
async fn main() {
pretty_env_logger::init();
let mut users = HashMap::new();
users.insert("PersonOne", "CorrectHorseBatteryStaple");
users.insert("PersonTwo", "Hunter2");
let users = Arc::new(RwLock::new(users));
let auth = Auth { users };
let readme = warp::any()
.and(warp::path::end())
.and(warp::fs::file("./README.md"));
let secret_examples = warp::path("ex")
.and(warp::fs::dir("./examples/"))
.with(basic("MyRealm", auth));
let routes = readme.or(secret_examples);
warp::serve(routes).run(([127, 0, 0, 1], 3030)).await;
} You can find a diff of my implementation against master as of today here: Any thoughts about this approach would be appreciated. |
It seems like this |
I would agree, this is a bit an issue with the current implementation linked above. Before spending the time getting that lined up, I am curious if there are preferences around getting auth setup via |
I took a shot at adding support for Authorization header filters. I would like to know whether you like the approach I took: https://github.com/algermissen/warp/blob/add-auth-headers/src/filters/authorization.rs
If so, I'll work on making the code better and submit a PR after that - given I am relatively new to Rust, there is a lot of stuff that needs improvement (eg reduce String usage), so please do not judge that yet.
Some questions:
The text was updated successfully, but these errors were encountered: