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

Strip security information from user documents #45

Merged
merged 6 commits into from
Jun 23, 2023
Merged

Conversation

kennsippell
Copy link
Member

Copy link
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

Approving to unblock, but have a light suggestion for readability and parallel structure against existing code.

Deferring to @garethbowen on this point: I would really want to test this important change, but do not have time this week. I should likely have time next week and am happy to do it then.

lib/importer.js Outdated
Comment on lines 102 to 108
const isUserDoc = row.doc && row.doc.type === 'user' && row.doc._id.startsWith('org.couchdb.user:');
if (isUserDoc) {
delete row.doc.password_scheme;
delete row.doc.derived_key;
delete row.doc.salt;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lightly would suggest parallel structure to our sanitise() call. That'd be add a function, maybe removePasswordHash() or something, that we'd call here instead of doing the declaration of isUserDoc and the delete inline within the map() call.

The main reason is that derived_key doesn't pop out why you'd delete it, but removePasswordHash makes a lot sense to be here!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition it's better not to define a function in a loop. Depending on the compiler it's bad for performance, but if nothing else it makes reuse somewhere else harder.

Furthermore this is a complicated line to put as an argument to a function - better to create a new variable called docs to store the map result and then invoke the format function.

// at the top of the file
      const isUserDoc = doc => doc && doc.type === 'user' && doc._id.startsWith('org.couchdb.user:');

// inline here 
      const docs = couchDbResult.rows.map(row => {
        if (isUserDoc(row.doc)) {
          delete row.doc.password_scheme;
          delete row.doc.derived_key;
          delete row.doc.salt;
        }
        return [row.doc];
      });
      var insertSql = format(INSERT_DOC_STMT, postgresTable, docs);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys. I believe I addressed all feedback here. I'm not quite 100% sure... sorry if I missed something.
Defs more readable now.

Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved so as not to block - readability suggestion inline.

@kennsippell kennsippell merged commit 174bb19 into main Jun 23, 2023
@kennsippell kennsippell deleted the 137-sync-users branch June 23, 2023 06:29
@mrjones-plip
Copy link
Contributor

Thanks @kennsippell - end result looks good!

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

Successfully merging this pull request may close these issues.

3 participants