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

[frontend/backend] Adds account status and lockout functionality by expire date #3303

Merged
merged 13 commits into from
Aug 25, 2023

Conversation

ParamConstructor
Copy link
Contributor

@ParamConstructor ParamConstructor commented May 9, 2023

Proposed changes

  • Adds the following fields to the User schema (account_status and account_lock_after_date)
  • account_status is an ENUM of Active, Inactive, LockedTraining, or Locked
    • Active - normal account that would be active and able to login.
    • Inactive - is manually set by an administrator. There is a customizable warning message handled by (account_inactive_message in config/default.json) for user that attempt to login and are flagged to this account_status.
    • LockedTraining - is to support organization processes that allow for an account to be created in the background - but require a new user to take some sort of specific trainings to be allowed access. There is a customizable warning message handled by (account_locked_missing_training_message in config/default.json) for user that attempt to login and are flagged to this account_status.
    • Locked - is automatically tied to the optional account_lockout_after_date field. There is a customizable warning message handled by (account_locked_message in config/default.json) for user that attempt to login and are flagged to this account_status.
  • account_lockout_after_date (optional field) - if populated will mark the User account Locked after the specific date. Users attempting to login after this date will receive the configured account_locked_message
  • NOTE: Changing to the account_status value on an account will result in termination of any "active" sessions. The point being, should you as an Admin be told to Lock or mark an account Inactive, you would not want - if that user were currently logged in - a session to persist for them after that flag was set. So, sessions are terminated for a user upon account_status change.

MIGRATION NOTE:

Upon startup of OpenCTI after these changes have been adopted:

  • A function (initUserAccountStatus) has been added to opencti-platform/opencti-graphql/src/config/providers.js. This function will "repair" / "seed" any User accounts that are missing a valid ENUM status for account_status to Active (default for default_initialize_account_status from conf/default.json)
    • Running of this feature can be disabled by setting initialize_account_status in conf/default.json to false for future startups - but needs to be true on initial migration or all existing accounts previous to this capability will have a NULL account_status resulting in lockout.
  • The function (initUserAccountStatus) is called by opencti-platform/opencti-graphql/src/initialization.js at system startup
    • The internal Administrator account will always be set at startup to Active

Related issues

  • Automation support for the LockedTraining status is planned for a possible additional future PR.

Checklist

  • [ X ] I consider the submitted work as finished
  • [ X ] I tested the code for its functionality
  • [ X ] I wrote test cases for the relevant uses case
  • Manually tested. There are Selenium automated tests that can be submitted as part of a Selenium GUI auto test future PR.
  • I added/update the relevant documentation (either on github or on notion)
  • [ X ] Where necessary I refactored code to improve the overall quality

Further comments

None.

@Bonsai8863 Bonsai8863 force-pushed the account_lockout branch 3 times, most recently from 1d0de7a to d1a92af Compare May 9, 2023 16:50
@ParamConstructor ParamConstructor marked this pull request as ready for review May 9, 2023 17:06
@Kedae Kedae added the community use to identify PR from community label May 11, 2023
@SamuelHassine SamuelHassine force-pushed the master branch 2 times, most recently from ff9a07d to c280f3b Compare May 20, 2023 08:51
@ParamConstructor ParamConstructor force-pushed the account_lockout branch 2 times, most recently from fa51528 to 78beb95 Compare May 31, 2023 15:52
@ParamConstructor ParamConstructor force-pushed the account_lockout branch 6 times, most recently from 1131e36 to 00b2c49 Compare June 1, 2023 19:17
@SamuelHassine SamuelHassine force-pushed the master branch 3 times, most recently from e12789a to cf6edc3 Compare June 8, 2023 09:18
@Bonsai8863 Bonsai8863 force-pushed the account_lockout branch 2 times, most recently from c1fedb7 to 5093a3b Compare June 20, 2023 13:18
@ParamConstructor ParamConstructor force-pushed the account_lockout branch 4 times, most recently from dc8cbc1 to 696705d Compare June 26, 2023 19:01
@Archidoit
Copy link
Member

Archidoit commented Aug 16, 2023

Can you also change the migration number (so that the migration file is the latest to run after merging on the main branch)?

@Archidoit
Copy link
Member

What about displaying the account status in a special box (design)?
image

You can use the ItemStatus component and get inspiration from the capture below for instance
image

@Archidoit
Copy link
Member

Behavior tested and works well :)

@ParamConstructor
Copy link
Contributor Author

ParamConstructor commented Aug 16, 2023

@Archidoit - Something like this maybe? (just hardcoded onto page to get a screen grab of all options - obviously only one would be displayed)
image

--> @ParamConstructor yes that's perfect.

@ParamConstructor
Copy link
Contributor Author

@Archidoit - Status Display change is pushed - it is building now. Thank you for the feedback and peer review.

containerstyle={fieldSpacingContainerStyle}
>
{settings.platform_user_statuses.map((s) => {
return <MenuItem key={s.status} value={s.status}>{s.status}</MenuItem>;
Copy link
Member

Choose a reason for hiding this comment

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

What about translating account statuses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Archidoit - give me a little bit - I will push a commit with the translations fixed - sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Archidoit - Corrected.

}
>
{settings.platform_user_statuses.map((s) => {
return <MenuItem key={s.status} value={s.status}>{s.status}</MenuItem>;
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Archidoit - Corrected.

@Archidoit
Copy link
Member

Thanks for all the changes! I'm going to do a final review. Can you just resolve the conflicts you have with the main branch before? Thanks

@ParamConstructor ParamConstructor force-pushed the account_lockout branch 2 times, most recently from e9b8388 to e2b94fe Compare August 21, 2023 12:40
@ParamConstructor
Copy link
Contributor Author

@Archidoit - Conflicts resolved, rebased, committed/pushed, and building now in the drone pipeline.

@Archidoit
Copy link
Member

Archidoit commented Aug 21, 2023

Tested and works well 👍
Waiting for some minors modifications of @richard-julien before merging

@richard-julien richard-julien merged commit bf89e1d into OpenCTI-Platform:master Aug 25, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community use to identify PR from community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants