Skip to content
Snippets Groups Projects

feat: entropy of user password is now checked by attributes passed from authsource

Merged Pavel Břoušek requested to merge sfa into main

Created by: xpavlic

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Pavel Břoušek
  • Pavel Břoušek
  • Pavel Břoušek
    Pavel Břoušek @433364 started a thread on commit ed05d30e
  • 45 46 return AuthSwitcher::DEFAULT_REQUESTED_CONTEXTS;
    46 47 }
    47 48 $supportedRequestedContexts = array_values(array_intersect($requestedContexts, AuthSwitcher::SUPPORTED));
    49 if (!$sfaEntropy) {
    • Author Contributor

      Created by: melanger

      If you want to do the check here, you have to handle it for the first case as well (if (empty($requestedContexts))).

      You can also move the check after the call to this function.

  • Pavel Břoušek
    Pavel Břoušek @433364 started a thread on commit ed05d30e
  • 247 277 }
    248 278 }
    249 279 }
    250 $result[] = AuthSwitcher::SFA;
    280 if (!$this->check_entropy || $this->checkSfaEntropy($state)) {
    281 $result[] = AuthSwitcher::SFA;
    282 }
    • Author Contributor

      Created by: melanger

      I don't think this is right - AuthSwitcher::SFA here does not mean the REFEDS SFA profile, it just means the user can log in via password only. If you kept this check, then users with passwords that don't meet the REFEDS SFA profile would not be able to log in, this method would return an empty array.

      I think it would be better to move this check and keep the original version of this method.

  • Pavel Břoušek
    Pavel Břoušek @433364 started a thread on commit 49db26c9
  • 174 184 }
    175 185 }
    176 186
    187 private function checkSfaEntropy($attributes)
    188 {
    189 if (!$this->sfa_len_attr || !$this->sfa_alphabet_attr || !in_array(
    190 $this->sfa_alphabet_attr,
    191 $attributes,
    192 true
    193 ) || !in_array($this->sfa_len_attr, $attributes, true)) {
    194 return false;
    • Author Contributor

      Created by: melanger

      This would mean a breaking change, because the original behavior is that anything satisfies SFA.

      Please change the code - if sfa_len_attr or sfa_alphabet_attr is not set, log a message with level INFO, that authswitcher could not check REFEDS SFA, so it assumes it was fulfiled. When the attribute config options are set, do the new behavior and return false here.

      Also update README - mention that if those 2 new options are not set, it is assumed that all passwords satisfy REFEDS SFA, and explain how to use them.

    • Author Contributor

      Created by: xpavlic

      For this case I added $check_entropy config option with false as default value. But this solution is probably better.

  • Author Contributor

    Merged by: melanger at 2022-06-02 12:54:16 UTC

  • Author Contributor

    Created by: github-actions[bot]

    :tada: This PR is included in version 10.5.0 :tada:

    The release is available on GitHub release

    Your semantic-release bot :package::rocket:

  • Please register or sign in to reply
    Loading