Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Initial support for v3 userpass authentication. #15
Conversation
urosj
reviewed
Aug 28, 2015
| + Name string `json:"name,omitempty"` | ||
| +} | ||
| + | ||
| +// v3AuthToken contains the token to use for token authentication. |
urosj
Aug 28, 2015
Owner
I'm not familiar with the details, but does this cover all types of tokens? the uuid, pki and fernet one?
|
code LGTM, I'm not entirely convinced the AuthV3UserPass is the right way and that maybe we should introduce keystone api version attribute instead. |
dimitern
reviewed
Aug 28, 2015
| + AuthLegacy = AuthMode(iota) // Legacy authentication | ||
| + AuthUserPass // Username + password authentication | ||
| + AuthKeyPair // Access/secret key pair authentication | ||
| + AuthV3UserPass //Username + password authentication (v3 API) |
dimitern
reviewed
Aug 28, 2015
| @@ -28,6 +29,8 @@ func (a AuthMode) String() string { | ||
| return "Legacy Authentication" | ||
| case AuthUserPass: | ||
| return "Username/password Authentication" | ||
| + case AuthV3UserPass: | ||
| + return "Username/password (Version 3) Authentication" |
dimitern
Aug 28, 2015
Username/password Authentication (Version 3) ?
To minimize confusion, we should perhaps clarify which version the original AuthUserPass is using.
dimitern
reviewed
Aug 28, 2015
| + Name string `json:"name,omitempty"` | ||
| +} | ||
| + | ||
| +/// / V3UserPass is an Authenticator that will perform username + password |
dimitern
reviewed
Aug 28, 2015
| + | ||
| +type v3TokenEndpoint struct { | ||
| + ID string `json:"id"` | ||
| + Region string `json:"region"` |
dimitern
Aug 28, 2015
According to the official API docs - http://developer.openstack.org/api-ref-identity-v3.html
this should be "region_id" and RegionID respectively.
dimitern
reviewed
Aug 28, 2015
| +} | ||
| + | ||
| +type v3TokenProject struct { | ||
| + ID string `json:"id"` |
dimitern
Aug 28, 2015
Why omit the optional domain section here (see the same link above - esp. the examples)?
dimitern
reviewed
Aug 28, 2015
| +} | ||
| + | ||
| +type v3TokenUser struct { | ||
| + ID string `json:"id"` |
dimitern
reviewed
Aug 28, 2015
| + Methods []string `json:"methods"` | ||
| + Catalog []v3TokenCatalog `json:"catalog"` | ||
| + Project v3TokenProject `json:"project"` | ||
| + User v3TokenUser `json:"user"` |
dimitern
reviewed
Aug 28, 2015
| + if tok == "" { | ||
| + return nil, fmt.Errorf("authentication failed") | ||
| + } | ||
| + rsu := make(map[string]ServiceURLs, len(resp.Token.Catalog)) |
dimitern
Aug 28, 2015
If I'm reading the API docs correctly, this code only caters for the case when "catalog" is returned inside a "token", but not when there is only "token", without "catalog" inside it.
dimitern
reviewed
Aug 28, 2015
| + c.Assert(auth.TenantId, gc.Equals, userInfo.TenantId) | ||
| +} | ||
| + | ||
| +func (s *V3UserPassTestSuite) TestAuthWithCatalog(c *gc.C) { |
dimitern
Aug 28, 2015
Please, add another test where catalog is missing, but token is present (see earlier comment).
dimitern
reviewed
Aug 28, 2015
| @@ -56,7 +56,7 @@ var authTemplate = `{ | ||
| }` | ||
| func userPassAuthRequest(URL, user, key string) (*http.Response, error) { | ||
| - client := &http.DefaultClient |
dimitern
Aug 28, 2015
Why? It's quite intentional to use a pointer here, instead of a copy.
If you needed to copy http.DefaultClient in order for the tests to pass, something is wrong.
mhilton
Aug 28, 2015
Member
It was indeed to make the tests compile.
http.DefaultClient is already a pointer (http://golang.org/pkg/net/http/#pkg-variables)
dimitern
reviewed
Aug 28, 2015
| @@ -30,7 +30,7 @@ func (s *LegacySuite) setupLegacy(user, secret string) (token, managementURL str | ||
| } | ||
| func LegacyAuthRequest(URL, user, key string) (*http.Response, error) { | ||
| - client := &http.DefaultClient |
dimitern
reviewed
Aug 28, 2015
| + Methods []string `json:"methods"` | ||
| + Password struct { | ||
| + User struct { | ||
| + Name string `json:"name"` |
dimitern
reviewed
Aug 28, 2015
| + } `json:"identity"` | ||
| + Scope struct { | ||
| + Project struct { | ||
| + Name string `json:"name"` |
dimitern
reviewed
Aug 28, 2015
| + | ||
| +type V3Endpoint struct { | ||
| + Interface string `json:"interface"` | ||
| + Region string `json:"region"` |
dimitern
reviewed
Aug 28, 2015
| + ID string `json:"id"` | ||
| + Name string `json:"name"` | ||
| + } `json:"user"` | ||
| +} |
dimitern
reviewed
Aug 28, 2015
| @@ -0,0 +1,198 @@ | ||
| +package identityservice |
dimitern
Aug 28, 2015
It looks like a lot of duplicated code from userpass.go - why not embed UserPass within V3UserPass and reuse it, overriding only where necessary?
dimitern
reviewed
Aug 28, 2015
| + | ||
| +// setupHTTP attaches all the needed handlers to provide the HTTP API. | ||
| +func (u *V3UserPass) SetupHTTP(mux *http.ServeMux) { | ||
| + mux.Handle("/v3/auth/tokens", u) |
dimitern
Aug 28, 2015
NOTE: Because V3UserPass handles a more specific URL paths, it needs to be registered before the older UserPass.
dimitern
reviewed
Aug 28, 2015
| @@ -0,0 +1,146 @@ | ||
| +package identityservice |
dimitern
Aug 28, 2015
Similarly here - to reduce the code duplication I'd suggest extracting a BaseUserPassSuite with the common code, then embed it into UserPassSuite and V3UserPassSuite.
dimitern
commented
Aug 28, 2015
|
NOT LGTM I'm afraid - it needs some clean up first and reduction of the amount of duplicated code. |
|
Closing this PR as support for v3 auth has landed recently. |
mhilton commentedAug 27, 2015
This is support for v3 authentication, currently only using the default domain.