Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup usernames created with invalid characters in community providers #824

Closed
justinwb opened this issue Oct 2, 2018 · 14 comments
Closed
Assignees

Comments

@justinwb
Copy link
Contributor

justinwb commented Oct 2, 2018

We'll need to clean up usernames that were created with invalid characters (e.g. periods, etc.) on inrupt.net and solid.community after we push the validation fix in #818.

I would propose that we do a quick search, pull the associated e-mail, and shoot them an e-mail letting them know that we need to change their username, or render it inactive unless we hear back from them with an update to it.

@justinwb justinwb added this to To do in Node Solid Server via automation Oct 2, 2018
@mintunitish
Copy link
Contributor

Is this done? If not can I grab this issue?

@kjetilk
Copy link
Member

kjetilk commented Oct 18, 2018

@mintunitish Thanks for volunteering, but I think this is something we have to do on the backend, so we would need to do it internally.

@megoth
Copy link
Contributor

megoth commented Oct 31, 2018

I realize now that I misread this issue a bit: I was thinking it should include blacklisted usernames as well.

With that said, should this issue include blacklisted usernames as well? (I think it should.)

And in any case; what process do we want to set up for this?

  1. Should it be a script that simply removes invalid usernames?
  2. Should it communicate to the owner that it is problematic, and given a set time to change it?
    2a) What if the owner hasn't given an email; should we add a bit of markup in the index as a notification?

@megoth megoth moved this from To do to In progress in Node Solid Server Oct 31, 2018
@megoth megoth self-assigned this Oct 31, 2018
@melvincarvalho
Copy link
Contributor

@megoth there's no real way to fix it. What I think should be communicated is that our systems had an error during account creation. Certainly not tho only approach, but I think a reasonable one. Happy for other ideas.

@kjetilk
Copy link
Member

kjetilk commented Oct 31, 2018

Yeah, I think we need to give a notice in advance and apologize for an error during account creation....

The blacklist should be involved, as well as the sanitization that we now have in place. We should send an email out first, and then allow a couple of weeks before we remove. So, this is basically the work of a couple of one-off scripts.

@kjetilk kjetilk added the inrupt-planning Issues that Inrupt will consider when planning development cycles label Nov 2, 2018
@megoth
Copy link
Contributor

megoth commented Nov 5, 2018

Ok, so this is the process that I'm considering.

  1. Adding command blacklist to bin/solid: by default it would give you a list of usernames that are in violation of the current blacklist
  2. Adding option --notify: this will add a note to the top of a user's index.html and send an email, if possible, that their username is in violation of our blacklist and will be deleted within two weeks unless they send an email to the POD provider requesting a new WebID
  3. Adding option --delete: this will delete the usernames

Does this look ok? I could automate step 3, but think it's better with a manual step for now

@Ryuno-Ki
Copy link

Ryuno-Ki commented Nov 5, 2018

What about renaming those accounts? Say replacing periods with dashes?
Then you would „just” have to inform them that the name changed.
(I had not looked into implementation details here).

Regarding the blacklist … pay attention to have a robust RegEx (there were some DDoS attacks against RegExps the other day … at least SNYK.io sent out some warnings - can dig up details if needed).

https://github.com/minimaxir/big-list-of-naughty-strings gives a list of possible candidates to throw against your algorithm.

@megoth
Copy link
Contributor

megoth commented Nov 5, 2018

@Ryuno-Ki that is another way of doing it, sure; I'm good with either way.

We've already implemented a BlacklistService btw (#893) that makes use of https://github.com/marteinn/The-Big-Username-Blacklist. The service is not making use of any RegEx, it's simply checking a giving word (in our case username) against a list of blacklisted words.

@megoth
Copy link
Contributor

megoth commented Nov 5, 2018

Sorry, I seem to forget that this issue is not only about blacklisted usernames O_O

I'll rename the command so that it doesn't only pertain to blacklisted usernames.

Does the command invalidusernames sound like a catch-em-all?

@Ryuno-Ki
Copy link

Ryuno-Ki commented Nov 5, 2018

Left some comments.
While I am looking at how usernames are computed …

Falsehoods Programmers Believe About Names is worth a read also.

Looking at https://github.com/solid/node-solid-server/blob/9b627e47ee191ea7b2a8f1710eb6a8a952e5557e/lib/requests/create-account-request.js#L202 - it looks like you would ban every non-alphanumeric username.

I am wondering what an international audience would do here.
Maybe you want to follow established frameworks like django's slugify which uses unicodedata to normalise strings.
There are pendants in node like unorm or normalize-strings-npm5 to achieve that (I'd tend to unorm, since the latter uses a RegEx …).

@megoth
Copy link
Contributor

megoth commented Nov 5, 2018

It is important to note that usernames are exposed in the URI of a users' POD, so we need strict rules on which usernames we accept. But in that sense we could accept all usernames that are valid subdomains, and we are limiting that today.

I propose we create a separate issue to allow more characters in the usernames (I'm starting to sound like a broken record when it comes to suggesting to create new issues =P )

@Ryuno-Ki Maybe you want to create an issue for it? Or if you want to discuss it with the community first, I suggest creating a thread on https://forum.solidproject.org/

@kjetilk kjetilk added inrupt-sprint Issues that Inrupt plans to work on in near future and removed inrupt-planning Issues that Inrupt will consider when planning development cycles labels Nov 6, 2018
@megoth
Copy link
Contributor

megoth commented Nov 8, 2018

I'm well on my way to completing the step where we inform the users of their invalid username. But I need to configure which email-address we should give users that want to move their username. I'm wondering where I should code this, and can think of a couple of options:

  1. As part of the init-process, so that it resides in config.json, or
  2. As value to the --notify option in the script

Both can be done of course, but I'm leaning toward option 1, as I think it's smart to have this support-email address for the POD provider in general.

I'll start with option 1, but am open to suggestion (hence this comment).

@megoth megoth moved this from In progress to Needs review in Node Solid Server Nov 8, 2018
@justinwb
Copy link
Contributor Author

justinwb commented Nov 9, 2018

@megoth Seems that both could be valuable. By default it pulls the value from config.json, but if the --notify flag is passed, then let that take precedence.

@megoth megoth moved this from Needs review to Reviewer approved in Node Solid Server Nov 9, 2018
@megoth megoth moved this from Reviewer approved to Done in Node Solid Server Nov 9, 2018
@kjetilk kjetilk moved this from Done to Released in Node Solid Server Nov 20, 2018
@michielbdejong michielbdejong removed inrupt-sprint Issues that Inrupt plans to work on in near future labels Mar 25, 2019
@michielbdejong
Copy link
Member

@kjetilk can this be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants