Skip to content

[feature] Supported password expiration feature of openwisp-users #491#494

Merged
nemesifier merged 6 commits intomasterfrom
issues/491-password-expiration
Nov 14, 2023
Merged

[feature] Supported password expiration feature of openwisp-users #491#494
nemesifier merged 6 commits intomasterfrom
issues/491-password-expiration

Conversation

@pandafy
Copy link
Member

@pandafy pandafy commented Oct 31, 2023

@coveralls
Copy link

coveralls commented Oct 31, 2023

Coverage Status

coverage: 98.755% (-0.05%) from 98.804%
when pulling 4fbc3e2 on issues/491-password-expiration
into 9ac7164 on master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

A test for this point seems missing:

  • Ensure that the freeradius authorize view won't authorize users with an expired password.

``OPENWISP_RADIUS_DELETE_INACTIVE_USERS``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

**Default**: ``540`` (18 months)
Copy link
Member

Choose a reason for hiding this comment

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

The default here looks incorrect to me.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this still wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referencing the default value? If so, the issue asked for the default value of 18 months.

Copy link
Member

Choose a reason for hiding this comment

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

The values in the README and code do not match, let's set this to disabled by default in both.

is_staff=False,
last_login__lt=timezone.now()
- timedelta(days=app_settings.DELETE_INACTIVE_USERS),
).delete()
Copy link
Member

Choose a reason for hiding this comment

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

Please move this logic to a model method and just call the model method here.
Let's stop putting logic in celery tasks, let's consider these tasks as some sort of views.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are the models which are defined in openwisp-radius:

RadiusCheck
RadiusReply
RadiusAccounting
RadiusGroup
RadiusGroupCheck
RadiusGroupReply
RadiusUserGroup
RadiusPostAuth
Nas
RadiusBatch
RadiusToken
OrganizationRadiusSettings
PhoneToken
RegisteredUser

RegisteredUser seems to be closest with the operation

``OPENWISP_RADIUS_DELETE_INACTIVE_USERS``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

**Default**: ``540`` (18 months)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this still wrong?

@pandafy pandafy force-pushed the issues/491-password-expiration branch from 376c8e9 to 17da885 Compare November 14, 2023 14:18
nemesifier
nemesifier previously approved these changes Nov 14, 2023
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Waiting for openwisp/openwisp-users#363 to be merged before merging.

@nemesifier nemesifier merged commit 7cf4490 into master Nov 14, 2023
@nemesifier nemesifier deleted the issues/491-password-expiration branch November 14, 2023 23:01
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.

[feature] Support password expiration feature of openwisp-users

3 participants