Invalidusernames command #919

megoth · 2018-11-08T21:15:12Z

This is my attempt at fixing #824.

A lot of things happening here, but it boils down to a command with two options:

[solid] invalidusernames: Will list all invalid usernames that exists on the server today
[solid] invalidusernames --notify: Will go through all invalid accounts with invalid usernames and
1. Make a backup of their existing index.html (move it to index.backup.html)
2. Create a new index.html that contains info that this account will be deleted in two weeks (based on the new template default-views/account/invalid-username.hbs)
3. Send an email (if available) with the same notification (based on the template new default-templates/emails/invalid-username.js)
[solid] invalidusernames --delete: Will delete all accounts with invalid usernames

I've also added a new option in the init script that prompts for a support-email for POD-providers (this will only show if the multiuser-property is set to true). As part of this I made use of the isEmail-method in https://www.npmjs.com/package/validator, which I understand from #916 might not've been the most well-thought through decision.

I've tried writing some tests, but find it difficult to write tests for a lot of this =/

Any and all feedback are (as always) appreciated, and I hope I'm not too far off with this solution.

RubenVerborgh · 2018-11-08T21:17:59Z

Why is there both a invalid-username.hbs and .js?

megoth · 2018-11-08T21:21:12Z

invalid-username.hbs is for generating the index.html that replaces the (backed up) index.html in a user's POD
invalid-username.js is for generating the email that is sent (if email address is available)

Ryuno-Ki · 2018-11-08T21:29:07Z

bin/lib/common.js

+const { cyan, bold } = require('colorette')
+const util = require('util')
+
+module.exports = {}


I could be wrong, but isn't this the default behaviour in CommonJS?

Yeah, you're right, it's unnecessary. I'll remove it.

Ryuno-Ki · 2018-11-08T21:32:36Z

bin/lib/common.js

