Skip to content

Commit

Permalink
oauth-manager: fix evaluation of should refresh function (#280)
Browse files Browse the repository at this point in the history
* oauth-manager: fix evaluation of should refresh function
  • Loading branch information
jkralik authored Jun 2, 2021
1 parent 8b33861 commit dbabe66
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 29 deletions.
2 changes: 1 addition & 1 deletion coap-gateway/schema/device/status/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,5 @@ func (s Status) IsOnline() bool {
if s.ValidUntil <= 0 {
return s.State == State_Online
}
return time.Now().Before(time.Unix(s.ValidUntil, 0))
return time.Now().UnixNano() < time.Unix(s.ValidUntil, 0).UnixNano()
}
2 changes: 1 addition & 1 deletion coap-gateway/service/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

func isExpired(e time.Time) bool {
return !e.IsZero() && time.Now().After(e)
return !e.IsZero() && e.UnixNano() < time.Now().UnixNano()
}

func NewAuthInterceptor() kitNetCoap.Interceptor {
Expand Down
9 changes: 8 additions & 1 deletion coap-gateway/service/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ func (a *authCtx) GetDeviceId() string {
return ""
}

func (a *authCtx) GetAccessToken() string {
if a != nil {
return a.AccessToken
}
return ""
}

const pendingDeviceSubscriptionToken = "pending"

//Client a setup of connection
Expand Down Expand Up @@ -115,7 +122,7 @@ func (client *Client) cancelResourceSubscription(token string, wantWait bool) (b
}

func (client *Client) observeResource(ctx context.Context, deviceID, href string, observable, allowDuplicit bool) (err error) {
log.Debugf("coap-gw: client.observeResource /%v%v ins %v: observe resource", deviceID, href)
log.Debugf("coap-gw: observing resource /%v%v", deviceID, href)
instanceID := getInstanceID(href)
client.observedResourcesLock.Lock()
defer client.observedResourcesLock.Unlock()
Expand Down
3 changes: 2 additions & 1 deletion coap-gateway/service/clientDeleteHandler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ func Test_clientDeleteHandler(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), TestExchangeTimeout)
defer cancel()
req, err := tcp.NewDeleteRequest(ctx, tt.args.path)
require.NoError(t, err)
resp, err := co.Do(req)
assert.NoError(t, err)
require.NoError(t, err)
assert.Equal(t, tt.wantsCode.String(), resp.Code().String())
if tt.wantsContent != nil {
require.NotEmpty(t, resp.Body())
Expand Down
4 changes: 2 additions & 2 deletions coap-gateway/service/devicesStatusUpdater.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (u *devicesStatusUpdater) updateOnlineStatus(client *Client, validUntil tim
return time.Time{}, fmt.Errorf("cannot get service token: %w", err)
}
ctx := kitNetGrpc.CtxWithUserID(kitNetGrpc.CtxWithToken(client.Context(), serviceToken.AccessToken), authCtx.GetUserID())
if authCtx.Expire.Before(validUntil) {
if authCtx.Expire.UnixNano() < validUntil.UnixNano() {
validUntil = authCtx.Expire
}

Expand All @@ -85,7 +85,7 @@ func (u *devicesStatusUpdater) getDevicesToUpdate(now time.Time) []*deviceExpire
case <-d.client.Context().Done():
delete(u.devices, key)
default:
if now.Add(u.deviceStatusValidity / 2).After(d.expires) {
if d.expires.UnixNano() < now.Add(u.deviceStatusValidity/2).UnixNano() {
res = append(res, d)
}
}
Expand Down
3 changes: 3 additions & 0 deletions coap-gateway/service/signUp.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ func signOffHandler(s mux.ResponseWriter, req *mux.Message, client *Client) {
if deviceID == "" {
deviceID = authCurrentCtx.GetDeviceId()
}
if accessToken == "" {
accessToken = authCurrentCtx.GetAccessToken()
}

err := validateSignOff(deviceID, accessToken)
if err != nil {
Expand Down
29 changes: 29 additions & 0 deletions coap-gateway/service/signUp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,32 @@ func TestSignOffHandler(t *testing.T) {
t.Run(test.name, tf)
}
}

func TestSignOffWithSignInHandler(t *testing.T) {
shutdown := setUp(t)
defer shutdown()

co := testCoapDial(t, testCfg.GW_HOST)
if co == nil {
return
}
defer co.Close()

signUpEl := testEl{"signUp", input{coapCodes.POST, `{"di": "` + CertIdentity + `", "accesstoken":"` + oauthTest.DeviceAccessToken + `", "authprovider": "` + oauthTest.NewTestProvider().GetProviderName() + `"}`, nil}, output{coapCodes.Changed, TestCoapSignUpResponse{RefreshToken: "refresh-token", UserID: AuthorizationUserId}, nil}}
testPostHandler(t, uri.SignUp, signUpEl, co)

signInEl := testEl{"signIn", input{coapCodes.POST, `{"di": "` + CertIdentity + `", "uid":"` + AuthorizationUserId + `", "accesstoken":"` + oauthTest.DeviceAccessToken + `", "login": true }`, nil}, output{coapCodes.Changed, TestCoapSignInResponse{}, nil}}
testPostHandler(t, uri.SignIn, signInEl, co)

tbl := []testEl{
{"Deleted", input{coapCodes.DELETE, `{}`, nil}, output{coapCodes.Deleted, nil, nil}},
}

for _, test := range tbl {
tf := func(t *testing.T) {
// delete record for signUp
testPostHandler(t, uri.SignUp, test, co)
}
t.Run(test.name, tf)
}
}
65 changes: 42 additions & 23 deletions pkg/security/oauth/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@ import (

// Manager holds certificates from filesystem watched for changes
type Manager struct {
mutex sync.Mutex
config clientcredentials.Config
requestTimeout time.Duration
tickFrequency time.Duration
startRefreshToken time.Time
token *oauth2.Token
httpClient *http.Client
tokenErr error
doneWg sync.WaitGroup
done chan struct{}
mutex sync.Mutex
config clientcredentials.Config
requestTimeout time.Duration
tickFrequency time.Duration
nextTokenRenewalTime time.Time
token *oauth2.Token
httpClient *http.Client
tokenErr error
doneWg sync.WaitGroup
done chan struct{}
}

// NewManagerFromConfiguration creates a new oauth manager which refreshing token.
Expand All @@ -40,18 +40,19 @@ func NewManagerFromConfiguration(config Config, tlsCfg *tls.Config) (*Manager, e
Transport: t,
Timeout: config.RequestTimeout,
}
token, startRefreshToken, err := getToken(cfg, httpClient, config.RequestTimeout)
token, nextTokenRenewalTime, err := getToken(cfg, httpClient, config.RequestTimeout)
if err != nil {
return nil, err
}
log.Infof("client credential token is refreshed, the next refresh token occurs after %v", nextTokenRenewalTime)

mgr := &Manager{
config: cfg,
token: token,
startRefreshToken: startRefreshToken,
requestTimeout: config.RequestTimeout,
httpClient: httpClient,
tickFrequency: config.TickFrequency,
config: cfg,
token: token,
nextTokenRenewalTime: nextTokenRenewalTime,
requestTimeout: config.RequestTimeout,
httpClient: httpClient,
tickFrequency: config.TickFrequency,

done: make(chan struct{}),
}
Expand All @@ -78,7 +79,23 @@ func (a *Manager) Close() {
}

func (a *Manager) shouldRefresh() bool {
return time.Now().After(a.startRefreshToken)
/*
We cannot use time.Now().After(a.nextTokenRenewalTime ) because
golang using monotonic clock for comparision.
So if we have 2 times:
// update time on PC to future eg: `date MMDDHHMM`
t1 := time.Now() eg (2021-06-15T12:00:00)
// return back time on PC: `date MMDDHHMM`
t2 := time.Now() eg (2021-06-01T12:00:00)
and then you call t2.After(t1) - it's return true :)
more info: https://github.com/golang/go/blob/master/src/time/time.go
the issue can occurs when pc hibernates.
*/

return time.Now().UnixNano() > a.nextTokenRenewalTime.UnixNano()
}

func getToken(cfg clientcredentials.Config, httpClient *http.Client, requestTimeout time.Duration) (*oauth2.Token, time.Time, error) {
Expand All @@ -88,24 +105,26 @@ func getToken(cfg clientcredentials.Config, httpClient *http.Client, requestTime
ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient)

token, err := cfg.Token(ctx)
var startRefreshToken time.Time
var nextTokenRenewalTime time.Time
if err == nil {
now := time.Now()
startRefreshToken = now.Add(token.Expiry.Sub(now) * 2 / 3)
nextTokenRenewalTime = now.Add(token.Expiry.Sub(now) * 2 / 3)
}
return token, startRefreshToken, err
return token, nextTokenRenewalTime, err
}

func (a *Manager) refreshToken() {
token, startRefreshToken, err := getToken(a.config, a.httpClient, a.requestTimeout)
token, nextTokenRenewalTime, err := getToken(a.config, a.httpClient, a.requestTimeout)
if err != nil {
log.Errorf("cannot refresh token: %v", err)
} else {
log.Infof("client credential token is refreshed, the next refresh token occurs after %v", nextTokenRenewalTime)
}
a.mutex.Lock()
defer a.mutex.Unlock()
a.token = token
a.tokenErr = err
a.startRefreshToken = startRefreshToken
a.nextTokenRenewalTime = nextTokenRenewalTime
}

func (a *Manager) watchToken() {
Expand Down

0 comments on commit dbabe66

Please sign in to comment.