Skip to content

Commit

Permalink
Update broker to not rely on offline expiration
Browse files Browse the repository at this point in the history
We used to have a limited amount of time for the user to authenticate
offline. Now we allow offline authentication as long as the user have a
token.
  • Loading branch information
denisonbarbosa committed Jun 19, 2024
1 parent 70f110d commit 4d1fc4d
Show file tree
Hide file tree
Showing 10 changed files with 26 additions and 33 deletions.
16 changes: 2 additions & 14 deletions internal/broker/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -592,27 +592,15 @@ func (b *Broker) loadAuthInfo(session *sessionInfo, password string) (loadedInfo
return authCachedInfo{}, false, fmt.Errorf("could not unmarshal token: %v", err)
}

// If the token is not expired, we should use the cached information.
if cachedInfo.Token.Valid() {
return cachedInfo, true, nil
}

// Tries to refresh the access token. If the service is unavailable and token is still valid from the broker
// perspective (i.e. last modification was between now and now-expiration), we can use it.
// Tries to refresh the access token. If the service is unavailable, we allow authentication.
tok, err := b.auth.oauthCfg.TokenSource(context.Background(), cachedInfo.Token).Token()
if err != nil {
castErr := &oauth2.RetrieveError{}
if !errors.As(err, &castErr) || castErr.Response.StatusCode != http.StatusServiceUnavailable {
return authCachedInfo{}, false, fmt.Errorf("could not refresh token: %v", err)
}

if time.Since(cachedInfo.AcquiredAt) >= b.offlineExpiration {
return authCachedInfo{}, false, errors.New("token exceeded offline expiration")
}

// If we get here, it means that the token is expired and we could not refresh it
// but we can still use it locally due to offline expiration configuration
// However, we don't want to update the token file neither its saved files.
// The provider is unavailable, so we allow offline authentication.
return cachedInfo, true, nil
}

Expand Down
27 changes: 12 additions & 15 deletions internal/broker/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ func TestIsAuthenticated(t *testing.T) {
wantSecondCall bool

preexistentToken string
offlineExpiration string
invalidAuthData bool
dontWaitForFirstCall bool
readOnlyCacheDir bool
Expand All @@ -267,10 +266,16 @@ func TestIsAuthenticated(t *testing.T) {

"Authenticating with qrcode reacquires token": {firstChallenge: "-", wantSecondCall: true, preexistentToken: "valid"},
"Authenticating with password refreshes expired token": {firstMode: "password", preexistentToken: "expired"},
"Authenticating with password still allowed if server is unreachable and token is not offline-expired": {
firstMode: "password",
preexistentToken: "expired",
offlineExpiration: "10",
"Authenticating with password still allowed if server is unreachable": {
firstMode: "password",
preexistentToken: "valid",
customHandlers: map[string]testutils.ProviderHandler{
"/token": testutils.UnavailableHandler(),
},
},
"Authenticating with password still allowed if token is expired and server is unreachable": {
firstMode: "password",
preexistentToken: "expired",
customHandlers: map[string]testutils.ProviderHandler{
"/token": testutils.UnavailableHandler(),
},
Expand All @@ -283,17 +288,9 @@ func TestIsAuthenticated(t *testing.T) {
"Error when IsAuthenticated is ongoing for session": {dontWaitForFirstCall: true, wantSecondCall: true},

"Error when mode is password and token does not exist": {firstMode: "password"},
"Error when mode is password and server offline and token expired": {
"Error when mode is password but server returns error": {
firstMode: "password",
preexistentToken: "expired",
customHandlers: map[string]testutils.ProviderHandler{
"/token": testutils.UnavailableHandler(),
},
},
"Error when mode is password and token is offline-valid but server returns error": {
firstMode: "password",
preexistentToken: "expired",
offlineExpiration: "10",
customHandlers: map[string]testutils.ProviderHandler{
"/token": testutils.BadRequestHandler(),
},
Expand Down Expand Up @@ -348,7 +345,7 @@ func TestIsAuthenticated(t *testing.T) {
defer cleanup()
provider = p
}
b, sessionID, key := newBrokerForTests(t, cacheDir, provider.URL, tc.offlineExpiration, tc.sessionMode)
b, sessionID, key := newBrokerForTests(t, cacheDir, provider.URL, tc.sessionMode)

if tc.preexistentToken != "" {
tok := generateCachedInfo(t, tc.preexistentToken, provider.URL)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
access: granted
data: |-
{"userinfo": {
"name": "test-user@email.com",
"uuid": "saved-user-id",
"gecos": "saved-user",
"dir": "/home/test-user@email.com",
"shell": "/usr/bin/bash",
"groups": [ {"name": "remote-group", "ugid": "12345"}, {"name": "linux-local-group", "ugid": ""} ]
}}
err: <nil>

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ data: |-
"gecos": "saved-user",
"dir": "/home/test-user@email.com",
"shell": "/usr/bin/bash",
"groups": [{"name": "saved-remote-group", "gid": "12345"}, {"name": "saved-local-group", "gid": ""}]
"groups": [ {"name": "remote-group", "ugid": "12345"}, {"name": "linux-local-group", "ugid": ""} ]
}}
err: <nil>

0 comments on commit 4d1fc4d

Please sign in to comment.