+    })
+  } catch (err) {
+    // No file exists, not a problem
+    console.log(cyan(bold('TIP')), 'create a config.json: `$ solid init`')


Think about using a logger library (likely a new issue). My favourite is loglevel (because it's simple and supports plugins).

winston is popular also.

This way you can fine-tune the logging.

Definitely a new issue, the required changes would be server-wide.
That said, it /should/ eventually be done, since one of the most frequently requested features about logging is timestamps, which our current logger does not support.

Logging is great for long-running processes (and the server has logging), but this is just a short one-time job.

Ah, actually, I should probably reuse debug that's laid out via lib/debug.js.

We might consider a logger too, but yeah, that should be its own issue.

Ryuno-Ki · 2018-11-08T21:33:27Z

bin/lib/common.js

+  let configFile = argv['configFile'] || './config.json'
+
+  try {
+    const file = await util.promisify(fs.readFile)(configFile)


Is that the only occurence of fs?
If so, would const { readFile } = require('fs') work also?

Plus const { promisify } = require('util').

After Node 8, I'm pretty sure all fs methods have a Promise interface natively, so the promisify is not even needed.

https://nodejs.org/api/fs.html#fs_fs_promises_api currently has stability: experimental.

currently has stability: experimental.

Ohhh right, I was thinking about fs-extra, never mind :)

Ok, I'll write away promisify.

Ryuno-Ki · 2018-11-08T21:35:53Z

bin/lib/invalidUsernames.js

+    .option('--notify', 'Will notify users with usernames that are invalid')
+    .option('--delete', 'Will delete users with usernames that are invalid')
+    .description('Manage usernames that are invalid')
+    .action(async (options) => {


My personal view: I move the implementation of the action into another file / function.
This way I can have different interfaces for it (CLI, Web UI etc).
Plus, it is easier to test.
Plus, it is a separation of concerns (CLI vs. using the lib).

I would do it 100% if I thought this code was reusable, which I'm leaning toward it isn't =/

As I mentioned earlier, I found it difficult to write tests for this. It relies on a lot of file-based functionality, which makes it unsuited for unit tests. I could write integration tests, but didn't see any for the other commands, so didn't know how to attack it.

From an Angular point of view I'd argue with Dependency Injection.
That is, providing the fs as an argument to the function.
But it feels meh somehow.

I'm not that familiar with dependency injection in Node. I've tried MockRequire earlier, but my tests always turned a nightmare to maintain.

Ryuno-Ki · 2018-11-08T21:40:11Z

bin/lib/invalidUsernames.js

+      }
+
+      const invalidUsernames = await getInvalidUsernames(config)
+      const host = SolidHost.from({ port: config.port, serverUri: config.serverUri })


Hm, I wonder, whether const host = SolidHost.from({ config }) would work.
Like in Unpacking fields from objects passed as function parameter

I reckon you mean const host = SolidHost.from(config)? (As const host = SolidHost.from({ config }) is a shortcut for const host = SolidHost.from({ config: config }).)

Could work, but I wanted to stay specific reg this.

Agreed, more specific is better in this case (since it's clearer which args are actually used)

Fair enough.

Ryuno-Ki · 2018-11-08T21:43:05Z

bin/lib/invalidUsernames.js

+    })
+}
+
+async function createNewIndexFile (username, accountManager, invalidUsernameTemplate, dateOfRemoval, supportEmail, fileOptions) {


Personally I don't like functions with so many arguments. I consider them a code smell.
Like the function trying to do too much.
It helps to have smaller functions, which are then called in order.

I don't like long signatures either, but in this case it's not really reusable functions, but I wanted more readable code by splitting it up into "logical" chunks of code (in what I hope is good named functions).

But yeah, if we wanted to reuse this functionality, I should probably restructure it a bit to decrease the length of the signatures.

Well, if you are struggling with writing tests, this should be argument enough to break the function up, shouldn't it?

If I knew how to break up this code into reusable, testable functions, I would do it ;)

Okay, I can write the high-level steps I see. Implementation goes into the functions then …

async function createNewIndexFile (/* arguments */) { if (isBackupNeeded(/* arguments */)) { await Promise.all([ backupIndexHtml(/* arguments */) backupAcl(/* arguments */) ]) await Promise.all([ generateNewIndexHtml(/* arguments */) generateNewAcl(/* arguments */) ]) logCompletion(/* arguments */) } }

or maybe group the backup + creation into a single function (like an transaction).
You may want to timestamp the index.backup.html also.

Plus, looking at the function name, createNewIndexFile is misleading, since it does more than it says …
so its caller would need to call more functions.

Ryuno-Ki · 2018-11-08T21:43:44Z

bin/lib/invalidUsernames.js

+    .map(async username => {
+      try {
+        const user = accountManager.userAccountFrom({ username })
+        await oidcManager.users.deleteUser(user)


IIRC I've read in the docs, that other auth managers (than oidc) would be supported as well.
So I am wondering here, why it has to be oidc.
Maybe add an comment? Could be that it is a „stupid question” for contributors familiar with the code.

No, it's not a stupid question; We support WebID-TLS and WebID-OIDC now, and WebID-TLS don't store user-data like WebID-OIDC does. (If I'm wrong, pleasepleaseplease correct me =P )

I could try to abstract away the OIDC-part of this, to accommodate future auth managers better, but I think that's going to might take a long time (for me, at least - I'm not that into auth managers yet). So I was thinking this can be done when we implement support for other auth managers.

Maybe you could leave a comment here, so the next person touching the code knows your thoughts (since I'd not look up the associated PR immediately, but rather git blame …).

I don't know the conventions in this project, though.

Ryuno-Ki · 2018-11-08T21:43:50Z

bin/lib/invalidUsernames.js

+  const userDirectory = accountManager.accountDirFor(username)
+  const currentIndex = path.join(userDirectory, 'index.html')
+  const currentIndexExists = await fileExists(currentIndex)
+  const backupIndex = path.join(userDirectory, 'index.backup.html')


This is still in documentRoot, i.e. accessible from the Internet (for everybody?)?

Oh, should probably copy the acl-file as well O_O

Ryuno-Ki · 2018-11-08T21:45:32Z

bin/lib/invalidUsernames.js

+  const isUserDirectory = new RegExp(`.${hostname}$`)
+  return files
+    .filter(file => isUserDirectory.test(file))
+    .map(userDirectory => userDirectory.substr(0, userDirectory.length - hostname.length - 1))


Why exactly userDirectory.length - hostname.length - 1?

Userdirectory can be read as [username].[hostname], so it's to remove the dot between username and hostname.

Is it fixed that there must be a dot?
If so, can't you split the hostname (and take the second-to-last value)?

Ryuno-Ki · 2018-11-08T21:45:58Z

bin/lib/invalidUsernames.js

+  if (usernames.length === 0) {
+    console.info('No invalid usernames was found')
+  }
+  console.info(`${usernames.length} invalid usernames were found:${usernames.map(username => `\n- ${username}`)}`)


Shouldn't that go into an else branch?

Oh, forgot a return in the if-block (that's at least how I like to write that logic).

Ryuno-Ki · 2018-11-08T21:50:18Z

bin/lib/invalidUsernames.js

+}
+
+async function notifyUsers (usernames, accountManager, config) {
+  const twoWeeksFromNow = Date.now() + 14 * 24 * 60 * 60 * 1000


If you ever struggle with datetimes, look at date-fns or moment.

Yeah, I prefer the former, but didn't want to include it just for this.

Ryuno-Ki · 2018-11-08T21:51:12Z

bin/lib/invalidUsernames.js

+    }))
+    const emailService = new EmailService(templates.email, config.email)
+    const sendingEmails = await users
+      .filter(user => !!user.emailAddress)


Maybe more readable as .filter(user => Boolean(user.emailAddress))?

I disagree, !! is more standard, and is used throughout our libs.

!! reminds me on the output of uglify :-)
But being used throughout is a valid argument.

Ryuno-Ki · 2018-11-08T21:52:05Z

bin/lib/invalidUsernames.js

+}
+
+async function updateIndexFiles (usernames, accountManager, dateOfRemoval, supportEmail) {
+  const invalidUsernameFilePath = path.join(process.cwd(), 'default-views/account/invalid-username.hbs')


Why do you use path.join when the second argument is not OS-agnostic?

Was planning to see if I could find another a way to reference default-views/account, but forgot about it. I'll make another try, or at least fix this to be OS-agnostic.

Ryuno-Ki · 2018-11-08T21:53:38Z

default-templates/emails/invalid-username.js

+     */
+    text: `Hi,
+
+We're sorry to inform you that the username for account ${data.accountUri} is not allowed after changes to username policy.


Maybe refer to username policy somehow? I'd be wondering, what exactly is in there if I would receive such an e-mail.

I don't think this is necessary, but open to be convinced otherwise.

Ryuno-Ki · 2018-11-08T21:54:12Z

default-views/account/invalid-username.hbs

+  <meta name="viewport" content="width=device-width, initial-scale=1">
+  <title>Invalid username</title>
+  <link rel="stylesheet" href="/common/css/bootstrap.min.css">
+  <script></script>


Huh? An empty tag?

Will remove.

Ryuno-Ki · 2018-11-08T21:54:45Z

default-views/account/invalid-username.hbs

+</head>
+<body>
+<div class="container">
+  <h4>Invalid username</h4>


Why not an h1? Semantic Web and so on.

Because of legacy; the other views use h4 (I reckon they did it like this initially because they thought h1 was to big; We (mostly) use bootstrap (3.3.4) for the presentation).

@megoth hehe, that's exactly why, h1 was too big on bootstrap :)

https://getbootstrap.com/docs/3.3/customize/#typography *cough

Ryuno-Ki · 2018-11-08T22:04:52Z

I left some comments while walking through the files.

kjetilk · 2018-11-08T22:54:09Z

Great with a lot of comments, I just wanted to note that this code will most likely run only once, so it is not worth spending too much time on it.

megoth added 9 commits November 8, 2018 21:51

Listing blacklisted usernames that exists

25d8075

Listing usernames from root

d1143b5

Extracted validation of username into user-utils

fd6b00b

Updating index files and sending emails to users with invalid usernames

7004ea1

Added supportEmail to init script

62b391e

Making use of supportEmail from config

40e89da

Delete users with usernames that are invalid

5c88ba8

Renaming the command to invalidusernames

356301a

Should only show supportEmail-option when it's a multiuser server

db73c6d

megoth requested review from kjetilk, RubenVerborgh and dmitrizagidulin November 8, 2018 21:15

ghost assigned megoth Nov 8, 2018

ghost added the in progress label Nov 8, 2018

RubenVerborgh approved these changes Nov 8, 2018

View reviewed changes

Ryuno-Ki reviewed Nov 8, 2018

View reviewed changes

kjetilk merged commit cdaff9b into master Nov 8, 2018

ghost removed the in progress label Nov 8, 2018

megoth mentioned this pull request Nov 9, 2018

Improvement/invalidusername command #924

Merged

Invalidusernames command #919

Invalidusernames command #919

Conversation

megoth commented Nov 8, 2018

RubenVerborgh commented Nov 8, 2018

megoth commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megoth Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megoth Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryuno-Ki Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryuno-Ki commented Nov 8, 2018

kjetilk commented Nov 8, 2018

megoth Nov 8, 2018 •

edited

megoth Nov 8, 2018 •

edited

Ryuno-Ki Nov 8, 2018 •

edited