Add members on group create #72

giarc3 · 2019-11-08T23:55:33Z

Made GroupCreateOpts contain a list of UserIds that should be added as members to the group. group_create() will fail if one of these UserIds cannot be found.
Made group_create() return a GroupCreateApiResponse with a try_into() for a new GroupCreateResult. This result is similar to GroupMetaResult but includes a list of group admins and members.
Added a test for adding members on create with the members list.
Changed some tests to return Results and use ? instead of unwrap() to get better error messages.

TODO:

Changelog
Discuss TODO questions aimed at reviewers

src/internal/group_api/mod.rs

src/group.rs

coltfred · 2019-11-14T20:12:31Z

src/group.rs

@@ -157,22 +162,35 @@ impl GroupOps for crate::IronOxide {
        rt.block_on(group_api::list(self.device.auth(), None))
    }

-    fn group_create(&self, opts: &GroupCreateOpts) -> Result<GroupMetaResult> {
+    fn group_create(&self, opts: &GroupCreateOpts) -> Result<GroupCreateResult> {


We don't usually do any business logic up in this layer of the api so this logic should go down into group_api::group_create instead of being done up here.

As we talked about, I'll need some help getting the logic into the other layer

src/group.rs

src/internal/group_api/mod.rs

src/group.rs

src/internal/group_api/mod.rs

coltfred · 2019-11-15T18:39:59Z

src/group.rs

            needs_rotation,
        }
    }
+
+    fn standardize(self, calling_id: &UserId) -> GroupCreateOpts {
+        let mut new_members = self.members.clone();


You might be able to do this clone inside the if to make it not need to happen in the case where we don't have to add the calling users id.

I made this a bit uglier but hopefully also better

src/internal/group_api/mod.rs

coltfred · 2019-11-15T18:46:03Z

src/internal/group_api/requests.rs

+                        .into_iter()
+                        .map(|(mem_id, (pub_key, trans_key))| GroupMember {
+                            user_id: mem_id,
+                            transform_key: trans_key.unwrap().into(), // we can unwrap because we know all members had it calculated


I would rather match on the Some(trans_key) and leave the None case empty (it'll be used in the next PR).

changed to map over the option then flatten out the Nones

clintfred · 2019-11-18T17:20:51Z

src/group.rs

 }

 impl Default for GroupCreateOpts {
    fn default() -> Self {
        // membership is the default!
-        GroupCreateOpts::new(None, None, true, false)
+        GroupCreateOpts::new(None, None, true, Vec::new(), false)


I would suggest creating a new vec with vec![]
Applicable in some other places in this PR as well...

clintfred · 2019-11-18T17:31:14Z

src/internal/group_api/mod.rs

+                            .into_iter()
+                            .map(|(id, user_pub_key)| {
+                                let maybe_transform_key = recrypt.generate_transform_key(
+                                    &group_priv_key.clone().into(),


is this clone needed? I don't see this being used below...

Removing it gives
cannot move out of `group_priv_key`, a captured variable in an `FnMut` closure
move occurs because `group_priv_key` has type `recrypt::api::PrivateKey`, which does not implement the `Copy` trait rustc(E0507)

clintfred · 2019-11-18T20:14:36Z

src/internal/group_api/mod.rs

@@ -297,47 +359,75 @@ pub(crate) fn get_group_keys<'a>(
 /// `user_master_pub_key` - public key of the user creating this group.
 /// `group_id` - unique id for the group within the segment.
 /// `name` - name for the group. Does not need to be unique.
-/// `add_as_member` - if true the user represented by the current DeviceContext will also be added to the group's membership.
-///     If false, the user will not be an member (but will still be an admin)
+/// `members` - list of user ids to add as members of the group. This list takes priority over `add_as_member`,


I don't understand the reference to add_as_member in this comment as it is not part of this function's signature.

clintfred · 2019-11-18T20:16:01Z

src/internal/group_api/mod.rs

+    user_api::user_key_list(auth, members)
+        .and_then(move |member_ids_and_keys| {
+            // this will occur when one of the UserIds in the members list cannot be found
+            if member_ids_and_keys.len() != members.len() {


Maybe this branch should be its own function? Would this be reusable when adding admins? It certainly would be more testable on its own, IMO.

clintfred · 2019-11-18T20:19:17Z

src/internal/group_api/requests.rs

+#[derive(Deserialize, Debug, Clone, PartialEq)]
+#[serde(rename_all = "camelCase")]
+pub struct GroupCreateApiResponse {
+    pub(crate) id: GroupId,


I have been making these response objects pub(in crate:internal) recently as we should never need these outside of the internal portion of the crate.

Added members, structs

82398f9

giarc3 changed the title ~~Added members, structs~~ Add members on group create Nov 9, 2019

ToniaSchlick reviewed Nov 11, 2019

View reviewed changes

src/internal/group_api/mod.rs Outdated Show resolved Hide resolved

giarc3 added 2 commits November 12, 2019 10:52

Self-review cleanup

abca03e

Removed TODO as it'll change with next PR

c2db9bf

coltfred requested changes Nov 14, 2019

View reviewed changes

giarc3 added 5 commits November 15, 2019 09:52

PR changes/updating for later PR

976750d

changed test assertion back

e3ccda0

PR changes

32f7349

changelog

a648c8a

doc change

50f6101

coltfred requested changes Nov 15, 2019

View reviewed changes

self-review

5da0f40

giarc3 requested a review from coltfred November 15, 2019 18:53

PR changes

9d32339

coltfred approved these changes Nov 15, 2019

View reviewed changes

giarc3 merged commit 7ac8904 into master Nov 15, 2019

giarc3 deleted the 65-group-create-members branch November 15, 2019 20:01

clintfred reviewed Nov 18, 2019

View reviewed changes

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts user_verify to async

c40b9b7

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts user_create to async

dfc7732

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts user_list_devices to async

aacaf1d

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts generate_device_key to async

1954423

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts device_delete to async

2af988a

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts user_key_list to async

7fef678

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts user_get_current and user_rotate_private_key to async

4b1adbf

clintfred added a commit that referenced this pull request Nov 20, 2019

[#72] converts get_user_keys to async

7fbc464

clintfred mentioned this pull request Nov 20, 2019

[#73] Converts internal user api and user/requests to async fn #77

Merged

clintfred added a commit that referenced this pull request Nov 21, 2019

[#72] streamlines a few async calls based on PR feedback

a6c4aaf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add members on group create #72

Add members on group create #72

giarc3 commented Nov 8, 2019 •

edited

Loading

coltfred Nov 14, 2019

giarc3 Nov 15, 2019

coltfred Nov 15, 2019

giarc3 Nov 15, 2019

coltfred Nov 15, 2019

giarc3 Nov 15, 2019

clintfred Nov 18, 2019

clintfred Nov 18, 2019

giarc3 Nov 18, 2019

clintfred Nov 18, 2019

clintfred Nov 18, 2019

clintfred Nov 18, 2019

Add members on group create #72

Add members on group create #72

Conversation

giarc3 commented Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giarc3 commented Nov 8, 2019 •

edited

Loading