Incorrectly incorrect `required_groups` behavior

We've used the auto-unblock feature of required_groups (ie. if you're in the group, your account should be unblocked on log in, solved by !3223 (merged)), which has been rendered unusable by !9489 (merged).

My original intent was not to change default behavior (where a blocked user keeps being a blocked user), but to make sure we can save license seats by either restricting users from a huge SSO pool, and by disabling users who might not even come back after they silently stop using the application.

Apparently, it looks like there was a special case which has not been handled properly by the original patch:

          def user_in_required_group?
            required_groups = saml_config.required_groups
            required_groups.empty? || !(auth_hash.groups & required_groups).empty?
          end

This piece of code has an assumption users should be handled the same way if they are in the required_groups, or when required_groups is empty. Sorry, my bad.

Instead, the solution made another assumption:

            if user_in_required_group?
              unblock_user(user, "in required group") if user.persisted? && user.ldap_blocked? # replacing `user.blocked?`
            elsif user.persisted?
              block_user(user, "not in required group") unless user.blocked?
            else
              user = nil
            end

My problem of the current situation is we can't just say to our users, who

  • never told us they don't want to use the service
  • are gone for more than two months
  • and they get angry if their accounts are not usable afterwards

to just log in again and all's well.

We can't, because now we would have to use ldap_blocked state, which is for a different situation: it is for LDAP to tell us, the user couldn't log in.

Likewise, what would be the purpose of saml_blocked?

SAML will never let the app know auth was unsuccessful. There is no practical sense for having a saml_blocked state, or any other than blocked. It is never managed by SAML, but by the admin.

Assignee Loading
Time tracking Loading