Page MenuHomePhabricator
Differential D7298

[Identity] Handle key payload
ClosedPublic
Actions

Authored by • jon on Apr 4 2023, 8:02 AM.
Tags
None
Referenced Files
F6294447: D7298.id24598.diff
Sat, Apr 26, 3:25 AM
F6273097: D7298.id24864.diff
Fri, Apr 25, 4:20 PM
Unknown Object (File)
Wed, Apr 23, 2:57 PM
Unknown Object (File)
Wed, Apr 23, 2:56 PM
Unknown Object (File)
Wed, Apr 23, 12:00 PM
Unknown Object (File)
Wed, Apr 23, 8:59 AM
Unknown Object (File)
Wed, Apr 23, 4:54 AM
Unknown Object (File)
Wed, Apr 23, 3:32 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
varun
bartek
Commits
rCOMM68bbaa297a06: [Identity] Handle key payload
Summary

Allow for the key payload to be serialized and deserialized.

This is needed later for verifying that the payload is correctly
structured and a device is also identified by the identity public key.

Test Plan
cd services/identity
cargo test

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jon created this revision.Apr 4 2023, 8:02 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptApr 4 2023, 8:02 AM
• jon updated this revision to Diff 24599.Apr 4 2023, 8:08 AM

Clean up accidental additions

Harbormaster completed remote builds in B17962: Diff 24599.Apr 4 2023, 8:25 AM
• jon requested review of this revision.Apr 4 2023, 8:25 AM
Harbormaster completed remote builds in B17961: Diff 24598.Apr 4 2023, 8:33 AM
varun requested changes to this revision.Apr 5 2023, 11:22 AM
varun added inline comments.
services/identity/src/database.rs
40–46
49

DDB complains if we don't escape the payload?

51

How about

impl FromStr for KeyPayload {
  type Err = serde_json::Error;

