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

Added HTTP Client Proxy Support with Basic Authentication #731

Merged
merged 1 commit into from
Jul 28, 2014
Merged

Added HTTP Client Proxy Support with Basic Authentication #731

merged 1 commit into from
Jul 28, 2014

Conversation

etcimon
Copy link
Contributor

@etcimon etcimon commented Jul 16, 2014

Here's how the proxy client works:

HTTPClientSettings settings = new HTTPClientSettings(new ProxyClientSettings("http://user:[email protected]:3128", ProxyAuth.Basic));

string data;

requestHTTP("http://google.com/", (scope HTTPClientRequest req){},
(scope HTTPClientResponse res)
{
    data = cast(string)res.bodyReader.readAll();
}, settings );

Where 192.168.2.50 has a remote squid proxy server installed and running on port 3128, with the configuration as specified here:

http://howtonixnux.blogspot.ca/2008/03/squid-with-password-authentication.html

to find the htpasswd command, I had to run yum provides \*bin/htpasswd, and ncsa_auth on my server was called basic_ncsa_auth.

@etcimon
Copy link
Contributor Author

etcimon commented Jul 16, 2014

The settings will allow to configure some perks like a CookieJar and redirect follower. I haven't figured out where the connection timeout is though.

@s-ludwig
Copy link
Member

Looks good so far, except for the Json workaround commit, which should be removed (hopefully being solved already anyway, but sanitizing UTF at that point wouldn't generally be a good idea).

About ProxySettings, what about replacing it with a single HTTPClientSettings.proxyURL field? It contains the same fields, except for the resolveHost field.

@etcimon
Copy link
Contributor Author

etcimon commented Jul 18, 2014

All should be cleaned up now, I'm wondering if you'd agree (for the next pull request) to have a @property string sessionID in HTTPServerResponse that simply returns the cookie or sets it if it's empty, because I've had to do this

SessionStore sessionStore;
void init(HTTPServerRequest req, HTTPServerResponse res)
{
    string sessionId = req.cookies["SID"];
    if (!req.session) {
        if (req.cookies.get("SID", null) !is null)
        {
            req.session = sessionStore.open(req.cookies["SID"]);
        }
        else
        {
            req.session = res.startSession();
            sessionId = req.session.id;
        }
    }
}

Which forces every function in my webInterface to have req and res parameters.

I've been using this to enable my own persistent SessionVar-type :

https://gist.github.com/etcimon/86eee4cd50eecde24df4

I'd make one for RedisSet, RedisList, with range support, etc.

The goal would be to expose just a sessionID property in vibe.web.web

@s-ludwig
Copy link
Member

I think that sessionID should rather be a UFCS-able function in vibe.http.session. The server itself is a lower level abstraction and it would be bad to mix the two even more. I'd like to refactor the existing session functionality, too, so that everything is in vibe.http.session. This will completely decouple the two and thus makes for much more flexibility w.r.t. sessions, including plugging an external session system without the standard one getting in the way.

What you do there with the persistent session var or redis vars, IMO, is the job of the SessionStore, at least inside of the vibe.d library. Of course the issue of different serialization formats still isn't solved there, but once that is done, the existing SessionVar should be lowered nicely to Redis types or in whatever form the data is supposed to be stored. However, exposing the session ID in vibe.web.web to enable custom session storage should also be OK, especially as long as the serialization issue isn't solved.


@property string proxyURL() const {
return proxy.toString();
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd really just call it URL proxyURL; and remove the property altogether. URL could maybe get an assignment operator that takes a string, but writing settings.proxyURL = URL("http://.."); also isn't really that bad. But this kind of trivial convenience wrappers is generally discouraged in the code (it blows up the API too much).

@s-ludwig
Copy link
Member

Apart from the naming/property issue, looks good to merge. I'd also slightly refactor how the settings parameter is used, so that it isn't necessary to continuously check for null (by setting it to a &defaultSettings value at the start of the function, if null), but that's not important and can be done later.

Revert Json Exception handling

Added HTTP Status Errors, removed proxy settings class

Fix assertion failure on empty proxy URL

Added a default settings object and lessened the interface of HTTPClientSettings
@etcimon
Copy link
Contributor Author

etcimon commented Jul 19, 2014

Having a default object removes a lot of useless !is, I'll have to remember that

I think that sessionID should rather be a UFCS-able function in vibe.http.session.

Yes, decoupling with a function in vibe.http.session would likely resolve it. I thought of ways to allow multiple session stores to be imported while allowing UFSC-like syntax and came up with this:

Session!T startSession(T, HTTPServerRequest, HTTPServerResponse)(T sessStore, Tuple!(HTTPServerRequest, HTTPServerResponse) context) if (isSessionStore!T)

A Session!T would implement the methods in the current SessionStore interface but type-generic, using composition to pass-through to the sessStore instance (perhaps it could even be cast to the SessionStore interface for compatibility!). In addition you'd also have the T sessStore move the bulk of its (generic) methods in Session!T using a modified std.typecons.proxy mixin or an opDispatch to keep the Session ID in sync, this would help keep the global namespace clean.

This would be most useful for vibe.web.web I think because it could redefine it without the context (req, res):

@property Session!T session(T)(T store) if (isSessionStore!T);

of course a utility function could be made that interacts only with the cookies to build a session ID but it won't be of any use if you agree on this concept being implemented

@etcimon
Copy link
Contributor Author

etcimon commented Jul 19, 2014

Giving it some more thought, I think there should be a trait isGetterSetter that requires a T get(string, T default) and set(string, T) from the passed storage. That way, the CookieJar!T can work exactly the same way as this Session!T with an underlying data store. Also, you can interchange different generic stores for multiple purposes, or even build up custom stores using templates / composition like this:

struct FileStore {
  File file;
  this(string name); // uses this file
  string get(string, string); // implemented with std.algorithm.find and file.byLine
  void set(string, string); // implemented with file.append
}

FileStore fileStore = FileStore("sessions.tmp");
fileStore.startSession();

@s-ludwig
Copy link
Member

I'll need to think about the session support some more (my plan is to use Redis as a session store now, so I'll have some time to actually play with that stuff).

BTW, forgot to merge...

s-ludwig added a commit that referenced this pull request Jul 28, 2014
Added HTTP Client Proxy Support with Basic Authentication
@s-ludwig s-ludwig merged commit 3d76766 into vibe-d:master Jul 28, 2014
@etcimon
Copy link
Contributor Author

etcimon commented Jul 28, 2014

I'll need to think about the session support some more (my plan is to use Redis as a session store now, so I'll have some time to actually play with that stuff).

This is how I'm currently doing it:

https://gist.github.com/etcimon/324f4c79c49b10f67610

This has some variables as global. It would be nice to segment them in the web interfaces but I think this would require some dependency injection as well (see #739), the only way to make this work with a mix of different www. domains / websites currently is to recompile it with different settings. We should only need to decide if it must be in a separate library for now.

ie. I was intending on having domain-specific callbacks in the web interfaces, e.g. void delegate(string user) [] onLogin;, and a domain-specific cache instance, but the graph between instances would be a little complex to resolve if there's dozens of libraries..

@etcimon etcimon deleted the patch3 branch July 30, 2014 18:55
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