Skip to content

Commit

Permalink
(core) updates from grist-core
Browse files Browse the repository at this point in the history
  • Loading branch information
paulfitz committed Oct 24, 2024
2 parents cbbfa51 + c945fac commit 6c278d1
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 47 deletions.
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,6 @@ xunit.xml
/docker-compose-examples/*/persist
/docker-compose-examples/*/secrets
/docker-compose-examples/grist-traefik-oidc-auth/.env

# Sample grist documents
/samples/
3 changes: 2 additions & 1 deletion app/gen-server/entity/Login.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {BaseEntity, Column, Entity, JoinColumn, ManyToOne, PrimaryColumn} from "typeorm";
import {BaseEntity, Column, Entity, Index, JoinColumn, ManyToOne, PrimaryColumn} from "typeorm";

import {User} from "./User";

Expand All @@ -9,6 +9,7 @@ export class Login extends BaseEntity {
public id: number;

// This is the normalized email address we use for equality and indexing.
@Index()
@Column({type: String})
public email: string;

Expand Down
50 changes: 27 additions & 23 deletions app/gen-server/lib/homedb/HomeDBManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2016,30 +2016,28 @@ export class HomeDBManager extends EventEmitter {
const result = await this._connection.transaction(async manager => {
const analysis = await this._usersManager.verifyAndLookupDeltaEmails(userId, delta, false, manager);
let {userIdDelta} = analysis;
let wsQuery = this._workspace(scope, wsId, {
const options = {
manager,
markPermissions: analysis.permissionThreshold,
})
// Join the workspace's ACL rules and groups/users so we can edit them.
.leftJoinAndSelect('workspaces.aclRules', 'acl_rules')
.leftJoinAndSelect('acl_rules.group', 'workspace_groups')
.leftJoinAndSelect('workspace_groups.memberUsers', 'workspace_users')
// Join the workspace's org and org member groups so we know what should be inherited.
.leftJoinAndSelect('workspaces.org', 'org')
.leftJoinAndSelect('org.aclRules', 'org_acl_rules')
.leftJoinAndSelect('org_acl_rules.group', 'org_groups')
.leftJoinAndSelect('org_groups.memberUsers', 'org_users');
wsQuery = this._addFeatures(wsQuery, 'org');
};
let wsQuery = this._buildWorkspaceWithACLRules(scope, wsId, options);
wsQuery = this._withAccess(wsQuery, userId, 'workspaces');
const queryResult = await verifyEntity(wsQuery);
if (queryResult.status !== 200) {
const wsQueryResult = await verifyEntity(wsQuery);

if (wsQueryResult.status !== 200) {
// If the query for the workspace failed, return the failure result.
return queryResult;
return wsQueryResult;
}
this._failIfPowerfulAndChangingSelf(analysis, queryResult);
const ws: Workspace = queryResult.data;
this._failIfPowerfulAndChangingSelf(analysis, wsQueryResult);
const ws: Workspace = wsQueryResult.data;

const orgId = ws.org.id;
let orgQuery = this._buildOrgWithACLRulesQuery(scope, orgId, options);
orgQuery = this._addFeatures(orgQuery);
const orgQueryResult = await orgQuery.getRawAndEntities();
const org: Organization = orgQueryResult.entities[0];
// Get all the non-guest groups on the org.
const orgGroups = getNonGuestGroups(ws.org);
const orgGroups = getNonGuestGroups(org);
// Get all the non-guest groups to be updated by the delta.
const groups = getNonGuestGroups(ws);
if ('maxInheritedRole' in delta) {
Expand All @@ -2062,7 +2060,7 @@ export class HomeDBManager extends EventEmitter {
await this._updateUserPermissions(groups, userIdDelta, manager);
this._checkUserChangeAllowed(userId, groups);
const nonOrgMembersAfter = this._usersManager.getUserDifference(groups, orgGroups);
const features = ws.org.billingAccount.getFeatures();
const features = org.billingAccount.getFeatures();
const limit = features.maxSharesPerWorkspace;
if (limit !== undefined) {
this._restrictShares(null, limit, removeRole(nonOrgMembersBefore),
Expand All @@ -2072,7 +2070,7 @@ export class HomeDBManager extends EventEmitter {
await manager.save(groups);
// If the users in workspace were changed, make a call to repair the guests in the org.
if (userIdDelta) {
await this._repairOrgGuests(scope, ws.org.id, manager);
await this._repairOrgGuests(scope, orgId, manager);
notifications.push(this._inviteNotification(userId, ws, userIdDelta, membersBefore));
}
return {
Expand Down Expand Up @@ -4652,9 +4650,8 @@ export class HomeDBManager extends EventEmitter {
return { personal: true, public: !realAccess };
}

private _getWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial<QueryOptions> = {}) {
const query = this._workspace(scope, wsId, {
markPermissions: Permissions.VIEW,
private _buildWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial<QueryOptions> = {}) {
return this._workspace(scope, wsId, {
...options
})
// Join the workspace's ACL rules (with 1st level groups/users listed).
Expand All @@ -4664,6 +4661,13 @@ export class HomeDBManager extends EventEmitter {
.leftJoinAndSelect('workspace_groups.memberGroups', 'workspace_group_groups')
.leftJoinAndSelect('workspace_group_users.logins', 'workspace_user_logins')
.leftJoinAndSelect('workspaces.org', 'org');
}

private _getWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial<QueryOptions> = {}) {
const query = this._buildWorkspaceWithACLRules(scope, wsId, {
markPermissions: Permissions.VIEW,
...options
});
return verifyEntity(query);
}

Expand Down
58 changes: 46 additions & 12 deletions app/gen-server/lib/homedb/UsersManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,15 +337,21 @@ export class UsersManager {
email: string,
manager?: EntityManager
): Promise<User|undefined> {
const normalizedEmail = normalizeEmail(email);
return await (manager || this._connection).createQueryBuilder()
.select('user')
.from(User, 'user')
.leftJoinAndSelect('user.logins', 'logins')
.where('email = :email', {email: normalizedEmail})
return await this._buildExistingUsersByLoginRequest([email], manager)
.getOne() || undefined;
}

/**
* Find some users by their emails. Don't create the users if they don't already exist.
*/
public async getExistingUsersByLogin(
emails: string[],
manager?: EntityManager
): Promise<User[]> {
return await this._buildExistingUsersByLoginRequest(emails, manager)
.getMany();
}

/**
*
* Fetches a user record based on an email address. If a user record already
Expand Down Expand Up @@ -471,7 +477,18 @@ export class UsersManager {
});
}

/**
/*
* This function is an alias of getUserByLogin
* Its purpose is to be more expressive and avoid confusion when reading code.
* FIXME :In the future it may be used to split getUserByLogin in two distinct functions
* One for creation
* the other for retrieving users in order to make it more maintainable
*/
public async createUser(email: string, options: GetUserOptions = {}): Promise<User> {
return await this.getUserByLogin(email, options);
}

/*
* Deletes a user from the database. For the moment, the only person with the right
* to delete a user is the user themselves.
* Users have logins, a personal org, and entries in the group_users table. All are
Expand Down Expand Up @@ -612,11 +629,16 @@ export class UsersManager {
// Lookup emails
const emailMap = delta.users;
const emails = Object.keys(emailMap);
const emailUsers = await Promise.all(
emails.map(async email => await this.getUserByLogin(email, {manager: transaction}))
);
emails.forEach((email, i) => {
const userIdAffected = emailUsers[i]!.id;
const existingUsers = await this.getExistingUsersByLogin(emails, transaction);
const emailsExistingUsers = existingUsers.map(user => user.loginEmail);
const emailsUsersToCreate = emails.filter(email => !emailsExistingUsers.includes(email));
const emailUsers = new Map(existingUsers.map(user => [user.loginEmail, user]));
for (const email of emailsUsersToCreate) {
const user = await this.createUser(email, {manager: transaction});
emailUsers.set(user.loginEmail, user);
}
emails.forEach((email) => {
const userIdAffected = emailUsers.get(normalizeEmail(email))!.id;
// Org-level sharing with everyone would allow serious spamming - forbid it.
if (emailMap[email] !== null && // allow removing anything
userId !== this.getSupportUserId() && // allow support user latitude
Expand Down Expand Up @@ -766,4 +788,16 @@ export class UsersManager {
}
delta.users = users;
}

private _buildExistingUsersByLoginRequest(
emails: string[],
manager?: EntityManager
) {
const normalizedEmails = emails.map(email=> normalizeEmail(email));
return (manager || this._connection).createQueryBuilder()
.select('user')
.from(User, 'user')
.leftJoinAndSelect('user.logins', 'logins')
.where('email IN (:...emails)', {emails: normalizedEmails});
}
}
16 changes: 16 additions & 0 deletions app/gen-server/migration/1729754662550-LoginsEmailIndex.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import {MigrationInterface, QueryRunner, TableIndex} from "typeorm";

export class LoginsEmailsIndex1729754662550 implements MigrationInterface {

public async up(queryRunner: QueryRunner): Promise<any> {
await queryRunner.createIndex("logins", new TableIndex({
name: "logins__email",
columnNames: ["email"]
}));
}

public async down(queryRunner: QueryRunner): Promise<any> {
await queryRunner.dropIndex("logins", "logins__email");
}
}

10 changes: 7 additions & 3 deletions app/server/lib/OIDCConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ import { SendAppPageFunction } from 'app/server/lib/sendAppPage';
import { StringUnionError } from 'app/common/StringUnion';
import { EnabledProtection, EnabledProtectionString, ProtectionsManager } from './oidc/Protections';
import { SessionObj } from './BrowserSession';
import { getOriginUrl } from './requestUtils';

const CALLBACK_URL = '/oauth2/callback';

Expand Down Expand Up @@ -278,18 +279,21 @@ export class OIDCConfig {
});
}

public async getLogoutRedirectUrl(req: express.Request, redirectUrl: URL): Promise<string> {
public async getLogoutRedirectUrl(req: express.Request): Promise<string> {
const session: SessionObj|undefined = (req as RequestWithLogin).session;
const stableRedirectUri = new URL('/signed-out', getOriginUrl(req)).href;
// For IdPs that don't have end_session_endpoint, we just redirect to the logout page.
if (this._skipEndSessionEndpoint) {
return redirectUrl.href;
// Ignore redirectUrl because OIDC providers don't allow variable redirect URIs
return stableRedirectUri;
}
// Alternatively, we could use a logout URL specified by configuration.
if (this._endSessionEndpoint) {
return this._endSessionEndpoint;
}
return this._client.endSessionUrl({
post_logout_redirect_uri: redirectUrl.href,
// Ignore redirectUrl because OIDC providers don't allow variable redirect URIs
post_logout_redirect_uri: stableRedirectUri,
id_token_hint: session?.oidc?.idToken,
});
}
Expand Down
7 changes: 5 additions & 2 deletions static/locales/zh_Hans.client.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"Remove column {{- colId }} from {{- tableId }} rules": "从 {{- tableId }} 规则中删除列 {{- colId }}",
"When adding table rules, automatically add a rule to grant OWNER full access.": "添加表规则时,自动添加规则以授予 OWNER 完全访问权限。",
"Allow editors to edit structure (e.g. modify and delete tables, columns, layouts), and to write formulas, which give access to all data regardless of read restrictions.": "允许编辑者编辑结构(例如修改和删除表、列、布局)和编写公式,无论读取限制如何,都可以访问所有数据。",
"This default should be changed if editors' access is to be limited. ": "如果要限制编辑者的访问权限,则应更改此默认值。 "
"This default should be changed if editors' access is to be limited. ": "如果要限制编辑者的访问权限,则应更改此默认值。 ",
"Add Table-wide Rule": "添加全表规则"
},
"AccountPage": {
"API": "API",
Expand Down Expand Up @@ -212,7 +213,9 @@
"Legacy": "遗产",
"Personal Site": "个人网站",
"Team Site": "团队网站",
"Grist Templates": "Grist 模板"
"Grist Templates": "Grist 模板",
"Billing Account": "计费账户",
"Manage Team": "管理团队"
},
"AppModel": {
"This team site is suspended. Documents can be read, but not modified.": "此团队网站已挂起。可以读取文档,但不能修改文档。"
Expand Down
4 changes: 3 additions & 1 deletion test/gen-server/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ import {UserLastConnection1713186031023
import {ActivationEnabled1722529827161
as ActivationEnabled} from 'app/gen-server/migration/1722529827161-Activation-Enabled';
import {Configs1727747249153 as Configs} from 'app/gen-server/migration/1727747249153-Configs';
import {LoginsEmailsIndex1729754662550
as LoginsEmailsIndex} from 'app/gen-server/migration/1729754662550-LoginsEmailIndex';

const home: HomeDBManager = new HomeDBManager();

Expand All @@ -56,7 +58,7 @@ const migrations = [Initial, Login, PinDocs, UserPicture, DisplayEmail, DisplayE
ExternalBilling, DocOptions, Secret, UserOptions, GracePeriodStart,
DocumentUsage, Activations, UserConnectId, UserUUID, UserUniqueRefUUID,
Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, BillingFeatures,
UserLastConnection, ActivationEnabled, Configs];
UserLastConnection, ActivationEnabled, Configs, LoginsEmailsIndex];

// Assert that the "members" acl rule and group exist (or not).
function assertMembersGroup(org: Organization, exists: boolean) {
Expand Down
13 changes: 8 additions & 5 deletions test/server/lib/OIDCConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ describe('OIDCConfig', () => {
});

describe('getLogoutRedirectUrl', () => {
const REDIRECT_URL = new URL('http://localhost:8484/docs/signed-out');
const STABLE_LOGOUT_URL = new URL('http://localhost:8484/signed-out');
const URL_RETURNED_BY_CLIENT = 'http://localhost:8484/logout_url_from_issuer';
const ENV_VALUE_GRIST_OIDC_IDP_END_SESSION_ENDPOINT = 'http://localhost:8484/logout';
const FAKE_SESSION = {
Expand All @@ -767,7 +767,7 @@ describe('OIDCConfig', () => {
env: {
GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT: 'true',
},
expectedUrl: REDIRECT_URL.href,
expectedUrl: STABLE_LOGOUT_URL.href,
}, {
itMsg: 'should use the GRIST_OIDC_IDP_END_SESSION_ENDPOINT when it is set',
env: {
Expand All @@ -778,14 +778,14 @@ describe('OIDCConfig', () => {
itMsg: 'should call the end session endpoint with the expected parameters',
expectedUrl: URL_RETURNED_BY_CLIENT,
expectedLogoutParams: {
post_logout_redirect_uri: REDIRECT_URL.href,
post_logout_redirect_uri: STABLE_LOGOUT_URL.href,
id_token_hint: FAKE_SESSION.oidc!.idToken,
}
}, {
itMsg: 'should call the end session endpoint with no idToken if session is missing',
expectedUrl: URL_RETURNED_BY_CLIENT,
expectedLogoutParams: {
post_logout_redirect_uri: REDIRECT_URL.href,
post_logout_redirect_uri: STABLE_LOGOUT_URL.href,
id_token_hint: undefined,
},
session: null
Expand All @@ -798,9 +798,12 @@ describe('OIDCConfig', () => {
clientStub.endSessionUrl.returns(URL_RETURNED_BY_CLIENT);
const config = await OIDCConfigStubbed.buildWithStub(clientStub.asClient());
const req = {
headers: {
host: STABLE_LOGOUT_URL.host
},
session: 'session' in ctx ? ctx.session : FAKE_SESSION
} as unknown as RequestWithLogin;
const url = await config.getLogoutRedirectUrl(req, REDIRECT_URL);
const url = await config.getLogoutRedirectUrl(req);
assert.equal(url, ctx.expectedUrl);
if (ctx.expectedLogoutParams) {
assert.isTrue(clientStub.endSessionUrl.calledOnce);
Expand Down

0 comments on commit 6c278d1

Please sign in to comment.