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

If AllowAccountReset is set to false, the user can not change the password even if it expires so he can not log in anymore #6261

Open
faustogut opened this issue Jul 17, 2024 · 17 comments
Assignees
Labels

Comments

@faustogut
Copy link
Contributor

Describe the bug
When domain.passwordrequirements.reset is set to X and domain.passwordrequirements.allowaccountreset is set to 'false' then X days after user creation the user cannot log in anymore (because once he enters his credentials he will be prompted to change his password but later the new password he entered does not work)

NOTE: According to meshcentral-config-schema.json
domain.passwordrequirements.reset = Number of days after which the user is required to change the account password.
domain.passwordrequirements.allowaccountreset = If set to false, the account reset option on the login screen will not be available to users.

To Reproduce
Steps to reproduce the behaviour:

  1. In config.json set domain.passwordrequirements.reset = 1
  2. In config.json set domain.passwordrequirements.allowaccountreset = false
  3. Restart the program to apply config changes
  4. Create a user
  5. Wait for the password to expire (pe. wait for 2 days)
  6. Try to log in with that user and you can see the problem (you enter your current credentials, you enter your new password, and finally you try to log in with new credentials and it does not work)

Expected behaviour
With this config, a user can not change his password unless the password expires.
When the password expires, you'll be prompted to enter a new one. Then the new password works.

Workaround
Setting domain.passwordrequirements.allowaccountreset = true avoids the problem.
Maybe there is no need to fix this scenario, but improve documentation (meshcentral-config-schema.json descriptions) this way (or similar):

domain.passwordrequirements.reset = Number of days after which the user is required to change the account password. 0 means the password never expires. NOTE: If you set this to a non-zero value, please be sure to set domain.passwordrequirements.allowaccountreset to true.

domain.passwordrequirements.allowaccountreset = If set to false, the account reset option on the login screen will not be available to users. NOTE: Set to true if you set domain.passwordrequirements.reset to a non-zero value.

@faustogut faustogut added the bug label Jul 17, 2024
@si458
Copy link
Collaborator

si458 commented Jul 17, 2024

you didnt quite follow the bug template report
can you share your config.json?
what meshcentral version are you using?
what nodejs version?
what server os?
what web browser?
do you use external auth at all like ldap/saml/azure?

@si458
Copy link
Collaborator

si458 commented Jul 17, 2024

also can you run node node_modules/meshcentral --debug web and share the output after 1 day?
it will show you a few debug lines for the web and also what function is causing it to fail changing the password
(if thats the case)

@faustogut
Copy link
Contributor Author

faustogut commented Jul 17, 2024

Server Software

  • Virtualization: Docker version 24.0.7, build afdd53b on Ubuntu 22.04.3 LTS as the host (using portainer)
  • OS: The one the docker image use: Linux 0276ef3915c2 5.15.0-94-generic WSMAN Parsing Error AMT v9.1.41 #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 Linux
  • Network: traefik reverse proxy as a Docker image: traefik:v2.11.0 (running in the same machine)
  • Version: Docker image: ghcr.io/ylianst/meshcentral:1.1.24
  • Node: The one the docker image use: v20.12.1
  • npm: The one the docker image use: 10.8.0

Client Device

  • Device: Desktop
  • OS: Windows 11
  • Network: Internet
  • Browser: Google Chrome v126.0.6478.114 (64 bits)
  • MeshCentralRouter Version: not applicable

Do you use external auth at all like ldap/saml/azure?
No

Your config.json file

{
  "$schema": "https://raw.githubusercontent.com/Ylianst/MeshCentral/master/meshcentral-config-schema.json",
  "settings": {
    "plugins": {
      "enabled": false
    },
    "mongoDb": "mongodb://mongodbadmin:thisisnotmyrealmongodbpasswd@mongodb:27017",
    "mongoDbName": "meshcentral",
    "mongoDbChangeStream": false,
    "mongoDbBulkOperations": true,
    "cert": "thisisnotmyrealdomainname",
    "sessionKey": "thisisnotthesessionkey",
    "port": 4430,
    "aliasPort": 443,
    "redirPort": 800,
    "redirAliasPort": 80,
    "AgentPong": 300,
    "tlsOffload": "traefik-reverse-proxy",
    "SelfUpdate": false,
    "AllowFraming": false,
    "WebRTC": true,
    "agentPong": 55,
    "autoBackup": {
      "backupIntervalHours": 60,
      "keepLastDaysBackup": 1
    }
  },
  "domains": {
    "": {
      "minify": true,
      "NewAccounts": false,
      "localSessionRecording": false,
      "certUrl": "thisisnotmyrealserveripaddress:443",
      "passwordRequirements": {
        "min": 14,
        "max": 128,
        "upper": 1,
        "lower": 1,
        "numeric": 1,
        "nonalpha": 1,
        "reset": 365,
        "email2factor": true,
        "sms2factor": false,
        "push2factor": false,
        "otp2factor": true,
        "msg2factor": false,
        "backupcode2factor": true,
        "single2factorWarning": true,
        "lock2factor": false,
        "force2factor": true,
        "skip2factor": "thisisnotmyrealipaddress",
        "oldPasswordBan": 5,
        "banCommonPasswords": true,
        "loginTokens": false,
        "twoFactorTimeout": 300,
        "autofido2fa": false,
        "maxfidokeys": null,
        "allowaccountreset": false
      },
      "twoFactorCookieDurantionDays": 0
    }
  },
  "smtp": {
    "host": "thisisnotmyrealsmtpserverurl",
    "port": 465,
    "from": "thisisnotmyrealemailaccount@ispprovider.com",
    "user": "thisisnotmyrealemailaccount@ispprovider.com",
    "pass": "thisisnotmyrealemailaccountpassword",
    "tls": true
  }
}

