From a572f52f46f9b5a93b0aaff7d5bbe950cc452a05 Mon Sep 17 00:00:00 2001 From: pajgo Date: Fri, 20 Sep 2019 16:26:18 +0600 Subject: [PATCH 1/2] feat: inactive users index --- rfcs/inactive_users/user_index.md | 22 ++++++++++++++++++++++ src/actions/activate.js | 5 +++++ src/actions/register.js | 6 ++++++ src/actions/remove.js | 4 ++++ src/constants.js | 4 ++++ src/utils/inactiveUsers/index.js | 27 +++++++++++++++++++++++++++ 6 files changed, 68 insertions(+) create mode 100644 rfcs/inactive_users/user_index.md create mode 100644 src/utils/inactiveUsers/index.js diff --git a/rfcs/inactive_users/user_index.md b/rfcs/inactive_users/user_index.md new file mode 100644 index 000000000..cc75fdca8 --- /dev/null +++ b/rfcs/inactive_users/user_index.md @@ -0,0 +1,22 @@ +# Inactive users index + +## Overview and motivation +As other task dependency and to provide additional internal statistics the Service needs additional index +to track users that didn't pass the activation. + +## `inactive-users` index +It's an Additional Redis Sorted Set which contains the list of IDs of the `inactive` users. +Each item score is equal to the `timestamp` set on user creation. + +To avoid hardcoded SET name new `USERS_INACTIVATED` constant introduced. + +## Inactive user tracking utils +New `deleteFromInactiveUsers(userID)` and `addToInactiveUsers(userID)` methods used to control data stored inside `inactive-users` set. + +## Registration process +When the user succeeds registration but activation not requested, the new entry added to `inactive-users`. + +**NOTE:** Old Redis `expire` setting methods left in their place, to save old service behavior. + +## Activation process +When the user succeeds activation the entry deleted from `inactive-users`. diff --git a/src/actions/activate.js b/src/actions/activate.js index 783f13029..073830ea9 100644 --- a/src/actions/activate.js +++ b/src/actions/activate.js @@ -5,6 +5,8 @@ const jwt = require('../utils/jwt.js'); const { getInternalData } = require('../utils/userData'); const getMetadata = require('../utils/getMetadata'); const handlePipeline = require('../utils/pipelineError.js'); +const { removeFromInactiveUsers } = require('../utils/inactiveUsers'); + const { USERS_INDEX, USERS_DATA, @@ -126,6 +128,9 @@ function activateAccount(data, metadata) { .persist(userKey) .sadd(USERS_INDEX, userId); + /* delete user id from the inactive users index */ + removeFromInactiveUsers(pipeline, userId); + if (alias) { pipeline.sadd(USERS_PUBLIC_INDEX, userId); } diff --git a/src/actions/register.js b/src/actions/register.js index ba0413630..32db3a19e 100644 --- a/src/actions/register.js +++ b/src/actions/register.js @@ -22,6 +22,8 @@ const checkLimits = require('../utils/checkIpLimits'); const challenge = require('../utils/challenges/challenge'); const handlePipeline = require('../utils/pipelineError'); const hashPassword = require('../utils/register/password/hash'); +const { addToInactiveUsers } = require('../utils/inactiveUsers'); + const { USERS_REF, USERS_INDEX, @@ -208,7 +210,11 @@ async function performRegistration({ service, params }) { pipeline.hset(USERS_USERNAME_TO_ID, username, userId); if (activate === false && config.deleteInactiveAccounts >= 0) { + /* TODO Remove this line when last task in group merged */ pipeline.expire(userDataKey, config.deleteInactiveAccounts); + + /* Add user id to the inactive users index */ + addToInactiveUsers(pipeline, userId, audience); } await pipeline.exec().then(handlePipeline); diff --git a/src/actions/remove.js b/src/actions/remove.js index cdad191e2..b13b2625a 100644 --- a/src/actions/remove.js +++ b/src/actions/remove.js @@ -9,6 +9,7 @@ const getMetadata = require('../utils/getMetadata'); const handlePipeline = require('../utils/pipelineError'); const { USERS_INDEX, + USERS_INACTIVATED, USERS_PUBLIC_INDEX, USERS_ALIAS_TO_ID, USERS_SSO_TO_ID, @@ -92,6 +93,9 @@ async function removeUser({ params }) { transaction.srem(USERS_PUBLIC_INDEX, userId); transaction.srem(USERS_INDEX, userId); + /* Delete user from the inactive users index, if user not activated */ + transaction.zrem(USERS_INACTIVATED, userId); + // remove metadata & internal data transaction.del(key(userId, USERS_DATA)); transaction.del(key(userId, USERS_METADATA, audience)); diff --git a/src/constants.js b/src/constants.js index 525284ecc..0c5867e70 100644 --- a/src/constants.js +++ b/src/constants.js @@ -6,6 +6,10 @@ module.exports = exports = { USERS_PUBLIC_INDEX: 'users-public', USERS_REFERRAL_INDEX: 'users-referral', ORGANIZATIONS_INDEX: 'organization-iterator-set', + + /* inactive user ids set */ + USERS_INACTIVATED: 'users-inactivated', + // id mapping USERS_ALIAS_TO_ID: 'users-alias', USERS_SSO_TO_ID: 'users-sso-hash', diff --git a/src/utils/inactiveUsers/index.js b/src/utils/inactiveUsers/index.js new file mode 100644 index 000000000..1827e182e --- /dev/null +++ b/src/utils/inactiveUsers/index.js @@ -0,0 +1,27 @@ +const { + USERS_INACTIVATED, +} = require('../../constants'); + +/** + * Add user id to inacive users list + * @param {ioredis} redis + * @param {userId} userId + */ +function addToInactiveUsers(redis, userId) { + const created = Date.now(); + redis.zadd(USERS_INACTIVATED, created, userId); +} + +/** + * Remove user id from inactive users list + * @param {ioredis} redis + * @param {userId} userId + */ +function removeFromInactiveUsers(redis, userId) { + redis.zrem(USERS_INACTIVATED, userId); +} + +module.exports = { + addToInactiveUsers, + removeFromInactiveUsers, +}; From 9612990ef8ae62822b5de9f2b28e0eb54393d6e2 Mon Sep 17 00:00:00 2001 From: pajgo Date: Thu, 26 Sep 2019 18:19:22 +0600 Subject: [PATCH 2/2] feat: inactive users index * index cleanup --- rfcs/inactive_users/user_index.md | 12 +++++++++--- src/actions/activate.js | 10 +++++++--- src/actions/register.js | 8 ++++++-- src/utils/inactiveUsers/index.js | 24 ++++++++++-------------- 4 files changed, 32 insertions(+), 22 deletions(-) diff --git a/rfcs/inactive_users/user_index.md b/rfcs/inactive_users/user_index.md index cc75fdca8..ea594ea69 100644 --- a/rfcs/inactive_users/user_index.md +++ b/rfcs/inactive_users/user_index.md @@ -10,9 +10,6 @@ Each item score is equal to the `timestamp` set on user creation. To avoid hardcoded SET name new `USERS_INACTIVATED` constant introduced. -## Inactive user tracking utils -New `deleteFromInactiveUsers(userID)` and `addToInactiveUsers(userID)` methods used to control data stored inside `inactive-users` set. - ## Registration process When the user succeeds registration but activation not requested, the new entry added to `inactive-users`. @@ -20,3 +17,12 @@ When the user succeeds registration but activation not requested, the new entry ## Activation process When the user succeeds activation the entry deleted from `inactive-users`. + +## Index cleanup +Temporarily add `inactive-users` index cleanup. The functionality will move into one LUA script +with `delete inactive users` logic. This will save us from `dlock` based implementations and all operations will execute in `one-shot` avoiding race conditions. +On `registration` cleanup method executes before user creation. + +On `activation` cleanup method executes before any checks performed by `activation` action. + + diff --git a/src/actions/activate.js b/src/actions/activate.js index 073830ea9..460eeb778 100644 --- a/src/actions/activate.js +++ b/src/actions/activate.js @@ -5,10 +5,11 @@ const jwt = require('../utils/jwt.js'); const { getInternalData } = require('../utils/userData'); const getMetadata = require('../utils/getMetadata'); const handlePipeline = require('../utils/pipelineError.js'); -const { removeFromInactiveUsers } = require('../utils/inactiveUsers'); +const { cleanInactiveUsersIndex } = require('../utils/inactiveUsers'); const { USERS_INDEX, + USERS_INACTIVATED, USERS_DATA, USERS_REFERRAL_INDEX, USERS_PUBLIC_INDEX, @@ -129,7 +130,7 @@ function activateAccount(data, metadata) { .sadd(USERS_INDEX, userId); /* delete user id from the inactive users index */ - removeFromInactiveUsers(pipeline, userId); + pipeline.zrem(USERS_INACTIVATED, userId); if (alias) { pipeline.sadd(USERS_PUBLIC_INDEX, userId); @@ -178,7 +179,7 @@ function hook(userId) { * @apiParam (Payload) {String} [audience] - additional metadata will be pushed there from custom hooks * */ -function activateAction({ params }) { +async function activateAction({ params }) { // TODO: add security logs // var remoteip = request.params.remoteip; const { token, username } = params; @@ -196,6 +197,9 @@ function activateAction({ params }) { erase: config.token.erase, }; + // TODO: REMOVEME: Execute `DeleteInactiveUsers` LUA script + await cleanInactiveUsersIndex.call(this); + return Promise .bind(context) .then(verifyRequest) diff --git a/src/actions/register.js b/src/actions/register.js index 32db3a19e..06cbc9383 100644 --- a/src/actions/register.js +++ b/src/actions/register.js @@ -22,12 +22,13 @@ const checkLimits = require('../utils/checkIpLimits'); const challenge = require('../utils/challenges/challenge'); const handlePipeline = require('../utils/pipelineError'); const hashPassword = require('../utils/register/password/hash'); -const { addToInactiveUsers } = require('../utils/inactiveUsers'); +const { cleanInactiveUsersIndex } = require('../utils/inactiveUsers'); const { USERS_REF, USERS_INDEX, USERS_SSO_TO_ID, + USERS_INACTIVATED, USERS_DATA, USERS_USERNAME_TO_ID, USERS_ACTIVE_FLAG, @@ -174,6 +175,9 @@ async function performRegistration({ service, params }) { await verifySSO(service, params); } + // TODO: REMOVEME: Execute `DeleteInactiveUsers` LUA script + await cleanInactiveUsersIndex.call(service); + const [creatorAudience] = audience; const defaultAudience = last(audience); const userId = service.flake.next(); @@ -214,7 +218,7 @@ async function performRegistration({ service, params }) { pipeline.expire(userDataKey, config.deleteInactiveAccounts); /* Add user id to the inactive users index */ - addToInactiveUsers(pipeline, userId, audience); + redis.zadd(USERS_INACTIVATED, created, userId); } await pipeline.exec().then(handlePipeline); diff --git a/src/utils/inactiveUsers/index.js b/src/utils/inactiveUsers/index.js index 1827e182e..6c4bb9e80 100644 --- a/src/utils/inactiveUsers/index.js +++ b/src/utils/inactiveUsers/index.js @@ -3,25 +3,21 @@ const { } = require('../../constants'); /** - * Add user id to inacive users list - * @param {ioredis} redis - * @param {userId} userId + * NOTE: Contents of this file will be removed when `DeleteInactiveUsers` feature merged. + * To avoid `dlock` based locks, index cleanup and inactive user remove process will be merged into one LUA script. */ -function addToInactiveUsers(redis, userId) { - const created = Date.now(); - redis.zadd(USERS_INACTIVATED, created, userId); -} /** - * Remove user id from inactive users list - * @param {ioredis} redis - * @param {userId} userId + * Cleans Inactive user index from User IDs that not activated in some period + * @returns {Promise} */ -function removeFromInactiveUsers(redis, userId) { - redis.zrem(USERS_INACTIVATED, userId); +async function cleanInactiveUsersIndex() { + const { redis } = this; + const { deleteInactiveAccounts } = this.config; + const expire = Date.now() - (deleteInactiveAccounts * 1000); + await redis.zremrangebyscore(USERS_INACTIVATED, '-inf', expire); } module.exports = { - addToInactiveUsers, - removeFromInactiveUsers, + cleanInactiveUsersIndex, };