Skip to content

Added code to skip setting password confirmation requirement for non-ldap user#435

Open
AltamashShaikh wants to merge 1 commit into5.x-devfrom
update-password-confirmation-check
Open

Added code to skip setting password confirmation requirement for non-ldap user#435
AltamashShaikh wants to merge 1 commit into5.x-devfrom
update-password-confirmation-check

Conversation

@AltamashShaikh
Copy link
Copy Markdown
Contributor

Description

Added code to skip setting password confirmation requirement for non-ldap user

Issue No

Steps to Replicate the Issue

Checklist

  • [✔] Tested locally or on demo2/demo3?
  • [✖] New test case added/updated?
  • [NA] Are all newly added texts included via translation?
  • [NA] Are text sanitized properly? (Eg use of v-text v/s v-html for vue)
  • [✔] Version bumped?
  • [NA] I have understood, reviewed, and tested all AI outputs before use
  • [NA] All AI instructions respect security, IP, and privacy rules
  • [NA] Documentation updated?

Copy link
Copy Markdown
Contributor

@lachiebol lachiebol left a comment

Choose a reason for hiding this comment

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

From codex, I had a bit of a look. Does this ldap user check need to be in those other methods?

• 1. Blocking: the branch only narrows the pre-check, not the actual bypass. The new observer in config/
     config.php:19 makes Piwik::doesUserRequirePasswordConfirmation() return true for non-LDAP users,
     which is the right direction for UI/API gating (/home/lachlan/matomo-code/matomo_5x-dev/core/
     Piwik.php:296, /home/lachlan/matomo-code/matomo_5x-dev/core/Plugin/API.php:127). But LoginLdap still
     auto-verifies the password for every user when enable_password_confirmation=0 in both
     LoginLdap.php:331 and Controller.php:80, with no LDAP-user check. So a non-LDAP user who reaches the
     confirmation flow can still bypass re-authentication entirely. For a security-sensitive path, that
     means the branch only partially fixes the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants