netbird icon indicating copy to clipboard operation
netbird copied to clipboard

Add session expire functionality based on inactivity

Open ctrl-zzz opened this issue 1 year ago • 4 comments

Describe your changes

I have implemented a session timeout feature based on inactivity, forcing re-authentication with every netbird up. This was necessary because, while evaluating if Netbird was suitable for my use case, I noticed that authentication is only required after the login expiration period has elapsed. For security reasons, I wanted access to be required with every connection: this way, the authentication process with the identity provider is triggered and, by using a session timeout setting in Keycloak, credentials are requested each time.

Building on the existing approach for login expiration, I implemented inactivity expiration by checking the status of a peer: after a configurable period of time following netbird down, the peer shows login required. I also added a setting, inactivityExpirationEnabled, that can enable or disable the inactivity expiration feature through the APIs.

I needed this functionality due to a business requirement and thought it might be useful to you as well.

Checklist

  • [ ] Is it a bug fix
  • [ ] Is a typo/documentation fix
  • [x] Is a feature enhancement
  • [ ] It is a refactor
  • [ ] Created tests that fail without the change (if possible)
  • [ ] Extended the README / documentation, if necessary

ctrl-zzz avatar Jul 25 '24 16:07 ctrl-zzz

Hello @ctrl-zzz can you check the failing tests and Sonar checks?

We will review the feature and give you a feedback ASAP

mlsmaycon avatar Aug 01 '24 14:08 mlsmaycon

Hello, I believe they are about the Cognitive Complexity of some methods. Since it's a new feature I added some new properties that had to be checked as well for some functionalities. I guess there needs to be less conditions to be checked, but not checking them wouldn't make the methods work as intended

ctrl-zzz avatar Aug 20 '24 16:08 ctrl-zzz

Hello, I believe they are about the Cognitive Complexity of some methods. Since it's a new feature I added some new properties that had to be checked as well for some functionalities. I guess there needs to be less conditions to be checked, but not checking them wouldn't make the methods work as intended

thanks, there are some optimizations that can done, see some suggestions below:

For func (am *DefaultAccountManager) UpdateAccountSettings, you can move some of the new checks into a separate method or function, and that should reduce complexity.

For func (am *DefaultAccountManager) MarkPeerConnected you can rewrite it like this:

	
if peer.AddedWithSSOLogin() {
       if  peer.LoginExpirationEnabled && account.Settings.PeerLoginExpirationEnabled {
		am.checkAndSchedulePeerLoginExpiration(ctx, account)
	}
	
        if peer.InactivityExpirationEnabled && account.Settings.PeerInactivityExpirationEnabled {
		am.checkAndSchedulePeerInactivityExpiration(ctx, account)
	}
}

mlsmaycon avatar Aug 20 '24 17:08 mlsmaycon