Improves the authentication flow

[chripo]

The main goal was to simplify the req method in
request.go and making it easier to add more authentication methods.

All the magic went into the auth.go file.

This feature introduces an Authorizer which acts as an
Authenticator factory. Under the hood it creates an authenticator
shim per request, which delegates the authentication flow
to our authenticators.

The authentication flow itself is broken down into Authorize' and Verify' steps to encapsulate and control complex authentication challenges.

Furthermore, the default NewAutoAuth authenticator can be overridden
by a custom implementation for more control over flow and resources.
The NewBacisAuth Authorizer gives us the feel of the old days.

[chripo]

Thanks @ueffel!
Any thoughts on the basic strategy of the game?
Is it clear or easier to gasp?

[ueffel]

The Authorizer and Authenticator look easy enough to implement for a third party. Maybe @Gamer92000 can checkout if this new API simplifies #70.
Overall it looks good to me.

[ueffel]

It may be interesting to try implementing Kerberos authentication without wrapping the http.Transport (which is need with github.com/dpotapov/go-spnego)
I think I will try that to check how it goes.

[ueffel]

I think there should be a way to create an Authorizer without the Basic and Digest methods, so one can just add (AddAuthenticator) the Authenticators, that are wanted.

func NewEmptyAuth() Authorizer { fmap := make(map[string]AuthFactory) az := &authorizer{fmap, sync.Mutex{}, &nullAuth{}} return az }

And maybe one Authorizer that does authenticates preemptively:

func NewPreemptiveAuth(auther Authenticator) Authorizer { fmap := make(map[string]AuthFactory) az := &authorizer{fmap, sync.Mutex{}, auther} return az }

Minor inconvience newPathError and newPathErrorErr could be public, so they can be reused.

Sample kerberos authentication

package main import ( "net/http" "os" "github.com/dpotapov/go-spnego" "github.com/studio-b12/gowebdav" ) type krb5Auth struct { sspi spnego.Provider NoCanonicalize bool } func NewKrb5Auth() *krb5Auth { return &krb5Auth{sspi: spnego.New()} } func (k *krb5Auth) Authorize(c *http.Client, rq *http.Request, method string, path string) error { return k.sspi.SetSPNEGOHeader(rq, !k.NoCanonicalize) } func (k *krb5Auth) Verify(rq *http.Request, rs *http.Response, method string, path string) (bool, error) { if rs.StatusCode == http.StatusUnauthorized { return false, &os.PathError{ Op: "Authorize", Path: path, Err: gowebdav.StatusError{Status: rs.StatusCode},	}	} return false, nil } func (k *krb5Auth) Clone() gowebdav.Authenticator { auther := NewKrb5Auth() auther.NoCanonicalize = k.NoCanonicalize return auther } func (k *krb5Auth) Close() error { return nil } var _ gowebdav.Authenticator = (*krb5Auth)(nil)

Usage like this

authorizer := gowebdav.NewEmptyAuth() authorizer.AddAuthenticator("negotiate", func(rq *http.Request, rs *http.Response, method, path string) (auth gowebdav.Authenticator, err error) { return NewKrb5Auth(), nil })

or

authorizer := gowebdav.NewPreemptiveAuth(NewKrb5Auth())

[chripo]

awesome!
for kerberos only auth it could shipped with a authorizer like the BasicAuth does.

func NewBasicAuth(login, secret string) Authorizer { return &basicAuthAuthorizer{&BasicAuth{login, secret}} }

[ueffel]

awesome! for kerberos only auth it could shipped with a authorizer like the BasicAuth does.

func NewBasicAuth(login, secret string) Authorizer { return &basicAuthAuthorizer{&BasicAuth{login, secret}} }

Yeah, but this requires far more code 😇.

Also I thought about what would happen, if a server offers multiple ways to authenticate (multiple Www-Authenticate headers) and the first one (what rs.Header.Get() returns) is not the one that should be taken by the client.
I think right now this wouldn't work.

