Ticket #25 (closed enhancement: fixed)

Opened 8 years ago

Last modified 5 years ago

User administration module

Reported by: dartar Owned by: DarTar
Priority: normal Milestone: 1.1.6.4
Component: administration Version: 1.1.6.0
Severity: normal Keywords: antispam,trunk-ported
Cc: BrianKoontz

Description (last modified by BrianKoontz) (diff)

A module to manage Wikka users.

Status

beta (code cleanup needed)

Refs

Related tickets

  • #26 Page administration module
  • #442 Removing spam/test users
  • #608 Enhanced User Administration module (single/mass user feedback)
  • #647 Update codebase to reflect new user status fields

Change History

  Changed 7 years ago by DotMG

  • owner changed from unassigned to DotMG
  • status changed from new to assigned

  Changed 7 years ago by DarTar

  • description modified (diff)
  • summary changed from User Administration to User administration module

  Changed 7 years ago by DarTar

  • keywords antispam added
  • owner changed from DotMG to DarTar
  • status changed from assigned to new
  • milestone changed from 1.1.8 to 1.1.6.4

Mahefa, I'm reassigning this to myself if you don't mind.

  Changed 7 years ago by DarTar

(In [772]) First draft of adminusers action. The code generates a simple GUI that can be used later to plug in different sorts of handlers and user-related functionality. Icons and general CSS selectors for alternate coloring of tables are included, refs #25 and #562

  Changed 6 years ago by DarTar

(In [810]) Minor stylistic change to adminusers, refs #25

  Changed 6 years ago by DarTar

(In [811]) Prefixing constants in adminusers to avoid name collision, adding CSS class for sort order information (to be used by any module that generates sortable lists or tables), refs #25

  Changed 6 years ago by DarTar

(In [813]) Admin modules:

  • Adding todos in phpdoc header;
  • Adding missing icons;
  • Hiding features yet to be implemented;

refs #25 and #26

  Changed 6 years ago by DarTar

(In [814]) Minor phpdoc change, refs #25

  Changed 6 years ago by DarTar

(In [815]) Styling: line-height for filter fieldsets in admin modules, refs #25 and #26

follow-up: ↓ 11   Changed 6 years ago by BrianKoontz

(In [830]) Mass user deletions, including password lockout and session tracking to "kick" banned users who might still be accessing the site via an existing session. Currently, there is no functionality to keep session table pruned of sessions that aren't deleted through a user logout (refs #25).

in reply to: ↑ 10 ; follow-up: ↓ 13   Changed 6 years ago by BrianKoontz

Replying to BrianKoontz:

(In [830]) Mass user deletions, including password lockout and session tracking to "kick" banned users who might still be accessing the site via an existing session. Currently, there is no functionality to keep session table pruned of sessions that aren't deleted through a user logout (refs #25).

Forgot to mention: This change requires the following DB change:

create table wikka_sessions (sessionid char(16) NOT NULL, userid varchar(75) NOT NULL, PRIMARY KEY (sessionid, userid));

Also, note to self: Single-user delete might not be working correctly.

  Changed 6 years ago by BrianKoontz