  fn from_str(s: &str) -> Result<Self, Self::Err> {
    serde_json::from_str(&s.replace(r#"\""#, r#"""#))
  }
}
This revision now requires changes to proceed.Apr 5 2023, 11:22 AM
• jon updated this revision to Diff 24725.Apr 6 2023, 7:22 AM
• jon marked 3 inline comments as done.

Address feedback

services/identity/src/database.rs
49

no, that's just what is currently in DDB. I'm assuming how we create the payload on the JS side is creating an escaped payload

51

sure

Harbormaster completed remote builds in B18044: Diff 24725.Apr 6 2023, 7:39 AM
varun accepted this revision.Apr 6 2023, 12:32 PM
varun added inline comments.
services/identity/src/database.rs
52 ↗(On Diff #24725)
57–59 ↗(On Diff #24725)

we're intentionally leaving this commented? we probably won't ever need this method since the payload is created on the client, right?

This revision is now accepted and ready to land.Apr 6 2023, 12:32 PM
• jon marked 2 inline comments as done.Apr 7 2023, 1:31 PM
• jon added inline comments.
services/identity/src/database.rs
57–59 ↗(On Diff #24725)

no, looks like I forgot about this.

we probably won't ever need this method since the payload is created on the client, right?

Did it because of it felt "right" for serialize/deserialize, but yea, this should be pretty much immutable, so there's no need to "serialize" it.

• jon updated this revision to Diff 24841.Apr 7 2023, 1:31 PM
• jon marked an inline comment as done.

Address feedback

• jon added a comment.Apr 7 2023, 1:32 PM

Address feedback

• jon added a child revision: D7299: [Identity] Support keygen and server setup for opaque 2.0.Apr 7 2023, 1:33 PM
Harbormaster completed remote builds in B18139: Diff 24841.Apr 7 2023, 1:45 PM
• jon updated this revision to Diff 24843.Apr 7 2023, 2:18 PM

Rebase on master

Harbormaster completed remote builds in B18141: Diff 24843.Apr 7 2023, 2:34 PM
Closed by commit rCOMM68bbaa297a06: [Identity] Handle key payload (authored by • jon). · Explain WhyApr 7 2023, 2:53 PM
This revision was automatically updated to reflect the committed changes.
• jon added a commit: rCOMM68bbaa297a06: [Identity] Handle key payload.

Revision Contents
Changeset List

PathSize
services/
identity/
2 lines
src/
54 lines

Diff 24599

View Options

services/identity/Cargo.lock

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js'; import { dbQuery, SQL } from '../database/database.js';
import { import {
fetchThreadInfos, fetchThreadInfos,
fetchServerThreadInfos, fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos, rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js'; } from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js'; import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js'; import type { Viewer } from '../session/viewer.js';
import { updateRole } from '../updaters/thread-updaters.js';
type InitialRoles = { type InitialRoles = {
+default: RoleInfo, +default: RoleInfo,
+creator: RoleInfo, +creator: RoleInfo,
}; };
async function createInitialRolesForNewThread( async function createInitialRolesForNewThread(
threadID: string, threadID: string,
threadType: ThreadType, threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines const hasPermission = await checkThreadPermission(
viewer, viewer,
request.community, request.community,
threadPermissions.CHANGE_ROLE, threadPermissions.CHANGE_ROLE,
); );
if (!hasPermission) { if (!hasPermission) {
throw new ServerError('invalid_credentials'); throw new ServerError('invalid_credentials');
} }
const { community, name, permissions, action } = request; const { community, existingRoleID, name, permissions, action } = request;
for (const permission of permissions) { for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) { if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters'); throw new ServerError('invalid_parameters');
} }
} }
const [id] = await createIDs('roles', 1); const [id] = await createIDs('roles', 1);
Show All 31 Lines): Promise<RoleModificationResult> {
let query = SQL``; let query = SQL``;
if (action === 'create_role') { if (action === 'create_role') {
query = SQL` query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time) INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row}) VALUES (${row})
`; `;
} else if (action === 'edit_role') { } else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet"); query = SQL`
UPDATE roles
SET name = ${name}, permissions = ${permissionsBlob}
WHERE id = ${existingRoleID}
`;
} }
await dbQuery(query); await dbQuery(query);
// The `updateRole` needs to occur after the role has been updated
// in the database because it will want the most up to date role info
// (permissions / name)
if (action === 'edit_role') {
atulUnsubmitted
Not Done
Inline Actions

Instead of two separate (action === 'edit_role') blocks, could we merge the two?

We could move await dbQuery(query); within both the (action === 'create_role') (line 139) and else if (action === 'edit_role') (line 144) blocks?

Not a huge deal, whatever you prefer

atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah we can, that should be cleaner too

rohan: Yeah we can, that should be cleaner too
if (!existingRoleID) {
throw new ServerError('invalid_parameters');
}
const membersWithRole = threadInfo.members.filter(
memberInfo => memberInfo.role === existingRoleID,
);
await updateRole(
viewer,
{
threadID: community,
role: existingRoleID,
memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
atulUnsubmitted
Not Done
Inline Actions

Should we add this .map to the end of the .filter on lines 162-164?

atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
rohanAuthorUnsubmitted
Done
Inline Actions

Sure that makes sense

rohan: Sure that makes sense
},
{ silenceNewMessages: true },
);
}
const fetchServerThreadInfosResult = await fetchServerThreadInfos({ const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community, threadID: community,
}); });
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult; const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community]; const serverThreadInfo = serverThreadInfos[community];
if (!serverThreadInfo) { if (!serverThreadInfo) {
throw new ServerError('internal_error'); throw new ServerError('internal_error');
Show All 32 Lines
View Options

services/identity/Cargo.toml

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js'; import { dbQuery, SQL } from '../database/database.js';
import { import {
fetchThreadInfos, fetchThreadInfos,
fetchServerThreadInfos, fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos, rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js'; } from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js'; import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js'; import type { Viewer } from '../session/viewer.js';
import { updateRole } from '../updaters/thread-updaters.js';
type InitialRoles = { type InitialRoles = {
+default: RoleInfo, +default: RoleInfo,
+creator: RoleInfo, +creator: RoleInfo,
}; };
async function createInitialRolesForNewThread( async function createInitialRolesForNewThread(
threadID: string, threadID: string,
threadType: ThreadType, threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines const hasPermission = await checkThreadPermission(
viewer, viewer,
request.community, request.community,
threadPermissions.CHANGE_ROLE, threadPermissions.CHANGE_ROLE,
); );
if (!hasPermission) { if (!hasPermission) {
throw new ServerError('invalid_credentials'); throw new ServerError('invalid_credentials');
} }
const { community, name, permissions, action } = request; const { community, existingRoleID, name, permissions, action } = request;
for (const permission of permissions) { for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) { if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters'); throw new ServerError('invalid_parameters');
} }
} }
const [id] = await createIDs('roles', 1); const [id] = await createIDs('roles', 1);
Show All 31 Lines): Promise<RoleModificationResult> {
let query = SQL``; let query = SQL``;
if (action === 'create_role') { if (action === 'create_role') {
query = SQL` query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time) INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row}) VALUES (${row})
`; `;
} else if (action === 'edit_role') { } else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet"); query = SQL`
UPDATE roles
SET name = ${name}, permissions = ${permissionsBlob}
WHERE id = ${existingRoleID}
`;
} }
await dbQuery(query); await dbQuery(query);
// The `updateRole` needs to occur after the role has been updated
// in the database because it will want the most up to date role info
// (permissions / name)
if (action === 'edit_role') {
atulUnsubmitted
Not Done
Inline Actions

Instead of two separate (action === 'edit_role') blocks, could we merge the two?

We could move await dbQuery(query); within both the (action === 'create_role') (line 139) and else if (action === 'edit_role') (line 144) blocks?

Not a huge deal, whatever you prefer

atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah we can, that should be cleaner too

rohan: Yeah we can, that should be cleaner too
if (!existingRoleID) {
throw new ServerError('invalid_parameters');
}
const membersWithRole = threadInfo.members.filter(
memberInfo => memberInfo.role === existingRoleID,
);
await updateRole(
viewer,
{
threadID: community,
role: existingRoleID,
memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
atulUnsubmitted
Not Done
Inline Actions

Should we add this .map to the end of the .filter on lines 162-164?

atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
rohanAuthorUnsubmitted
Done
Inline Actions

Sure that makes sense

rohan: Sure that makes sense
},
{ silenceNewMessages: true },
);
}
const fetchServerThreadInfosResult = await fetchServerThreadInfos({ const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community, threadID: community,
}); });
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult; const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community]; const serverThreadInfo = serverThreadInfos[community];
if (!serverThreadInfo) { if (!serverThreadInfo) {
throw new ServerError('internal_error'); throw new ServerError('internal_error');
Show All 32 Lines
View Options

services/identity/src/database.rs

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js'; import { dbQuery, SQL } from '../database/database.js';
import { import {
fetchThreadInfos, fetchThreadInfos,
fetchServerThreadInfos, fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos, rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js'; } from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js'; import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js'; import type { Viewer } from '../session/viewer.js';
import { updateRole } from '../updaters/thread-updaters.js';
type InitialRoles = { type InitialRoles = {
+default: RoleInfo, +default: RoleInfo,
+creator: RoleInfo, +creator: RoleInfo,
}; };
async function createInitialRolesForNewThread( async function createInitialRolesForNewThread(
threadID: string, threadID: string,
threadType: ThreadType, threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines const hasPermission = await checkThreadPermission(
viewer, viewer,
request.community, request.community,
threadPermissions.CHANGE_ROLE, threadPermissions.CHANGE_ROLE,
); );
if (!hasPermission) { if (!hasPermission) {
throw new ServerError('invalid_credentials'); throw new ServerError('invalid_credentials');
} }
const { community, name, permissions, action } = request; const { community, existingRoleID, name, permissions, action } = request;
for (const permission of permissions) { for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) { if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters'); throw new ServerError('invalid_parameters');
} }
} }
const [id] = await createIDs('roles', 1); const [id] = await createIDs('roles', 1);
Show All 31 Lines): Promise<RoleModificationResult> {
let query = SQL``; let query = SQL``;
if (action === 'create_role') { if (action === 'create_role') {
query = SQL` query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time) INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row}) VALUES (${row})
`; `;
} else if (action === 'edit_role') { } else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet"); query = SQL`
UPDATE roles
SET name = ${name}, permissions = ${permissionsBlob}
WHERE id = ${existingRoleID}
`;
} }
await dbQuery(query); await dbQuery(query);
// The `updateRole` needs to occur after the role has been updated
// in the database because it will want the most up to date role info
// (permissions / name)
if (action === 'edit_role') {
atulUnsubmitted
Not Done
Inline Actions

Instead of two separate (action === 'edit_role') blocks, could we merge the two?

We could move await dbQuery(query); within both the (action === 'create_role') (line 139) and else if (action === 'edit_role') (line 144) blocks?

Not a huge deal, whatever you prefer

atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah we can, that should be cleaner too

rohan: Yeah we can, that should be cleaner too
if (!existingRoleID) {
throw new ServerError('invalid_parameters');
}
const membersWithRole = threadInfo.members.filter(
memberInfo => memberInfo.role === existingRoleID,
);
await updateRole(
viewer,
{
threadID: community,
role: existingRoleID,
memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
atulUnsubmitted
Not Done
Inline Actions

Should we add this .map to the end of the .filter on lines 162-164?

atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
rohanAuthorUnsubmitted
Done
Inline Actions

Sure that makes sense

rohan: Sure that makes sense
},
{ silenceNewMessages: true },
);
}
const fetchServerThreadInfosResult = await fetchServerThreadInfos({ const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community, threadID: community,
}); });
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult; const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community]; const serverThreadInfo = serverThreadInfos[community];
if (!serverThreadInfo) { if (!serverThreadInfo) {
throw new ServerError('internal_error'); throw new ServerError('internal_error');
Show All 32 Lines