Skip to content

Commit 20bcf6d

Browse files
committed
feat: delete unconfirmed users
1 parent 0c2d22b commit 20bcf6d

File tree

12 files changed

+351
-4
lines changed

12 files changed

+351
-4
lines changed

doc/rfcs/inactive_users_cleanup.md

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
# Inactive user cleanup
2+
3+
## Overview and motivation
4+
Currently `ms-users` service using TTL based cleanup for users who didn't pass activation process. In this case there's a lots of data keys beeing kept in database. That brings additional 'mess' into db data.
5+
6+
In order to provide generally better data handling and clean database structure, I introduce some changes in service logic.
7+
8+
## General defs
9+
- `inactive-users`
10+
Redis database sorted set key. Bound to `USER_ACTIVATE` constant.
11+
Record contains `userId` as value and `timestamp` as score.
12+
- `user-audience`
13+
Redis database hash key. Bound to `USERS_AUDIENCE` constant. Stores `userId` to `audience[string]` mapping, contains any audiences used in user's metadata.
14+
- `deleteInactivated` Redis script, handling all cleanup logic
15+
16+
17+
## Registration and activation hooks
18+
Hook `users:cleanup` executed before any Registration or Activation request. Hook not breaking general logic and not throwing errors, logs error into logfile (IMHO We don't need to show this exception to service consumers). Such execution strategy saves from any inconsistent changes eg: inactive user hit TTL but tried to pass activation process, we shouldn't allow such situations.
19+
20+
## Registration process
21+
When user succeds registration but activation not requested new record added to `inactive-users`. Record contains `userId` and `currenttimestamp`
22+
23+
## Activation process
24+
When user succeds activation `userId` record deleted from `inactive-users`.
25+
26+
## User's MetaData
27+
`updateMetadata` util must be updated to track user audience on any data change. This change brings additinal benefits in further processing user data.
28+
29+
## `users:cleanup` hook
30+
Calls `deleteInactivated` script with TTL parameter from `service.config.deleteInactiveAccounts`
31+
32+
## Remove User process
33+
For correct code deduplication in redis scripts, we need to provide some templating in them. Currently only added logic to remove `audience` and `inactive-users` data.
34+
35+
## Redis Delete Inactive User Delete script
36+
`deleteInactivated.lua`
37+
Delete process requires a lot of data resolved before execution and all database actions must be executed in one shot, to save database consistence. Exports `deleteInactivated` command into namespace and wrapped by `utils/inactive/deleteUsers.js`.
38+
39+
When script started:
40+
1. It extracts from `inactive-user` list of `userId` with score range '-inf' to 'currentimestamp - ttl'.
41+
2. Removes all user data including aliases, emails, metadata, tokens, SSO accounts, throttles and cleans all bound indicies
42+
3. Removes processed records from `inactive-user`
File renamed without changes.
File renamed without changes.

scripts/deleteInactivated.lua

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
local outIndexKey = KEYS[1];
2+
local dataKeyTemplate = KEYS[2];
3+
local metaKeyTemplate = KEYS[3];
4+
local kIndUserTokenTemplate = KEYS[4];
5+
6+
local exTime = ARGV[6];
7+
8+
-- hashes
9+
local kUserAudiences = KEYS[5];
10+
local kAliasToId = KEYS[6];
11+
local kUsernameToId = KEYS[7];
12+
local kSSOToId = KEYS[8];
13+
14+
-- indexes
15+
local kIndUsers = KEYS[9];
16+
local kIndUsersPublic = KEYS[10];
17+
18+
-- throttleprefix
19+
local kThrottlePrefix = KEYS[11];
20+
21+
-- constant fields names
22+
local fUserId = ARGV[1]
23+
local fUserAlias = ARGV[2];
24+
local fUserName = ARGV[3];
25+
26+
local _rawSSOProviders = ARGV[4];
27+
local _rawThrottleActions = ARGV[5];
28+
29+
local throttleActions = cjson.decode(_rawThrottleActions);
30+
local ssoProviders = cjson.decode(_rawSSOProviders);
31+
32+
local delimiter = "!";
33+
34+
local function isempty(s)
35+
return s == nil or s == '' or s == false;
36+
end
37+
38+
local function toTable(data)
39+
local result = {};
40+
for i = 1, #data, 2 do
41+
result[data[i]] = data[i + 1]
42+
end
43+
return result;
44+
end
45+
46+
-- key generators
47+
local function key(...)
48+
return table.concat(arg, delimiter);
49+
end
50+
51+
local function decode(strval)
52+
if type(strval) == "string" then
53+
return cjson.decode(strval);
54+
else
55+
return {}
56+
end
57+
end
58+
59+
local function makeKey (userId, template)
60+
local str = template:gsub('{userid}', userId, 1);
61+
return str
62+
end
63+
64+
local function makeMetaDataKey (userId, audience, template)
65+
local str = template:gsub('{userid}', userId, 1);
66+
str = str:gsub('{audience}', audience, 1);
67+
return str;
68+
end
69+
70+
local function getUserAudiences(userId)
71+
local strVal = redis.call("HGET", kUserAudiences, userId);
72+
return decode(strVal)
73+
end
74+
75+
local function getUserData (userId)
76+
local key = makeKey(userId, dataKeyTemplate);
77+
local data = redis.call("HGETALL", key);
78+
79+
if #data > 0 then
80+
return toTable(data)
81+
end
82+
83+
return nil;
84+
end
85+
86+
-- delete logic
87+
local function deleteUser(userId, userData)
88+
if userData == nil then
89+
return;
90+
end
91+
92+
local alias = userData[fUserAlias];
93+
local username = userData[fUserName];
94+
95+
if isempty(alias) == false then
96+
redis.call("HDEL", kAliasToId, alias, string.lower(alias));
97+
end
98+
99+
--almost impossible but
100+
if isempty(username) == false then
101+
redis.call("HDEL", kUsernameToId, username);
102+
end
103+
104+
for k,v in pairs(ssoProviders) do
105+
local rawData = userData[v];
106+
local provData = decode(rawData);
107+
if isempty(provData['uid']) == false then
108+
redis.call("HDEL", kSSOToId, provData['uid']);
109+
end
110+
end
111+
112+
-- clean indicies
113+
redis.call("HDEL", kIndUsersPublic, userId );
114+
redis.call("HDEL", kIndUsers, userId);
115+
116+
-- delete user data
117+
redis.call("DEL", makeKey(userId, dataKeyTemplate))
118+
119+
-- delete meta
120+
for k,v in pairs(getUserAudiences(userId)) do
121+
local metaKey = makeMetaDataKey(userId, v, metaKeyTemplate);
122+
redis.call("DEL", metaKey);
123+
end
124+
125+
-- delete USERS_TOKENS
126+
local userTokensKey = makeKey(userId, kIndUserTokenTemplate);
127+
redis.call("DEL", userTokensKey);
128+
129+
-- remove audience list
130+
redis.call("HDEL", kUserAudiences, userId);
131+
132+
-- throttle constants
133+
for k,v in pairs(throttleActions) do
134+
local throttleKey = key(kThrottlePrefix, v, userId);
135+
redis.call("DEL", throttleKey);
136+
end
137+
138+
end
139+
140+
local function getInactiveUsers()
141+
return redis.call("ZRANGEBYSCORE", outIndexKey, '-inf', exTime);
142+
end
143+
144+
local function cleanInactiveUsers(idList)
145+
for k,v in pairs(idList) do
146+
redis.call('ZREM', outIndexKey, v);
147+
end
148+
end
149+
150+
-- Command body
151+
local inactiveUsers = getInactiveUsers();
152+
153+
for k,v in pairs(inactiveUsers) do
154+
local userData = getUserData(v);
155+
deleteUser(v, userData);
156+
end
157+
158+
cleanInactiveUsers(inactiveUsers);
159+
-- returns count of deleted users
160+
return { #inactiveUsers }

src/actions/activate.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const jwt = require('../utils/jwt.js');
55
const { getInternalData } = require('../utils/userData');
66
const getMetadata = require('../utils/getMetadata');
77
const handlePipeline = require('../utils/pipelineError.js');
8+
const { removeInactiveUser } = require('../utils/inactive/users');
89
const {
910
USERS_INDEX,
1011
USERS_DATA,
@@ -126,6 +127,8 @@ function activateAccount(data, metadata) {
126127
.persist(userKey)
127128
.sadd(USERS_INDEX, userId);
128129

130+
removeInactiveUser(pipeline, userId);
131+
129132
if (alias) {
130133
pipeline.sadd(USERS_PUBLIC_INDEX, userId);
131134
}
@@ -192,6 +195,8 @@ function activateAction({ params }) {
192195
};
193196

194197
return Promise
198+
.resolve(['users:cleanup'])
199+
.spread(this.hook.bind(this))
195200
.bind(context)
196201
.then(verifyRequest)
197202
.bind(this)

src/actions/register.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ const checkLimits = require('../utils/checkIpLimits');
2222
const challenge = require('../utils/challenges/challenge');
2323
const handlePipeline = require('../utils/pipelineError');
2424
const hashPassword = require('../utils/register/password/hash');
25+
const { addInactiveUser } = require('../utils/inactive/users');
2526
const {
2627
USERS_REF,
2728
USERS_INDEX,
@@ -208,7 +209,7 @@ async function performRegistration({ service, params }) {
208209
pipeline.hset(USERS_USERNAME_TO_ID, username, userId);
209210

210211
if (activate === false && config.deleteInactiveAccounts >= 0) {
211-
pipeline.expire(userDataKey, config.deleteInactiveAccounts);
212+
addInactiveUser(pipeline, userId, audience);
212213
}
213214

214215
await pipeline.exec().then(handlePipeline);
@@ -348,7 +349,11 @@ module.exports = async function registerUser({ params }) {
348349
.disposer(lockDisposer);
349350

350351
return Promise
351-
.using({ service, params }, acquireLock, performRegistration)
352+
.resolve(['users:cleanup'])
353+
.spread(service.hook.bind(service))
354+
.then(() => {
355+
return Promise.using({ service, params }, acquireLock, performRegistration);
356+
})
352357
.catchThrow(LockAcquisitionError, ErrorRaceCondition);
353358
};
354359

src/actions/remove.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ const {
1212
USERS_PUBLIC_INDEX,
1313
USERS_ALIAS_TO_ID,
1414
USERS_SSO_TO_ID,
15+
USERS_ACTIVATE,
16+
USERS_AUDIENCE,
1517
USERS_USERNAME_TO_ID,
1618
USERS_USERNAME_FIELD,
1719
USERS_DATA,
@@ -92,6 +94,10 @@ async function removeUser({ params }) {
9294
transaction.srem(USERS_PUBLIC_INDEX, userId);
9395
transaction.srem(USERS_INDEX, userId);
9496

97+
// clean audiences and delete from inactive list
98+
transaction.zrem(USERS_ACTIVATE, userId);
99+
transaction.hdel(USERS_AUDIENCE, userId);
100+
95101
// remove metadata & internal data
96102
transaction.del(key(userId, USERS_DATA));
97103
transaction.del(key(userId, USERS_METADATA, audience));

src/constants.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ module.exports = exports = {
66
USERS_PUBLIC_INDEX: 'users-public',
77
USERS_REFERRAL_INDEX: 'users-referral',
88
ORGANIZATIONS_INDEX: 'organization-iterator-set',
9+
// inactive user id's list
10+
USERS_ACTIVATE: 'users-activate',
11+
// handles json list of audiences
12+
USERS_AUDIENCE: 'users-audience',
913
// id mapping
1014
USERS_ALIAS_TO_ID: 'users-alias',
1115
USERS_SSO_TO_ID: 'users-sso-hash',
@@ -108,6 +112,7 @@ module.exports = exports = {
108112
// lock names
109113
lockAlias: alias => `users:alias:${alias}`,
110114
lockRegister: username => `users:register:${username}`,
115+
lockActivate: username => `users:activate:${username}`,
111116
lockOrganization: organizationName => `organizations:create:${organizationName}`,
112117
};
113118

src/users.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ module.exports = class Users extends Microfleet {
7676
// init token manager
7777
const tokenManagerOpts = { backend: { connection: redis } };
7878
this.tokenManager = new TokenManager(merge({}, config.tokenManager, tokenManagerOpts));
79+
this.on('users:cleanup', require('./utils/inactive/users').cleanUsers);
7980
});
8081

8182
this.on('plugin:start:http', (server) => {
@@ -96,6 +97,7 @@ module.exports = class Users extends Microfleet {
9697
this.on(`plugin:close:${this.redisType}`, () => {
9798
this.dlock = null;
9899
this.tokenManager = null;
100+
this.removeListener('users:cleanup');
99101
});
100102

101103
// add migration connector

src/utils/inactive/delete.js

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
const key = require('../key');
2+
3+
const {
4+
USERS_ACTIVATE,
5+
USERS_AUDIENCE,
6+
USERS_ALIAS_TO_ID,
7+
USERS_SSO_TO_ID,
8+
USERS_DATA,
9+
USERS_METADATA,
10+
USERS_TOKENS,
11+
USERS_INDEX,
12+
USERS_PUBLIC_INDEX,
13+
USERS_ID_FIELD,
14+
USERS_ALIAS_FIELD,
15+
USERS_USERNAME_TO_ID,
16+
USERS_USERNAME_FIELD,
17+
SSO_PROVIDERS,
18+
THROTTLE_PREFIX,
19+
USERS_ACTION_ACTIVATE,
20+
USERS_ACTION_REGISTER,
21+
USERS_ACTION_PASSWORD,
22+
USERS_ACTION_RESET,
23+
} = require('../../constants');
24+
25+
const encode = obj => JSON.stringify(obj);
26+
27+
/**
28+
* Clean all users, who did't pass activation
29+
* see scripts/deleteInactivated.lua
30+
*/
31+
async function deleteInactiveUsers(redis, ttl) {
32+
const userDataTmpl = key('{userid}', USERS_DATA);
33+
const metaDataTmpl = key('{userid}', USERS_METADATA, '{audience}');
34+
const tokenTmpl = key('{userid}', USERS_TOKENS);
35+
const ssoProviders = encode(SSO_PROVIDERS);
36+
const throttleActions = encode([USERS_ACTION_ACTIVATE, USERS_ACTION_PASSWORD, USERS_ACTION_RESET, USERS_ACTION_REGISTER]);
37+
38+
const expire = Date.now() - (ttl * 1000);
39+
40+
// not pretty but....
41+
// Leaving binding to THROTTLE_PREFIX, but this constants absent.
42+
const cmd = redis.deleteInactivated(11,
43+
USERS_ACTIVATE, userDataTmpl, metaDataTmpl, tokenTmpl, USERS_AUDIENCE,
44+
USERS_ALIAS_TO_ID, USERS_USERNAME_TO_ID, USERS_SSO_TO_ID, USERS_INDEX, USERS_PUBLIC_INDEX,
45+
THROTTLE_PREFIX,
46+
USERS_ID_FIELD, USERS_ALIAS_FIELD, USERS_USERNAME_FIELD,
47+
ssoProviders, throttleActions, expire);
48+
49+
return cmd;
50+
}
51+
52+
module.exports = deleteInactiveUsers;

0 commit comments

Comments
 (0)