(In [832]) Implemented cleanup routine for session records in sessions table (keyed off the persistent cookie lifetime) to keep the table pruned of old sessions. Minor code cleanup. (refs #25)

in reply to: ↑ 11   Changed 6 years ago by raffa

Replying to BrianKoontz:

Forgot to mention: This change requires the following DB change: create table wikka_sessions (sessionid char(16) NOT NULL, userid varchar(75) NOT NULL, PRIMARY KEY (sessionid, userid));

This should be added to the install script then.

cheers,

raffa

follow-up: ↓ 15   Changed 6 years ago by raffa

btw, the create string is not correct, since:

Query failed: INSERT INTO wikka_sessions (sessionid, userid, session_start) VALUES('some_session...', 'admin', FROM_UNIXTIME(1196582038.32)) (Unknown column 'session_start' in 'field list')

in reply to: ↑ 14 ; follow-up: ↓ 16   Changed 6 years ago by BrianKoontz

Replying to raffa:

btw, the create string is not correct, since: Query failed: INSERT INTO wikka_sessions (sessionid, userid, session_start) VALUES('some_session...', 'admin', FROM_UNIXTIME(1196582038.32)) (Unknown column 'session_start' in 'field list')

You'll need to do an update, raffa...this has been fixed.

in reply to: ↑ 15   Changed 6 years ago by BrianKoontz

Replying to BrianKoontz:

Replying to raffa:

btw, the create string is not correct, since: Query failed: INSERT INTO wikka_sessions (sessionid, userid, session_start) VALUES('some_session...', 'admin', FROM_UNIXTIME(1196582038.32)) (Unknown column 'session_start' in 'field list')

You'll need to do an update, raffa...this has been fixed.

Spoke too quickly...you'll need to add the session_start field:

alter table wikka_sessions add session_start datetime NOT NULL;

Will add this to the install script...

  Changed 6 years ago by BrianKoontz

(In [833]) Forgot to create sessions table for new installs, fixed. Also, if one is upgrading between 1.1.6.4 revisions, DB updates will need to be done manually (refs #25).

  Changed 6 years ago by raffa

the javascript checks in the install file bug me:

  • CamelCase in username *should* be used. JS forces me to use it
  • Email addresses like name@… are not recognized to be valid, there could also be other combinations?

By the way: wikka is still giving away too much information, which makes attacking vulnerable installations easier. See ticket #560 & #597.

  Changed 6 years ago by DarTar

(In [834]) Adding default pages (AdminUsers and AdminPages) and removing redundant update of FormattingRules, refs #25 and #26 and #562

  Changed 6 years ago by BrianKoontz

  • description modified (diff)

Migrated over from dupe ticket #442:

(In [823]) User status is a feature introduced in 1.1.6.4 so the upgrader should add it for any version <1.1.6.4, refs #442

(In [837]) Delete links disabled for admins; error message displayed when attempting to mass-delete admin users; delete link fixed (refs #442).

  Changed 6 years ago by DarTar

(In [846]) Stripping single/mass user feedback from the AdminUser module included in 1.1.6.4 since they depend on the enhanced feedback module that will be released with 1.1.7, refs #25, #486, #608

  Changed 6 years ago by DarTar

  • description modified (diff)

  Changed 6 years ago by DarTar

  • description modified (diff)

  Changed 6 years ago by DarTar

(In [854]) Installation of admin modules in 1.1.6.4: - creating new CategoryAdmin for default pages for admin modules; - creating new DatabaseInfo page as a default page for {{dbinfo}}; - setting ACL for AdminUsers, AdminPages and DatabaseInfo to admin-only; refs #25, #26 and #27

  Changed 6 years ago by BrianKoontz

  • status changed from new to closed
  • resolution set to fixed

Tests successful using revision 863.

follow-up: ↓ 30   Changed 6 years ago by DarTar

  • status changed from closed to reopened
  • resolution fixed deleted

Brian, a few random question on adminusers (not sure if we need to address them all before the 1.1.6.4 release):

  • could user deletion display a confirmation screen? (the same that appears when trying to mass-delete)
  • could we add a filter option to display deleted users? (in which case I think an "undelete" option could be useful)
  • do other actions that process users data take into account the new status field? If they don't have we considered the issues that could possibly arise from this?

* if we give admins the possibility to completely restrict access, it would be necessary to have a form to be able to add new users from the admin panel, otherwise this will only be possible by directly feeding the DB.

Let me know what you think of these issues so we can plan some further coding if needed before definitely closing the ticket for the release.

follow-up: ↓ 29   Changed 6 years ago by DarTar

  • cc BrianKoontz added

Also, is there a reason why the pw value for deleted users is replaced with a "!"? Why not just leave it there (assuming that the status field is sufficient to determine whether a user can still try to login or not).

This makes me think, incidentally, of the consequences of the status field on functionality such as {{emailpassword}}.

  Changed 6 years ago by DarTar

I just realised that I haven't committed yet the userpages and userchanges actions required by the user admin module (!). I'll take care of this.

in reply to: ↑ 27 ; follow-up: ↓ 31   Changed 6 years ago by BrianKoontz

Replying to DarTar:

Also, is there a reason why the pw value for deleted users is replaced with a "!"?

Yes, because there is no possible MD5 hash that will match that value. It's the same principle used in Unix to lock a user account.

Why not just leave it there (assuming that the status field is sufficient to determine whether a user can still try to login or not).

Consider it an additional safeguard. If we've gone so far as to delete a user, there's no need for a password. And if said user has access restored by mistake, they still can't access until positive measures are taken to restore the password.

This makes me think, incidentally, of the consequences of the status field on functionality such as {{emailpassword}}.

Yes, there are probably several places in the Wikka codebase that needs to check the status of a user when an action would not depend on the user actually being logged in.

in reply to: ↑ 26   Changed 6 years ago by BrianKoontz

Replying to DarTar:

Brian, a few random question on adminusers (not sure if we need to address them all before the 1.1.6.4 release)

I've moved your comments to #608, which I created to handle just these types of questions. I realize there are a lot of additional enhancements that can be made to the user/page admin code, but at some point you have to draw the line in the sand and say "this is it for 1.1.6.4". I think we're at that point with the user/page admin modules.

in reply to: ↑ 29   Changed 6 years ago by DarTar

Replying to BrianKoontz:

Yes, because there is no possible MD5 hash that will match that value. It's the same principle used in Unix to lock a user account. [...] Consider it an additional safeguard. If we've gone so far as to delete a user, there's no need for a password.

ok so this answers my question re: the possibility of reinstating a previously removed user: this won't be possible with deleted users. I assume disabled users will be treated otherwise.

Yes, there are probably several places in the Wikka codebase that needs to check the status of a user when an action would not depend on the user actually being logged in.

ok, I suggest we open a separate ticket for this.

  Changed 6 years ago by BrianKoontz

  • status changed from reopened to closed
  • resolution set to fixed
  • description modified (diff)

Since tickets #608 and #647 address further upgrades and modifications to the User Administration module as requested here, there appear to be no more issues to address with respect to this ticket and 1.1.6.4, so I'm closing it (again).

  Changed 6 years ago by DarTar

Brian,

as per my last email and comment above, there's an important issue still pending for this ticket:

I haven't committed yet the userpages and userchanges actions required by the user admin module (!). I'll take care of this.

I'm not sure yet if I can find a good internet connection here, if I do I can try to fix these.

  Changed 6 years ago by BrianKoontz

  • status changed from closed to reopened
  • resolution fixed deleted

Missed that, sorry.

  Changed 6 years ago by BrianKoontz

(In [915]) These changes all stem from enabling the {{my*}} actions in AdminUsers. There is an important fix to the comment actions (hidden comments were being displayed) that needs to be ported to 1.1.7. New parameter ($user) added to Load...Comment() calls in main library.

Refs #25, #562, #575

  Changed 6 years ago by DarTar

Excellent, thanks for taking care of this, Brian.

  Changed 6 years ago by DarTar

  • status changed from reopened to closed
  • resolution set to fixed

Reviewed and tested - closing the ticket.

  Changed 6 years ago by BrianKoontz

  • status changed from closed to reopened
  • resolution fixed deleted

  Changed 6 years ago by BrianKoontz

The problem with the module is that one form is trying to do the work of multiple forms, so all POST fields are being processed, which results in things getting out of sync (for instance, clearing the search and then resubmitting returns unexpected results if num of records displayed has been modified previously...likewise, modifying the num of records displayed after a search has been performed does not return the expected results).

The proper fix is to completely refactor this module, and perform only one action at a time (i.e., use the value of 'submit' to determine if the user has asked for a search or something else). The quick fix is to insert enough branching instructions so as to properly process all permutations of fields that are submitted en masse.

See below for the quick fix, which I believe captures all combinations of searches and record display settings. It's rather ugly, though, and I doubt I'll remember what I did next week...

  Changed 6 years ago by BrianKoontz

(In [1007]) Addressed issues involving the simultaneous submission of multiple POST params in the same POST action. Refs #25

  Changed 6 years ago by DarTar

works for me now

  Changed 6 years ago by BrianKoontz

  • status changed from reopened to closed
  • resolution set to fixed

  Changed 6 years ago by DarTar

  • keywords 1.1.7-ported added

follow-up: ↓ 47   Changed 6 years ago by DarTar

  • keywords 1.1.7-unported added; 1.1.7-ported removed

Brian, I just realized that the upgrader needs a:

ALTER TABLE trunk_users ADD status enum('invited','signed-up','pending','active','suspended','banned','deleted')

My current AdminUser in trunk is broken and I had to fix the DB manually.

  Changed 5 years ago by BrianKoontz

  • keywords trunk-unported added; antispam 1.1.7-unported removed

  Changed 5 years ago by BrianKoontz

  • keywords antispam,trunk-unported added; trunk-unported removed

in reply to: ↑ 44   Changed 5 years ago by BrianKoontz

  • keywords antispam,trunk-ported added; antispam,trunk-unported removed

Replying to DarTar:

Brian, I just realized that the upgrader needs a: {{{ ALTER TABLE trunk_users ADD status enum('invited','signed-up','pending','active','suspended','banned','deleted') }}} My current AdminUser in trunk is broken and I had to fix the DB manually.

Looks like this was fixed in [1139].

Note: See TracTickets for help on using tickets.