@si458
Copy link
Collaborator

si458 commented Jul 17, 2024

Thank u :)

I will have a look when I get chance for you!

I have a feeling it's calling the password reset function, and that function has a checker for allowaccountreset==false,

so it might be returning false and not actually changing the password.

Can u try logging in, resetting pass, login with new pass, then login with old pass?

Does it still let u login with old pass after u change it to new pass Or does it just go back to the u need to change password screen?

@PetieM
Copy link

PetieM commented Sep 30, 2024

I can confirm this behavior as well. Will provide my config tomorrow if it’s needed though it hasn’t changed much since the last time I posted it. When allowAccountReset is set to false, all password reset functionality fails. My most recent test involved inviting a new user, setting their password, and having it prompt for reset on first login. The prompt came up successfully, they were able to type in a new password and submit it, but the password was not saved and they were able to log in again with the old, temporary password. This resulted in a permanent loop until I reset the password from my side, did not force reset, and then walked them through changing it manually after.

@si458 si458 self-assigned this Sep 30, 2024
@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

ok all so i can confirm the issue
bug is here in this code
const allowAccountReset = ((typeof domain.passwordrequirements != 'object') || (domain.passwordrequirements.allowaccountreset !== false));
i need to make allowaccountreset say true so im just trying to work/figure it out

si458 added a commit that referenced this issue Sep 30, 2024
@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

should be fixed! very simple fix here if you wanna patch and try yourselves! 8e5aa35

@PetieM
Copy link

PetieM commented Sep 30, 2024

That patch does look easy but I have a couple of quick questions.

  1. This will allow resetting of expired passwords without bringing back the forgot password functionality, correct? I was actually the one who originally requested an option to disable account recovery from the login screen and want to ensure that stays disabled while giving users the ability to reset a password that has expired (e.g. during a new account setup).
  2. I'm happy to make this change directly and test but it will be the first time I've done so and I want to make sure that it's not going to mess up future updates first. Will it see that I have modified this file and skip it in the future or will any changes I make directly be overwritten during an update?

Thanks!

@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

@PetieM from my testing, no it shouldnt break anything!
if i set allowaccountreset: false and leave reset out, it doesnt show the reset password option
if i set allowaccountreset: true and leave reset out, it shows the reset password option and that works,
if i set allowaccountreset: false and set reset: 1, the reset password is hidden and when the user logs in, they can reset there password because in the backend it sets allowaccountreset as true so the user can proceed with changing there password!

in theory it wont break any updates in the future if you patch it manually!
if your worried, backup whole meshcentral folders (including node-modules), then if goofs up, roll back
or just copy the single file to say file.js.backup then edit the original and if it stops working, move the backup file back!

@PetieM
Copy link

PetieM commented Sep 30, 2024

Sounds good. One more thing though - what/where is this reset variable you're referring to? Is that a flag on the user?

@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

sorry its the number of days value from the schema file
image

@PetieM
Copy link

PetieM commented Sep 30, 2024

Oh! Got it. I forgot that was a setting as I'm not mandating forced resets after a certain time, only during account creation or if I reset a password for someone. This is how that block looks for me currently:
image

@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

oh ffs! the commit doesnt work with allowaccountreset: false and not setting reset!
looking into it now!

@PetieM
Copy link

PetieM commented Sep 30, 2024

lol, apologies for throwing a wrench into that otherwise elegant 1-line solution! If setting it to 0 instead of not setting it at all accomplishes the same thing, I'm okay with change that instead.

si458 added a commit that referenced this issue Sep 30, 2024
@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

ok ive had to revert that commit 👎
this is very complicated because of the different methods of calculating if you can allowedacountreset or not
i created a simply function to test but its very difficult because basically we always want to allow the passwordchange

function determineAllowAccountReset(domain) {
  if (typeof domain.passwordrequirements !== 'object' || domain.passwordrequirements === null) { return true; } // no passwordrequirements, return true
  if (typeof domain.passwordrequirements.allowaccountreset === 'boolean' && typeof domain.passwordrequirements.reset === 'undefined') {
    return domain.passwordrequirements.allowaccountreset; // passwordrequirements.allowaccountreset is true or false but passwordrequirements.reset not set, so return allowaccountreset value
  } else if (typeof domain.passwordrequirements.allowaccountreset === 'boolean' && typeof domain.passwordrequirements.reset === 'number') {
    return true; // passwordrequirements.allowaccountreset is true or false BUT passwordrequirements.reset is set so always return true
  }
  return true;
}
const domain1 = {
  passwordrequirements: {
    allowaccountreset: true
  }
};
const domain2 = {
  passwordrequirements: {
    allowaccountreset: true,
    reset: 5
  }
};
const domain3 = {
  passwordrequirements: {
    allowaccountreset: false,
    reset: 3
  }
};
const domain4 = {
  passwordrequirements: {
    allowaccountreset: false
  }
};
const domain5 = {};
console.log(determineAllowAccountReset(domain1)); // true
console.log(determineAllowAccountReset(domain2)); // true
console.log(determineAllowAccountReset(domain3)); // true
console.log(determineAllowAccountReset(domain4)); // false
console.log(determineAllowAccountReset(domain5)); // true

@PetieM
Copy link

PetieM commented Sep 30, 2024

Maybe it would make more sense to entirely decouple the mandatory password reset for users who are already logged in (or in the process of successfully logging in) from the account recovery feature for users who have forgotten their password entirely?

@si458
Copy link
Collaborator

si458 commented Sep 30, 2024

@PetieM good idea! will do more testing!
@Ylianst in process of doing another release due to things/translation issues,
so it wont be included in this release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants