Modify

Opened 4 years ago

Last modified 7 months ago

#8595 new enhancement

Ability to ban accounts

Reported by: jteh Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: user account lock permanent
Cc: Trac Release: 0.12

Description

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.

Attachments (0)

Change History (12)

comment:1 follow-up: Changed 4 years ago by hasienda

  • Keywords user account lock permanent added

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?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by 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.

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.

comment:3 in reply to: ↑ 2 Changed 4 years ago by hasienda

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.

comment:4 Changed 2 years ago by hasienda

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.

comment:5 Changed 2 years ago by hasienda

(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.

comment:6 Changed 2 years ago by hasienda

(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.

comment:7 Changed 2 years ago by hasienda

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.

comment:8 follow-up: Changed 2 years ago by hasienda

#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"

comment:9 in reply to: ↑ 8 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by hasienda

(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.

comment:11 Changed 7 months ago by jun66j5

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)

comment:12 Changed 7 months ago by rjollos

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.

Add Comment

Modify Ticket

Action
as new The owner will remain hasienda.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.