Skip to content

Avoid reattempting authentication for auth versions where it has no effect #188

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
*~
*.pyc
test-env*
junk/
junk/
.idea
14 changes: 14 additions & 0 deletions auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ type Authenticator interface {
Token() string
// The CDN url if available
CdnUrl() string
// Is there a point in reattempting auth in this scheme?
ShouldReattemptAuth() bool
}

// Expireser is an optional interface to read the expiration time of the token
Expand Down Expand Up @@ -130,6 +132,12 @@ func (auth *v1Auth) CdnUrl() string {
return auth.Headers.Get("X-CDN-Management-Url")
}

// v1 Authentication - should reattempt login
func (auth *v1Auth) ShouldReattemptAuth() bool {
// No point in reattempting the login in case of auth failure, the expected result is the same
return false
}

// ------------------------------------------------------------

// v2 Authentication
Expand Down Expand Up @@ -261,6 +269,12 @@ func (auth *v2Auth) CdnUrl() string {
return auth.endpointUrl("rax:object-cdn", EndpointTypePublic)
}

// v2 Authentication - should reattempt login
func (auth *v2Auth) ShouldReattemptAuth() bool {
// As part of the Rackspace auth workaround authentication sometimes needs to be retried
return true
}

// ------------------------------------------------------------

// V2 Authentication request
Expand Down
5 changes: 5 additions & 0 deletions auth_v3.go
Original file line number Diff line number Diff line change
Expand Up @@ -301,3 +301,8 @@ func (auth *v3Auth) Expires() time.Time {
func (auth *v3Auth) CdnUrl() string {
return ""
}

func (auth *v3Auth) ShouldReattemptAuth() bool {
// No point in reattempting the login in case of auth failure, the expected result is the same
return false
}
7 changes: 4 additions & 3 deletions swift.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,8 @@ func (c *Connection) authenticate(ctx context.Context) (err error) {
}
}

retries := 1
shouldReattempt := c.Auth.ShouldReattemptAuth()
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't ever seem to be used?

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't ever seem to be used?

It's checked on line 523 and assigned false on line 524, no?


again:
var req *http.Request
req, err = c.Auth.Request(ctx, c)
Expand All @@ -519,8 +520,8 @@ again:
// Try again for a limited number of times on
// AuthorizationFailed or BadRequest. This allows us
// to try some alternate forms of the request
if (err == AuthorizationFailed || err == BadRequest) && retries > 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this still needs the retries logic otherwise it will retry forever won't it?

Copy link
Author

Choose a reason for hiding this comment

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

I think this still needs the retries logic otherwise it will retry forever won't it?

The inclusion of shouldReattempt in the if-condition and setting it to false if the condition evaluates to true should prevent infinite retries, no?

retries--
if (err == AuthorizationFailed || err == BadRequest) && shouldReattempt {
shouldReattempt = false
goto again
}
return
Expand Down
25 changes: 20 additions & 5 deletions swift_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,19 +335,34 @@ func TestInternalAuthenticate(t *testing.T) {
}
}

func TestInternalAuthenticateDenied(t *testing.T) {
func setVersion(version int) {
c.AuthVersion = version
c.Auth = nil
}

func TestInternalAuthenticateDeniedv2(t *testing.T) {
server.AddCheck(t).Error(400, "Bad request")
server.AddCheck(t).Error(401, "DENIED")
defer server.Finished()
defer setVersion(c.AuthVersion)
setVersion(2)
c.UnAuthenticate()
err := c.Authenticate(context.Background())
if err != AuthorizationFailed {
t.Fatal("Expecting AuthorizationFailed", err)
}
}

func TestInternalAuthenticateDeniedv3(t *testing.T) {
server.AddCheck(t).Error(401, "DENIED")
defer server.Finished()
defer setVersion(c.AuthVersion)
setVersion(3)
c.UnAuthenticate()
err := c.Authenticate(context.Background())
if err != AuthorizationFailed {
t.Fatal("Expecting AuthorizationFailed", err)
}
// FIXME
// if c.Authenticated() {
// t.Fatal("Expecting not authenticated")
// }
}

func TestInternalAuthenticateBad(t *testing.T) {
Expand Down