Ability to ban accounts

[James Teh]

There are times when it would be useful to have the ability to ban an account. Examples are spammer accounts and troublesome users. An alternative would be to have separate functionality to blacklist email addresses, though this seems somewhat more complicated.

[Steffen Hoffmann]

Well second first: Not that I would reject a dedicated ban functionality, but with (administratively) adding a dummy user you could effectively disable this for new accounts, if you enable registration support in a recent AcctMgr version. I've been testing this a lot, so this should work. If not, tell me your issues.

I've already thought about that recently. It would be achievable without adding more logic by setting an account lock somewhere far in the future. But OTOH it'll lock little too hackisch to be nice, and imaging a login failure along the lines of "...try again in 50 years..." that would certainly surface in that setup. So this is not the best solution.

What we should do is adding dedicated administrative account locking, right?

As a side-note I think there is not much difference from deleting the account(s) AFAIK. It would be much more useful, if we were able to do kind of a fake-login for such accounts and just deny any action. This should be much more effective against automated approaches, because it'll waste more of their resources (admittedly at the Trac side too). I wonder if this could still be intergrated via an own permission provider inside AcctMgr. Do you agree or it there something totally missing/wrong at my side?

[James Teh]

Replying to hasienda:

Well second first: Not that I would reject a dedicated ban functionality, but with (administratively) adding a dummy user you could effectively disable this for new accounts

Right. What I've been doing is changing the password on any accounts I want to ban. However, I think your approach is better, as if the user stays logged in, they'll still be authenticated once the password is changed. Another problem with my approach is that I can't tell which accounts are banned accounts. I guess with your approach, I could set the account name to indicate this.

I've already thought about that recently. It would be achievable without adding more logic by setting an account lock somewhere far in the future. But OTOH it'll lock little too hackisch to be nice

Agreed.

What we should do is adding dedicated administrative account locking, right?

Agreed.

As a side-note I think there is not much difference from deleting the account(s) AFAIK. It would be much more useful, if we were able to do kind of a fake-login for such accounts and just deny any action.

If I understand what you're suggesting, treating the accounts in the same way unverified accounts are treated would work nicely.

[Steffen Hoffmann]

Replying to jteh:

Replying to hasienda:

Well second first: Not that I would reject a dedicated ban functionality, but with (administratively) adding a dummy user you could effectively disable this for new accounts

Right. What I've been doing is changing the password on any accounts I want to ban. However, I think your approach is better, as if the user stays logged in, they'll still be authenticated once the password is changed. Another problem with my approach is that I can't tell which accounts are banned accounts. I guess with your approach, I could set the account name to indicate this.

The first consideration was just regarding blocking emails. Sorry, if I wasn't clear enough. So it wont' help for your "already logged in" scenario. Only my last suggestion would do, since we could revoke an authentication via that custom !IPermissionPolicy more or less silently at any time.

![...]

As a side-note I think there is not much difference from deleting the account(s) AFAIK. It would be much more useful, if we were able to do kind of a fake-login for such accounts and just deny any action.

If I understand what you're suggesting, treating the accounts in the same way unverified accounts are treated would work nicely.

Exactly. And the longer I think about it, the more I like the idea.

[Steffen Hoffmann]

Release of acct_mgr-0.4 helped to cut down a rather long list of issues.

Hereby I acknowledge, that - while working at #843 - now this feature is finally getting serious attention.

I've got a preliminary implementation for registration approval, some working unit tests and positive test results from my development system. So this is definitely on its way now.

[Steffen Hoffmann]

(In [12504]) AccountManagerPlugin: Use new account approval feature to ban accounts, refs #843 and #8595.

While simple, this might be an effective counter-measure against spammer registrations, because intentionally it's rather hard to spot the difference between an approved authenticated session and one with pending approval. So I forsee more wasted resources on attacker's side to figure out, why one still fails to spam the site even after successful registration, and that is certainly a good thing.

Note: Due to its implementation banning is instantly effective and will prevent even authenticated sessions from doing more than a user could in an anonymous session, unlike the account lock provided by AccountGuard. That wouldn't prevent authenticated sessions from continuing indefinitely, at least until next authentication time, and this might be a rather long time, if persistant sessions had been allowed before.

[Steffen Hoffmann]

(In [12505]) AccountManagerPlugin: Some enhancements regarding approval/ban feature, refs #843, #8595 and #10741.

Adding the registration approval configuration option to config admin panel. Taking care for marking all potential message parts for translation as well. A recent suggestion by Ryan J Ollos is merged in here too, so that all kinds of restricted accounts are clearly visible in user listings now.

[Steffen Hoffmann]

The current tests in the user admin template are not totally sufficient yet.

I.e. they won't detect 'ACCTMGR_USER_ADMIN' privileges for any account and wrongly mark them with 'Approval revoked', while this is impossible. These accounts are excluded intentionally from permission cut-down. I'll take care for this detail when establishing a better test abstraction for all kinds of account limiting.

[Steffen Hoffmann]

#10002 has been closed as a duplicate of this ticket.

Replying to rjollos:

Yes, I agree it is a duplicate now that the Toggle account approval feature is in place. The only reservation I had was that it might not be completely obvious to someone that Toggle account approval is the way to lock an account, but I think we can cover that in the docs.

I coined 'Toggle' for shortness to describe the universal function that this button really has: Once press will approve accounts with state 'pending' set on registration time (once) and switch on/off account banning for the rest of selected accounts.

How about adding a more descriptive and verbose tool-tip then? Suggestion: "Approve pending registrations, ban/unban accounts"

[anonymous]

Replying to hasienda:

How about adding a more descriptive and verbose tool-tip then? Suggestion: "Approve pending registrations, ban/unban accounts"

Yeah, I think that would be a useful addition.

[Steffen Hoffmann]

(In [12571]) AccountManagerPlugin: Add descriptive tool-tip to 'Toggle account approval', refs #7426 and #8595.

A CSS style comment is now corrected as well.

Thanks to Ryan J Ollos for hints on these minor issues, nevertheless I care and I'm happy to receive code reviews for getting things straight.

[Jun Omae]

I think current approach of approval feature has two issues.

1. If [ticket] commit_ticket_update_check_perms = enabled and ticket is updated via commit_updater, it doesn't work for users which the session has 'approval'. Because AccountManager.pre_process_request is applied when only web requests.
2. Order of applying pre_process_requests depends on the order of loaded plugins. Also, components in Trac core will be first applied. I think we could avoid it to prepend AccountManager to [trac] request_filters.

Another approach is using IPermissionPolicy (untested and AccountManagerApprovalPolicy is needed to prepend to [trac] permission_policies).

class AccountManagerApprovalPolicy(Component):

    implements(IPermissionPolicy)

    def check_permission(self, action, username, resource, perm):
        if username != 'anonymous' and 'ACCTMGR_USER_ADMIN' not in perm:
            approval = self.env.db_query("""
                    SELECT value FROM session_attribute
                    WHERE sid=%s AND authenticated=1 AND name='approval'
                """, (username,))
            if approval:
                anon_perm = PermissionCache(self.env, 'anonymous')
                return action in anon_perm(resource)

[Ryan J Ollos]

Implementing IPermissionPolicy seems like a good approach. We had a similar issue with a permission check based on the readonly property of wiki pages not being checked in attachment web requests, and for that case psuter's idea of implementing IPermissionPolicy (ReadonlyWikiPolicy) was an elegant solution.