internationalscratchwiki / scratch-confirmaccount-v3 Goto Github PK

For more consistency, the scratch_accountrequest table should be renamed to scratch_accountrequest_request even tho the name sounds redundant, to match scratch_accountrequest_block and scratch_accountrequest_history.

Also, the table's names and column's names should be put in (global) variables to avoid repetition and to make it easier to maintain.

I have received numerous reports of the email confirmation emails not being sent. I have yet to test this locally, but am noting this here as a development task.

Should be pretty simple.

Backend: add a field for when the block expires, update the database interactions to search based on the expiration date, and add a task to the cleanup job to delete expired bans.
Frontend: add a field in the admin interface (both on the form to add a ban and the form to edit one) and a thing to the ban page indicating the expiration date.

We can use CheckUser as we do on the existing system

Title says all. Also don't show in the menu for those who don't have permission.

Currently users can leave a comment for admin with no content. No harm in this, but probably shouldn't be allowed. It's also likely that admins can leave empty comments as well, which likewise should not be the case.

formatting can be like deletereason-dropdown

there can be generic ones in the default setting, then en will create a mediawiki page based on the needs of its policies

When a user submits an account request, see if their IP is blocked with the parameter "account creation disabled" and don't let them submit the form if that is the case.

https://github.com/InternationalScratchWiki/scratch-confirmaccount-v3/blob/master/src/ScratchConfirmAccount.i18n.alias.php

"DemenderUnCompte" should be writed like that "DemanderUnCompte"

We should make this.

My proposal is:

• Actions
  ** comment This action can be taken by both the admin and the user requesting. If the user does it, it automatically changes the status to awaiting-admin. If the admin does it, it automatically changes the status to awaiting-user, although the admin can move it elsewhere if needed.
  ** set-status Sets the status, with the input being a status below
  ** set-lock Sets the locked state to true or false (depending on whether or not locking should be seperate from approving/denying)
• Statuses
  ** approved Moves the request to Approved archive, creates the account with the specified password, then deletes the password from the request system
  ** awaiting-user Moves the request out of the "needs review" section, but still in an easily accessible place by the admin. When a user comments, it will be set to awaiting-admin
  ** awaiting-admin Where requests that users comment on go to. Also where new requests submitted go to. When an admin comments, it bounces back to awaiting-user
  ** denied Moves the request to the Denied archive, deletes the password, and sets a one-week timer that won't let them request again until then. The user cannot comment in this state (unless we make locking and unlocking seperate)

Any objections or changes? The one thing I'm not sure about is if locking should be tied to the request being denied or its own seperate action.

What I am suggesting might decrease the amount of obviously rejected account requests.

A minimum request length (probably around 50-100 characters) would prevent account requests that are too short, which would almost never get accepted (although there might need to be phrase blacklists for different languages). Preventing several phrases from the sample request notes being used would prevent submitting account requests that copy and past sample request notes.

If any of these were to be rejected, it should notify the user that they have not read S:CONTRIB properly and state it is most likely that they will not get accepted.

The only problems I can think of of this is this could be assisting users in writing their request notes a tiny bit (the 1 week delay may or may not help solve this) and users might modify random characters if they are using the sample request notes.

Somehow this slipped through the cracks. All that's needed is to add the required attribute, this doesn't even need to be backend-validated.

The extension normally should prohibit this. They most likely hit the submit button twice, but this means we have a race condition. Specifically, the following:

hasBeenSubmitted <- does user have active request?
(intermediate logic)
if hasBeenSubmitted then show error that user has already submitted request
else store request

The race condition is between the two. Ideally this should be done as a transaction to avoid this problem. This may require some significant refactoring since currently the way this is designed, each database operation is designed to be atomic. The way I am thinking to do this is make the database driver an argument to each of the database interaction functions (ones that do writes will require a writeable database instance). Then we can create a function for starting and committing a transaction. So it would be something like this:

db <- get writeable database and begin transaction
hasBeenSubmitted <- does user have active request? (determine using db)
(intermediate logic)
if hasBeenSubmitted then show error that user has already been submitted request
else store request (using db)
commit transaction for db
release db

Currently if an account request ban is set to expire it doesn't show the expiration date. It should be added.

Should be hidden

Repro:
(optional: comment out the verification part of the code)

1. request account
2. reject it
3. request account with same username and password
4. log in on FindRequest
5. you will be redirected to the old, "rejected" one

For the requester, don't allow them to submit comments unless 60 or more seconds have passed since their previous one. (Of course make this number configurable in code)

This will avoid the issue of CSS sometimes loading after the page does and also make the no-JS enhancements work properly.

