Skip to content
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

Add session expire functionality based on inactivity #2326

Merged
merged 15 commits into from
Oct 13, 2024

Conversation

ctrl-zzz
Copy link
Contributor

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
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@mlsmaycon
Copy link
Collaborator

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

We will review the feature and give you a feedback ASAP

@ctrl-zzz
Copy link
Contributor Author

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

@mlsmaycon
Copy link
Collaborator

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)
	}
}

@ctrl-zzz ctrl-zzz force-pushed the feat/peer-inactivity branch from 902c88a to 342c939 Compare August 20, 2024 18:15
Copy link

pascal-fischer
pascal-fischer previously approved these changes Oct 10, 2024
mlsmaycon
mlsmaycon previously approved these changes Oct 12, 2024
# Conflicts:
#	management/server/account.go
Copy link

@mlsmaycon mlsmaycon merged commit 49e6510 into netbirdio:main Oct 13, 2024
21 checks passed
@ctrl-zzz ctrl-zzz deleted the feat/peer-inactivity branch October 17, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants