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

How to add an Authorization layer to Client or RequestBuild ? #346

Closed
Byron opened this issue Feb 27, 2015 · 11 comments
Closed

How to add an Authorization layer to Client or RequestBuild ? #346

Byron opened this issue Feb 27, 2015 · 11 comments

Comments

@Byron
Copy link
Contributor

Byron commented Feb 27, 2015

When looking at the google API implementations, you usually see the pattern that services are instantiated with an http client, through which all requests will be performed. Authorisation is handled through a special version of an HTTP-Client, which will insert the respective headers on the fly and even refresh expired tokens when needed.

As Client has no trait, and as the only things I can affect are the NetworkConnector and NetworkStream, both of which don't have access to the Header anymore, I wonder how I would best achieve that without working around Hyper entirely, in that case.

Sorry again for posting a question here, just let me know if you would like me to re-post on StackOverflow.

Thank you

@Byron Byron changed the title How to adding Authorization layer to Client or RequestBuild How to adding Authorization layer to Client or RequestBuild ? Feb 27, 2015
@pyfisch
Copy link
Contributor

pyfisch commented Feb 28, 2015

Actually hyper does not have a function to provide authorisation. But you can create a new client by wrapping client and passing all functions down to the original client.

Pseudocode:

pub struct AuthClient{
  client: hyper::Client,
  credentials: Credentials,
}

impl AuthClient {
    // All the same methods as hyper::Client plus some for auth
}

@Byron
Copy link
Contributor Author

Byron commented Feb 28, 2015

Thank you.
But how would aggregation work within generics ? The APIs I write use generics a lot, and these need traits.
Unfortunately, there is no Client trait yet.
Even better than that would be if there was a RequestBuilder trait, and a generic parameter for it in the Client. That way, I could sneak in my own which could insert the respective headers.
Maybe something along the lines of this:

pub struct Client<C, B: Builder> {
    connector: C,
    redirect_policy: RedirectPolicy,

   _m: PhantomData<B>,
}

@Byron
Copy link
Contributor Author

Byron commented Mar 17, 2015

Just for the record

The original issue was not solved. A hyper Client is missing a trait, which makes it more difficult to implement new kinds of Clients, some of which may automatically deal with authentication under the hood.
As the RequestBuilder is finally issuing the send() call, it would also be sufficient to be able to replace the RequestBuilder with an own implementation, possibly by adding a generic parameter for it in the Client type. After all, most people would want to inject an Authorization header field.

Closing the issue without actually dealing with the described problem is behaviour I do not approve of. In case the point made here is not clear or faulty, then this was not made clear in the previous communication either.

@reem
Copy link
Contributor

reem commented Mar 17, 2015

Hey @Byron, if you feel like there's more to discuss to have in this issue, I'm happy to reopen. I noticed that @seanmonstar closed this, but the issue also looked resolved to me as @pyfisch told you what to do in this case. On closer inspection, I can see how the solution is not entirely satisfactory.

I agree that there's definitely more to do in terms of API surface to allow more complex modifications or extensions to Client and RequestBuilder, but that seems only tangentially related to this issue. I'd rather open another issue and link to this one from there though, since the major point of this issue has sort of been dealt with. Sound good?

@Byron Byron changed the title How to adding Authorization layer to Client or RequestBuild ? How to add an Authorization layer to Client or RequestBuild ? Mar 17, 2015
@Byron
Copy link
Contributor Author

Byron commented Mar 17, 2015

Considering I didn't (even) get the subject's grammar right, I'd be happy if you would open an issue that is more to the point, the way you see it by now.
After all, I still believe that hyper can improve in the realm of extensibility, and should pursue this path further.
Thank you

@reem
Copy link
Contributor

reem commented Mar 17, 2015

Filed #378 :)

@seanmonstar
Copy link
Member

@Byron sorry, I didn't mean to just shrug this off. I read @pyfisch's response and felt it was sufficient.


As for this specific case, I don't think Client nor RequestBuilder require traits. I don't believe they need to be generic over something. I'll show how I would imagine adding an Auth (and any other kind of extension) to the Client.

Similar in nodejs, where there is the request package on npm, and http as a standard module, I'd imagine people could build specialized clients on top of the one hyper provides. For instance, with request, you can write:

var request = require('request');

request.post({
  url: 'https://example.domain/path',
  auth: someAuthValue
}).pipe(process.stdout);

I'd imagine a specialized client looking like this:

struct Client {
    inner: hyper::Client,
    auth: String
}

impl Client {
    fn request(&mut self, method: hyper::Method, url: url::Url) -> hyper::client::RequestBuilder {
        self.inner.request(method,url)
            .header(hyper::header::Authorization(self.auth.clone()))
    }
}

You could imagine any sorts of options that a specialized client could handle, such as oauth, json, tunnel, gzip, hawk, multipart, etc. Actually, there is already a multipart crate using hyper.

@Byron
Copy link
Contributor Author

Byron commented Mar 17, 2015

@seanmonstar It's alright :) - I might have been reacting a bit strongly to this too, had a bad day.

The reason I seem to be so much into having a Client trait primarily because I see the Go code for youtube APIs which is happy just with an OAuth-enabled client. I thought it might be a wrong move to enforce my own Trait to make that usable with generics, and would have preferred such a 'tool' to be delivered by hyper.

func New(client *http.Client) (*Service, error) {
    if client == nil {
        return nil, errors.New("client is nil")
    }
    s := &Service{client: client, BasePath: basePath}
    s.Userinfo = NewUserinfoService(s)
    return s, nil
}

By now I feel differently about it. Currently I just take a hyper-client and an authenticator, which takes care about providing tokens for authentication. That looks very much like this and feels OK to me, at least on the client side.

On the side of the implementor, you see that not having a Client trait forces me to carry around the NC type parameter. Maybe there are other ways to get rid of it though ... after all, I am only interested in the hyper client and don't care what connection mechanism it might use.

Maybe some day there will be a clear idea on how these cases can and should be tackled, and we do the right thing™ to our APIs to make them as nice and easy to use as possible.

@seanmonstar
Copy link
Member

I see. That's no fun carrying around that generic, since you don't care about it. Client could be changed to have Client { connector: Box<NetworkConnector>, .. } instead of being generic. That would mean dynamic calls instead of static dispatch for everyone, though.

@seanmonstar
Copy link
Member

Though, we already do this for Request and Response, for the exact same reason: so people don't have worry about the exact type of the Stream underneath... and it doesn't seem to be a source of slowness.

@reem
Copy link
Contributor

reem commented Mar 17, 2015

Compared to the cost of other operations, mainly io, the virtual calls seem to not matter at all.

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

No branches or pull requests

4 participants