(Edit: wouldn't work)

[chripo]

Yeah, but this requires far more code innocent.

I LIKE U

Also I thought about what would happen, if a server offers multiple ways to authenticate (multiple Www-Authenticate headers) ....
we ship this since 2014, so it works(tm)
but it's a good thought. we need to sort the Authenticators and pull out all headers.
PR welcome.

[chripo]

Minor inconvience newPathError and newPathErrorErr could be public, so they can be reused.

we will take care about that too.

[ueffel]

@chripo
Have a look at the "next-plus" branch
I tried myself at supporting multiple Www-Authenticate header but i'm not happy with the solution. Too many new struct members for the authorizer. Maybe you have an idea to make it better?

[chripo]

@ueffel
indeed that a lot of changes.
take a look at 9da199c this should be enough to favor the order if the server supports multiple authentication methods.

[ueffel]

take a look at 9da199c this should be enough to favor the order if the server supports multiple authentication methods.

but this still only looks at the first authentication header header := strings.ToLower(rs.Header.Get("Www-Authenticate")) instead of all of them headers := rs.Header.Values("Www-Authenticate")

[chripo]

In fact, I was looking up in the wrong docs, somehow...
This needs some more work, I've got an idea.

[chripo]

Well, I'm done so far.
@ueffel What do you think about the last changes?

What needs to be done to get merged:

• add some auth flow / API documentation and examples of how to enroll a custom authentication (Readme)
• clarify how to deal with 3rd party authentication that pulls in dependencies
• more reviews
• feat(auth): add support for MS-PASS #70 may show up with more changes, hopefully not

[ueffel]

We are on a very good way to make this awesome :)

Also: At least the multiAuthTest from my branch made it :D

[chripo]

thank you again @ueffel! Absolut Fett!
your tests made it awesome.
I'll pull them off at the final squash / end, that you can apply them by our own to preserve ownership!!

[chripo]

@zekroTJA Do you have time for a feature review?

[zekroTJA]

@zekroTJA Do you have time for a feature review?

Yeah, I would take a look into it in the next couple of days if that fits. I am not that deep into the WebDav protocol itself but I'll take a look into it. :)

[zekroTJA]

Looks good so far. Just some code style things and proposes for avoiding panics in the package.

Some of my annotations are based on personal preference and may be a bit nitpicky, but I am open for discussion. :)

[zekroTJA]

I would strongly avoid panicing in packages. Especially in a method which is "hidden" behind an interface. I would split the Authenticatior interface into something like the following.

type Authenticator interface { Authorize(c *http.Client, rq *http.Request, path string) error Verify(c *http.Client, rs *http.Response, path string) (redo bool, err error)	io.Closer } type ClonableAuthenticator interface { Authorize(c *http.Client, rq *http.Request, path string) error Verify(c *http.Client, rs *http.Response, path string) (redo bool, err error)	io.Closer Clone() ClonableAuthenticator }

Another approach would be to make Authenticators not clonable by default, adding a Clonable interface and checking the clonability when required.

type Authenticator interface { Authorize(c *http.Client, rq *http.Request, path string) error Verify(c *http.Client, rs *http.Response, path string) (redo bool, err error)	io.Closer } type Clonable interface { Clone() interface{} }

Then, the clone call could look something like this.

var a, cloned Authenticator = // ... if ac, ok := a.(Clonable); ok { cloned = a.Clone().(Authenticator) }

Another approach would be to let Clone() return the cloned Authenticator as well as an error. Then, this implementation could return an "not implemented" error. But actually, I am not a huge fan of introducing an error return just for the sake of covering "not implemented" errors.

[chripo]

Thanks for the suggestions.
I'm not so comfortable with it because it adds more code and introduces checks in other parts of our code.
In this particular case, it now returns a noAuth instance instead of a panic.

[zekroTJA]

Same here like with Clone. Maybe proceed here as I proposed above.

[chripo]

What would an user expect?

b:= &BasicAuth{user: "user", pw: "password"} a:= NewPreemptiveAuth(b) a.AddAuthenticator("fooAuth", func(...) (auth Authenticator, err error) { return &fooAuth{...}, nil })

In this particular case I'm opt for the panic, because it's only a convenience method which has to be setup manually.

[Gamer92000]

Hey, sorry to bother you. Could you get this resolved and merged? This is needed for #70 which in turn would benefit a lot of projects downstream (e.g. rmfakecloud).