(core) Adding new column in users table "ref" with unique identifier.

Summary:
There is a new column in users table called ref (user reference).
It holds user's unique reference number that can be used for features
that require some kind of ownership logic (like comments).

Test Plan: Updated tests

Reviewers: georgegevoian, paulfitz

Reviewed By: georgegevoian, paulfitz

Differential Revision: https://phab.getgrist.com/D3641
This commit is contained in:
Jarosław Sadziński 2022-10-03 16:45:44 +02:00
parent 356090abae
commit 9628253fd8
16 changed files with 189 additions and 33 deletions

View File

@ -146,6 +146,7 @@ export class AccessRules extends Disposable {
{ruleIndex: -1, value: 'user.Origin'}, {ruleIndex: -1, value: 'user.Origin'},
{ruleIndex: -1, value: 'user.SessionID'}, {ruleIndex: -1, value: 'user.SessionID'},
{ruleIndex: -1, value: 'user.IsLoggedIn'}, {ruleIndex: -1, value: 'user.IsLoggedIn'},
{ruleIndex: -1, value: 'user.UserRef'},
]; ];
for (const [i, rule] of rules.entries()) { for (const [i, rule] of rules.entries()) {
const tableId = use(rule.tableId); const tableId = use(rule.tableId);

View File

@ -8,10 +8,11 @@ export interface UserProfile {
loginMethod?: 'Google'|'Email + Password'|'External'; loginMethod?: 'Google'|'Email + Password'|'External';
} }
// User profile including user id. All information in it should // User profile including user id and user ref. All information in it should
// have been validated against database. // have been validated against database.
export interface FullUser extends UserProfile { export interface FullUser extends UserProfile {
id: number; id: number;
ref?: string|null; // Not filled for anonymous users.
allowGoogleLogin?: boolean; // when present, specifies whether logging in via Google is possible. allowGoogleLogin?: boolean; // when present, specifies whether logging in via Google is possible.
} }

View File

@ -1,6 +1,7 @@
import {UserOptions} from 'app/common/UserAPI'; import {UserOptions} from 'app/common/UserAPI';
import {nativeValues} from 'app/gen-server/lib/values'; import {nativeValues} from 'app/gen-server/lib/values';
import {BaseEntity, Column, Entity, JoinTable, ManyToMany, OneToMany, OneToOne, import {makeId} from 'app/server/lib/idUtils';
import {BaseEntity, BeforeInsert, Column, Entity, JoinTable, ManyToMany, OneToMany, OneToOne,
PrimaryGeneratedColumn} from "typeorm"; PrimaryGeneratedColumn} from "typeorm";
import {Group} from "./Group"; import {Group} from "./Group";
@ -50,6 +51,19 @@ export class User extends BaseEntity {
@Column({name: 'connect_id', type: String, nullable: true}) @Column({name: 'connect_id', type: String, nullable: true})
public connectId: string | null; public connectId: string | null;
/**
* Unique reference for this user. Primarily used as an ownership key in a cell metadata (comments).
*/
@Column({name: 'ref', type: String, nullable: false})
public ref: string;
@BeforeInsert()
public async beforeInsert() {
if (!this.ref) {
this.ref = makeId();
}
}
/** /**
* Get user's email. Returns undefined if logins has not been joined, or no login * Get user's email. Returns undefined if logins has not been joined, or no login
* is available * is available

View File

@ -419,6 +419,15 @@ export class HomeDBManager extends EventEmitter {
throw new Error(`Cannot testGetId(${name})`); throw new Error(`Cannot testGetId(${name})`);
} }
/**
* For tests only. Get user's unique reference by name.
*/
public async testGetRef(name: string): Promise<string> {
const user = await User.findOne({where: {name}});
if (user) { return user.ref; }
throw new Error(`Cannot testGetRef(${name})`);
}
/** /**
* Clear all user preferences associated with the given email addresses. * Clear all user preferences associated with the given email addresses.
* For use in tests. * For use in tests.
@ -2537,6 +2546,7 @@ export class HomeDBManager extends EventEmitter {
const login = new Login(); const login = new Login();
login.displayEmail = login.email = ANONYMOUS_USER_EMAIL; login.displayEmail = login.email = ANONYMOUS_USER_EMAIL;
user.logins = [login]; user.logins = [login];
user.ref = '';
return user; return user;
} }
@ -3917,6 +3927,9 @@ export class HomeDBManager extends EventEmitter {
cond = cond.orWhere(`gu3.user_id = ${users}`); cond = cond.orWhere(`gu3.user_id = ${users}`);
// Support the special "everyone" user. // Support the special "everyone" user.
const everyoneId = this._specialUserIds[EVERYONE_EMAIL]; const everyoneId = this._specialUserIds[EVERYONE_EMAIL];
if (everyoneId === undefined) {
throw new Error("Special user id for EVERYONE_EMAIL not found");
}
cond = cond.orWhere(`gu0.user_id = ${everyoneId}`); cond = cond.orWhere(`gu0.user_id = ${everyoneId}`);
cond = cond.orWhere(`gu1.user_id = ${everyoneId}`); cond = cond.orWhere(`gu1.user_id = ${everyoneId}`);
cond = cond.orWhere(`gu2.user_id = ${everyoneId}`); cond = cond.orWhere(`gu2.user_id = ${everyoneId}`);
@ -3925,6 +3938,9 @@ export class HomeDBManager extends EventEmitter {
// Support also the special anonymous user. Currently, by convention, sharing a // Support also the special anonymous user. Currently, by convention, sharing a
// resource with anonymous should make it listable. // resource with anonymous should make it listable.
const anonId = this._specialUserIds[ANONYMOUS_USER_EMAIL]; const anonId = this._specialUserIds[ANONYMOUS_USER_EMAIL];
if (anonId === undefined) {
throw new Error("Special user id for ANONYMOUS_USER_EMAIL not found");
}
cond = cond.orWhere(`gu0.user_id = ${anonId}`); cond = cond.orWhere(`gu0.user_id = ${anonId}`);
cond = cond.orWhere(`gu1.user_id = ${anonId}`); cond = cond.orWhere(`gu1.user_id = ${anonId}`);
cond = cond.orWhere(`gu2.user_id = ${anonId}`); cond = cond.orWhere(`gu2.user_id = ${anonId}`);

View File

@ -0,0 +1,33 @@
import {User} from 'app/gen-server/entity/User';
import {makeId} from 'app/server/lib/idUtils';
import {MigrationInterface, QueryRunner, TableColumn} from "typeorm";
export class UserUUID1663851423064 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<any> {
// Add ref column, for now make it nullable and not unique, we
// first need to put a value in it for all existing users.
await queryRunner.addColumn("users", new TableColumn({
name: "ref",
type: 'varchar',
isNullable: true,
isUnique: false,
}));
// Updating so many rows in a multiple queries is not ideal. We will send updates in chunks.
// 300 seems to be a good number, for 24k rows we have 80 queries.
const userList = await queryRunner.manager.createQueryBuilder()
.select("users")
.from(User, "users")
.getMany();
userList.forEach(u => u.ref = makeId());
await queryRunner.manager.save(userList, { chunk: 300 });
// We are not making this column unique yet, because it can fail
// if there are some old workers still running, and any new user
// is created. We will make it unique in a later migration.
}
public async down(queryRunner: QueryRunner): Promise<any> {
await queryRunner.dropColumn("users", "ref");
}
}

View File

@ -0,0 +1,37 @@
import {User} from 'app/gen-server/entity/User';
import {makeId} from 'app/server/lib/idUtils';
import {MigrationInterface, QueryRunner} from "typeorm";
export class UserRefUnique1664528376930 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
// This is second part of migration 1663851423064-UserUUID, that makes
// the ref column unique.
// Update users that don't have unique ref set.
const userList = await queryRunner.manager.createQueryBuilder()
.select("users")
.from(User, "users")
.where("ref is null")
.getMany();
userList.forEach(u => u.ref = makeId());
await queryRunner.manager.save(userList, {chunk: 300});
// Mark column as unique and non-nullable.
const users = (await queryRunner.getTable('users'))!;
const oldRef = users.findColumnByName('ref')!;
const newRef = oldRef.clone();
newRef.isUnique = true;
newRef.isNullable = false;
await queryRunner.changeColumn('users', oldRef, newRef);
}
public async down(queryRunner: QueryRunner): Promise<void> {
// Mark column as non unique and nullable.
const users = (await queryRunner.getTable('users'))!;
const oldRef = users.findColumnByName('ref')!;
const newRef = oldRef.clone();
newRef.isUnique = false;
newRef.isNullable = true;
await queryRunner.changeColumn('users', oldRef, newRef);
}
}

View File

@ -528,33 +528,46 @@ export interface Authorizer {
getCachedAuth(): DocAuthResult; getCachedAuth(): DocAuthResult;
} }
export interface DocAuthorizerOptions {
dbManager: HomeDBManager;
key: DocAuthKey;
openMode: OpenDocMode;
linkParameters: Record<string, string>;
userRef?: string|null;
docAuth?: DocAuthResult;
profile?: UserProfile;
}
/** /**
* *
* Handle authorization for a single document and user. * Handle authorization for a single document and user.
* *
*/ */
export class DocAuthorizer implements Authorizer { export class DocAuthorizer implements Authorizer {
public readonly openMode: OpenDocMode;
public readonly linkParameters: Record<string, string>;
constructor( constructor(
private _dbManager: HomeDBManager, private _options: DocAuthorizerOptions
private _key: DocAuthKey,
public readonly openMode: OpenDocMode,
public readonly linkParameters: Record<string, string>,
private _docAuth?: DocAuthResult,
private _profile?: UserProfile
) { ) {
this.openMode = _options.openMode;
this.linkParameters = _options.linkParameters;
} }
public getUserId(): number { public getUserId(): number {
return this._key.userId; return this._options.key.userId;
} }
public getUser(): FullUser|null { public getUser(): FullUser|null {
return this._profile ? {id: this.getUserId(), ...this._profile} : null; return this._options.profile ? {
id: this.getUserId(),
ref: this._options.userRef,
...this._options.profile
} : null;
} }
public getDocId(): string { public getDocId(): string {
// We've been careful to require urlId === docId, see DocManager. // We've been careful to require urlId === docId, see DocManager.
return this._key.urlId; return this._options.key.urlId;
} }
public getLinkParameters(): Record<string, string> { public getLinkParameters(): Record<string, string> {
@ -562,18 +575,18 @@ export class DocAuthorizer implements Authorizer {
} }
public async getDoc(): Promise<Document> { public async getDoc(): Promise<Document> {
return this._dbManager.getDoc(this._key); return this._options.dbManager.getDoc(this._options.key);
} }
public async assertAccess(role: 'viewers'|'editors'|'owners'): Promise<void> { public async assertAccess(role: 'viewers'|'editors'|'owners'): Promise<void> {
const docAuth = await this._dbManager.getDocAuthCached(this._key); const docAuth = await this._options.dbManager.getDocAuthCached(this._options.key);
this._docAuth = docAuth; this._options.docAuth = docAuth;
assertAccess(role, docAuth, {openMode: this.openMode}); assertAccess(role, docAuth, {openMode: this.openMode});
} }
public getCachedAuth(): DocAuthResult { public getCachedAuth(): DocAuthResult {
if (!this._docAuth) { throw Error('no cached authentication'); } if (!this._options.docAuth) { throw Error('no cached authentication'); }
return this._docAuth; return this._options.docAuth;
} }
} }

View File

@ -87,6 +87,7 @@ export class Client {
private _profile: UserProfile|null = null; private _profile: UserProfile|null = null;
private _userId: number|null = null; private _userId: number|null = null;
private _userName: string|null = null; private _userName: string|null = null;
private _userRef: string|null = null;
private _firstLoginAt: Date|null = null; private _firstLoginAt: Date|null = null;
private _isAnonymous: boolean = false; private _isAnonymous: boolean = false;
private _nextSeqId: number = 0; // Next sequence-ID for messages sent to the client private _nextSeqId: number = 0; // Next sequence-ID for messages sent to the client
@ -388,25 +389,26 @@ export class Client {
return this._userId; return this._userId;
} }
public getCachedUserRef(): string|null {
return this._userRef;
}
// Returns the userId for profile.email, or null when profile is not set; with caching. // Returns the userId for profile.email, or null when profile is not set; with caching.
public async getUserId(dbManager: HomeDBManager): Promise<number|null> { public async getUserId(dbManager: HomeDBManager): Promise<number|null> {
if (!this._userId) { if (!this._userId) {
if (this._profile) { await this._refreshUser(dbManager);
const user = await this._fetchUser(dbManager);
this._userId = (user && user.id) || null;
this._userName = (user && user.name) || null;
this._isAnonymous = this._userId && dbManager.getAnonymousUserId() === this._userId || false;
this._firstLoginAt = (user && user.firstLoginAt) || null;
} else {
this._userId = dbManager.getAnonymousUserId();
this._userName = 'Anonymous';
this._isAnonymous = true;
this._firstLoginAt = null;
}
} }
return this._userId; return this._userId;
} }
// Returns the userRef for profile.email, or null when profile is not set; with caching.
public async getUserRef(dbManager: HomeDBManager): Promise<string|null> {
if (!this._userRef) {
await this._refreshUser(dbManager);
}
return this._userRef;
}
// Returns the userId for profile.email, or throws 403 error when profile is not set. // Returns the userId for profile.email, or throws 403 error when profile is not set.
public async requireUserId(dbManager: HomeDBManager): Promise<number> { public async requireUserId(dbManager: HomeDBManager): Promise<number> {
const userId = await this.getUserId(dbManager); const userId = await this.getUserId(dbManager);
@ -431,6 +433,22 @@ export class Client {
return meta; return meta;
} }
private async _refreshUser(dbManager: HomeDBManager) {
if (this._profile) {
const user = await this._fetchUser(dbManager);
this._userId = (user && user.id) || null;
this._userName = (user && user.name) || null;
this._isAnonymous = this._userId && dbManager.getAnonymousUserId() === this._userId || false;
this._firstLoginAt = (user && user.firstLoginAt) || null;
this._userRef = user?.ref ?? null;
} else {
this._userId = dbManager.getAnonymousUserId();
this._userName = 'Anonymous';
this._isAnonymous = true;
this._firstLoginAt = null;
}
}
/** /**
* Processes a request from a client. All requests from a client get a response, at least to * Processes a request from a client. All requests from a client get a response, at least to
* indicate success or failure. * indicate success or failure.

View File

@ -261,7 +261,7 @@ export class DocManager extends EventEmitter {
* `doc` - the object with metadata tables. * `doc` - the object with metadata tables.
*/ */
public async openDoc(client: Client, docId: string, public async openDoc(client: Client, docId: string,
mode: OpenDocMode = 'default', openMode: OpenDocMode = 'default',
linkParameters: Record<string, string> = {}): Promise<OpenLocalDocResult> { linkParameters: Record<string, string> = {}): Promise<OpenLocalDocResult> {
let auth: Authorizer; let auth: Authorizer;
const dbManager = this._homeDbManager; const dbManager = this._homeDbManager;
@ -271,6 +271,7 @@ export class DocManager extends EventEmitter {
const org = client.getOrg(); const org = client.getOrg();
if (!org) { throw new Error('Documents can only be opened in the context of a specific organization'); } if (!org) { throw new Error('Documents can only be opened in the context of a specific organization'); }
const userId = await client.getUserId(dbManager) || dbManager.getAnonymousUserId(); const userId = await client.getUserId(dbManager) || dbManager.getAnonymousUserId();
const userRef = await client.getUserRef(dbManager);
// We use docId in the key, and disallow urlId, so we can be sure that we are looking at the // We use docId in the key, and disallow urlId, so we can be sure that we are looking at the
// right doc when we re-query the DB over the life of the websocket. // right doc when we re-query the DB over the life of the websocket.
@ -284,7 +285,15 @@ export class DocManager extends EventEmitter {
// than a docId. // than a docId.
throw new Error(`openDoc expected docId ${docAuth.docId} not urlId ${docId}`); throw new Error(`openDoc expected docId ${docAuth.docId} not urlId ${docId}`);
} }
auth = new DocAuthorizer(dbManager, key, mode, linkParameters, docAuth, client.getProfile() || undefined); auth = new DocAuthorizer({
dbManager,
key,
openMode,
linkParameters,
userRef,
docAuth,
profile: client.getProfile() || undefined
});
} else { } else {
log.debug(`DocManager.openDoc not using authorization for ${docId} because GRIST_SINGLE_USER`); log.debug(`DocManager.openDoc not using authorization for ${docId} because GRIST_SINGLE_USER`);
auth = new DummyAuthorizer('owners', docId); auth = new DummyAuthorizer('owners', docId);
@ -302,7 +311,7 @@ export class DocManager extends EventEmitter {
// If opening in (pre-)fork mode, check if it is appropriate to treat the user as // If opening in (pre-)fork mode, check if it is appropriate to treat the user as
// an owner for granular access purposes. // an owner for granular access purposes.
if (mode === 'fork') { if (openMode === 'fork') {
if (await activeDoc.canForkAsOwner(docSession)) { if (await activeDoc.canForkAsOwner(docSession)) {
// Mark the session specially and flush any cached access // Mark the session specially and flush any cached access
// information. It is easier to make this a property of the // information. It is easier to make this a property of the

View File

@ -122,15 +122,17 @@ export function getDocSessionUser(docSession: OptDocSession): FullUser|null {
const user = getUser(docSession.req); const user = getUser(docSession.req);
const email = user.loginEmail; const email = user.loginEmail;
if (email) { if (email) {
return {id: user.id, name: user.name, email}; return {id: user.id, name: user.name, email, ref: user.ref};
} }
} }
if (docSession.client) { if (docSession.client) {
const id = docSession.client.getCachedUserId(); const id = docSession.client.getCachedUserId();
const ref = docSession.client.getCachedUserRef();
const profile = docSession.client.getProfile(); const profile = docSession.client.getProfile();
if (id && profile) { if (id && profile) {
return { return {
id, id,
ref,
...profile ...profile
}; };
} }

View File

@ -1608,6 +1608,7 @@ export class GranularAccess implements GranularAccessForBundle {
user.Origin = docSession.req?.get('origin') || null; user.Origin = docSession.req?.get('origin') || null;
user.SessionID = isAnonymous ? `a${getDocSessionAltSessionId(docSession)}` : `u${user.UserID}`; user.SessionID = isAnonymous ? `a${getDocSessionAltSessionId(docSession)}` : `u${user.UserID}`;
user.IsLoggedIn = !isAnonymous; user.IsLoggedIn = !isAnonymous;
user.UserRef = fullUser?.ref || null; // Empty string should be treated as null.
if (this._ruler.ruleCollection.ruleError && !this._recoveryMode) { if (this._ruler.ruleCollection.ruleError && !this._recoveryMode) {
// It is important to signal that the doc is in an unexpected state, // It is important to signal that the doc is in an unexpected state,
@ -2600,6 +2601,7 @@ export class User implements UserInfo {
public Origin: string | null = null; public Origin: string | null = null;
public LinkKey: Record<string, string | undefined> = {}; public LinkKey: Record<string, string | undefined> = {};
public Email: string | null = null; public Email: string | null = null;
public UserRef: string | null = null;
[attribute: string]: any; [attribute: string]: any;
constructor(_info: Record<string, unknown> = {}) { constructor(_info: Record<string, unknown> = {}) {

View File

@ -10,6 +10,7 @@ class TestCompletion(test_engine.EngineTestCase):
user = { user = {
'Name': 'Foo', 'Name': 'Foo',
'UserID': 1, 'UserID': 1,
'UserRef': '1',
'StudentInfo': ['Students', 1], 'StudentInfo': ['Students', 1],
'LinkKey': {}, 'LinkKey': {},
'Origin': None, 'Origin': None,
@ -103,6 +104,7 @@ class TestCompletion(test_engine.EngineTestCase):
('user.SessionID', "'u1'"), ('user.SessionID', "'u1'"),
('user.StudentInfo', 'Students[1]'), ('user.StudentInfo', 'Students[1]'),
('user.UserID', '1'), ('user.UserID', '1'),
('user.UserRef', "'1'"),
] ]
) )
# Should follow user attribute references and autocomplete those types. # Should follow user attribute references and autocomplete those types.
@ -130,6 +132,7 @@ class TestCompletion(test_engine.EngineTestCase):
'Email': 'baro@example.com', 'Email': 'baro@example.com',
'LinkKey': {}, 'LinkKey': {},
'UserID': 2, 'UserID': 2,
'UserRef': '2',
'Access': 'owners', 'Access': 'owners',
'SessionID': 'u2', 'SessionID': 'u2',
'IsLoggedIn': True 'IsLoggedIn': True
@ -145,6 +148,7 @@ class TestCompletion(test_engine.EngineTestCase):
('user.Origin', 'None'), ('user.Origin', 'None'),
('user.SessionID', "'u2'"), ('user.SessionID', "'u2'"),
('user.UserID', '2'), ('user.UserID', '2'),
('user.UserRef', "'2'"),
] ]
) )
self.assertEqual( self.assertEqual(

View File

@ -323,6 +323,7 @@ class TestRenames(test_engine.EngineTestCase):
user = { user = {
'Name': 'Foo', 'Name': 'Foo',
'UserID': 1, 'UserID': 1,
'UserRef': '1',
'LinkKey': {}, 'LinkKey': {},
'Origin': None, 'Origin': None,
'Email': 'foo@example.com', 'Email': 'foo@example.com',

View File

@ -565,6 +565,7 @@ class TestTriggerFormulas(test_engine.EngineTestCase):
user1 = { user1 = {
'Name': 'Foo Bar', 'Name': 'Foo Bar',
'UserID': 1, 'UserID': 1,
'UserRef': '1',
'StudentInfo': ['Students', 1], 'StudentInfo': ['Students', 1],
'LinkKey': {}, 'LinkKey': {},
'Origin': None, 'Origin': None,
@ -576,6 +577,7 @@ class TestTriggerFormulas(test_engine.EngineTestCase):
user2 = { user2 = {
'Name': 'Baz Qux', 'Name': 'Baz Qux',
'UserID': 2, 'UserID': 2,
'UserRef': '2',
'StudentInfo': ['Students', 1], 'StudentInfo': ['Students', 1],
'LinkKey': {}, 'LinkKey': {},
'Origin': None, 'Origin': None,

View File

@ -14,6 +14,7 @@ class TestUser(test_engine.EngineTestCase):
'Name': 'Foo Bar', 'Name': 'Foo Bar',
'Email': 'email@example.com', 'Email': 'email@example.com',
'UserID': 1, 'UserID': 1,
'UserRef': '1',
'LinkKey': { 'LinkKey': {
'Param1': 'Param1Value', 'Param1': 'Param1Value',
'Param2': 'Param2Value' 'Param2': 'Param2Value'
@ -42,6 +43,7 @@ class TestUser(test_engine.EngineTestCase):
'Name': None, 'Name': None,
'Email': 'email@getgrist.com', 'Email': 'email@getgrist.com',
'UserID': 1, 'UserID': 1,
'UserRef': '1',
'LinkKey': { 'LinkKey': {
'Param1': 'Param1Value', 'Param1': 'Param1Value',
'Param2': 'Param2Value' 'Param2': 'Param2Value'

View File

@ -13,6 +13,7 @@ the following fields:
- Access: string or None - Access: string or None
- UserID: integer or None - UserID: integer or None
- UserRef: string or None
- Email: string or None - Email: string or None
- Name: string or None - Name: string or None
- Origin: string or None - Origin: string or None
@ -43,7 +44,7 @@ class User(object):
""" """
def __init__(self, data, tables, is_sample=False): def __init__(self, data, tables, is_sample=False):
for attr in ('Access', 'UserID', 'Email', 'Name', 'Origin', 'SessionID', for attr in ('Access', 'UserID', 'Email', 'Name', 'Origin', 'SessionID',
'IsLoggedIn'): 'IsLoggedIn', 'UserRef'):
setattr(self, attr, data[attr]) setattr(self, attr, data[attr])
self.LinkKey = LinkKey(data['LinkKey']) self.LinkKey = LinkKey(data['LinkKey'])