Before, we thought the issue was a race condition (see #81, which was also good to fix), but now we're getting requests spaced hours apart from the same user, including one submitted less than a day after a previous one was requested. So, somehow we need to better check for duplicate requests.

This will likely be done by adding the button using JavaScript

We have now added it to Special:RequestAccount, also add it to Special:RequestAccount/FindRequest and RequestPage

#134 caused a fatal exception whenever viewing a request. This has been rolled back on production, but needs to be investigated.

See this topic for more information. It is unclear at this point when project comments will be migrated.

This happens when a request is automatically rejected for sitting as "awaiting-user" for over 30 days and two admins have commented.

The issue is that somehow my logic to only pick one admin per request was flawed. For fixing this it doesn't matter which admin to use when there are multiple choices, as long as there is only one.

Admins should be able to delete comments on a request. This includes comments by themselves, other admins, and the user. This can help for when there are accidentally left comments,

For example, if someone includes "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" as their request notes, the column containing request notes will overflow the page body and require scrolling to get to the "view" button. The solution is to break words if they are excessively long.

Reported by @Ascor8522

The order of parameters passed to requestTable in SpecialConfirmAccounts.php is swapped, but then swapped again in requestTable when passing to the pager (which is why this didn't actually break).

Let's be honest, a lot of people don't realize it's an error. Use the red box used by other MediaWiki components to make it more glaringly obvious it's an error.

The current method looks for who most recently commented the code and will deny if it's not the user requesting. This means that if someone comments a code, you can comment the same code to prevent them from being able to request an account. While it won't let you request one either, it does prevent them from requesting and can be used maliciously. So a new method of code finding may be needed.

For now, on the main page listing all active requests, you can only go to the requests's detail pages by using the link in the "action" section
However, the page also shows the username of the user who performed the request. Unfortunately, it doesn't act as a link to the user's Scratch profile.
For that, you need to first go the request's detail page, then click the link to go to the user's Scratch profile.

Would be nice if the main page could do that do.
Basically turning text into a link.

This can be done with Linker::formatComment

If the request notes are longer than a certain amount (say, 500 characters), cut them off in table view to avoid having them monopolize the page. Show them entirely on the request page though, since they're in a scrollable box (and it is important to be able to see the whole request notes).

Also potentially put a max length (something like 5000 characters, far longer than anyone would actually include in their request) to avoid someone trying to overwhelm an admin's browser.

The removing from expired requests can be done as a job that's triggered randomly or something like that. Removing from accepted requests should be done as soon as the new account is created (after creation though in case creation fails).

The current approach might create swear words or unwantedly get some codes blocked, while pure hex may trip the phone censors. Instead, I recommend switching to delimited hex (like the old system, but change to using - as punctuation and punctuate every 4 chars)

Basic idea for the solution

• Have a hidden input on the request form for the timestamp when the page was opened
• When submitting the form, check if the request was last modified since that timestamp
• If the request wasn't modified, just continue on as normal
• If the request was modified, then don't make the modification, show the request page (up-to-date including the intermediate change) with the fields pre-populated, and a nice big error message indicating there was a time conflict

We can use hooks for this. Potentially LoginFormValidErrorMessages

A nice little "copy" icon would appear next to the verification code that would automatically put it in the clipboard and show a tooltip after it's clicked saying "Verification code copied to clipboard!" or something to that effect after being clicked and the code copied.

I'm not sure what the best way to go about this is, as we delete the password. We could alternately allow users to view requests with Scratch comment verification, similar to ScratchLogin (this would also help in the cases where users forget their request passwords, as we currently can't reset those and there wouldn't be a safe way to give the passwords to the requesters anyway).

There are a few ideas for how to go about this:

1. Just use comment verification again
2. Allow admins to reset passwords manually (though there wouldn't be a safe way to deliver them to users)

A GH search doesn't show any results in code, only in i18n. Should this string be removed?

Specifically the code in getNumberOfRequestsByStatusAndUser is an inefficient operation. It requires making one big query that has every request that the user has ever handled, which scales poorly. A better way to do this would be a single query along these lines: select every request ID which is of status [status] and has a corresponding history entry performed by [user]. This offloads the work to the database and also allows the database to handle it more efficiently. In particular, by filtering based on the status, since odds are the majority of the requests won't be "awaiting-admin" (the query optimizer will take care of filtering in the most efficient order, that's not our problem), that will make this much more efficient.

This means on non-English Wikis, blocks are failing to enter because they expire immediately.

If a username is not a "safe" Scratch username (for example, _jvvg_ or jvvg__test) and we make a user use another account, have a way to allow that username to bypass the hard requirements (Scratcher status and account age).

Components:

• DB design: have a table with one column (username, which would be a primary key)
• Admin panel: simply allow adding and removing usernames (don't bother adding search at the moment, we can revisit this at a later date if needed)
• Request: bypass requirements if the username is granted a bypass
• Request action: if a request is accepted, remove the username from the list

Probably the easiest way to do this would be either to add this to the password clearing job or to create a new job. The job would just find requests with status "awaiting-user" that haven't been commented on in more than some time period (default 30 days maybe?) and for each request set the status to rejected and add an automatically generated comment (the comment could be "from" the admin who first handled the request, since there is guaranteed to be one otherwise the request won't have status awaiting-user).

This would both keep things cleaner from an admin perspective and not force users who abandoned requests to continue with their old one and instead let them request anew.

We kicked the can down the road on this one while originally developing the extension so we could prioritize other things, but it's probably time to start looking into this given that the rest of the extension seems to be working fairly well. It's not an urgent priority, but something to start thinking about (also for reference, the oldest request with status awaiting-user is from October 3, so we still aren't close to the 30 days yet anyway for that request).

This way even a user whose request is accepted can view it after the fact since the password hash won't be immediately removed.

See https://en.scratch-wiki.info/wiki/MediaWiki_talk:Scratch-confirmaccount-password for what it should be

For example, suppose we get a request from jvvg, who then goes on to make an edit. It's reasonable to assume that the edit and request were made from the same IP, so looking back at the accepted request will trigger the alt warning saying that jvvg has edited from that IP.