Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add members on group create #72

Merged
merged 10 commits into from
Nov 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ cache:
before_cache:
- rm -rf /home/travis/.cargo/registry
rust:
- 1.38.0
- 1.39.0
branches:
only:
- master
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## 0.14.0 (Unreleased)

- [[#72](https://github.com/IronCoreLabs/ironoxide/pull/72)]
- Allows adding members at group creation time
- [[#69](https://github.com/IronCoreLabs/ironoxide/pull/69)]
- Allows changing of IronCore environment at runtime.
- [[#64](https://github.com/IronCoreLabs/ironoxide/pull/64)]
Expand Down
42 changes: 34 additions & 8 deletions src/group.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pub use crate::internal::group_api::{
GroupAccessEditErr, GroupAccessEditResult, GroupGetResult, GroupId, GroupListResult,
GroupMetaResult, GroupName,
GroupAccessEditErr, GroupAccessEditResult, GroupCreateResult, GroupGetResult, GroupId,
GroupListResult, GroupMetaResult, GroupName,
};
use crate::{
internal::{group_api, user_api::UserId},
Expand All @@ -18,6 +18,9 @@ pub struct GroupCreateOpts {
// true (default) - creating user will be added to the group's membership (in addition to being the group's admin);
// false - creating user will not be added to the group's membership
add_as_member: bool,
// list of users to add as members to the group.
// note: even if `add_as_member` is false, the calling user will be added as a member if they are in this list.
members: Vec<UserId>,
// true - group's private key will be marked for rotation
// false (default) - group's private key will not be marked for rotation
needs_rotation: bool,
Expand All @@ -32,28 +35,49 @@ impl GroupCreateOpts {
/// - `add_as_member`
/// - `true` - The creating user should be added as a member (in addition to being a group admin)
/// - `false` - The creating user will not be a member of the group, but will still be an admin.
/// - `members` - List of users to be added as members of the group.
/// Note: even if `add_as_member` is false, the calling user will be added as a member if they are in this list.
/// - `needs_rotation`
/// - true - group's private key will be marked for rotation
/// - false (default) - group's private key will not be marked for rotation
pub fn new(
id: Option<GroupId>,
name: Option<GroupName>,
add_as_member: bool,
members: Vec<UserId>,
needs_rotation: bool,
) -> GroupCreateOpts {
GroupCreateOpts {
id,
name,
add_as_member,
members,
needs_rotation,
}
}

fn standardize(self, calling_id: &UserId) -> GroupCreateOpts {
let standardized_members = if self.add_as_member && !self.members.contains(calling_id) {
let mut members = self.members.clone();
members.push(calling_id.clone());
members
} else {
self.members
};
GroupCreateOpts::new(
self.id,
self.name,
self.add_as_member,
standardized_members,
self.needs_rotation,
)
}
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand All @@ -68,7 +92,7 @@ pub trait GroupOps {
///
/// # Arguments
/// `group_create_opts` - See `GroupCreateOpts`. Use the `Default` implementation for defaults.
fn group_create(&self, group_create_opts: &GroupCreateOpts) -> Result<GroupMetaResult>;
fn group_create(&self, group_create_opts: &GroupCreateOpts) -> Result<GroupCreateResult>;

/// Get the full metadata for a specific group given its ID.
///
Expand Down Expand Up @@ -157,22 +181,24 @@ 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut rt = Runtime::new().unwrap();

let GroupCreateOpts {
id: maybe_id,
name: maybe_name,
add_as_member,
add_as_member: _,
members,
needs_rotation,
} = opts.clone();
} = opts.clone().standardize(self.device.auth().account_id());

rt.block_on(group_api::group_create(
&self.recrypt,
self.device.auth(),
&self.user_master_pub_key,
maybe_id,
maybe_name,
add_as_member,
&members,
needs_rotation,
))
}
Expand Down
158 changes: 124 additions & 34 deletions src/internal/group_api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ impl GroupMetaResult {
pub fn id(&self) -> &GroupId {
&self.id
}
/// Name of the group
pub fn name(&self) -> Option<&GroupName> {
self.name.as_ref()
}
Expand All @@ -120,12 +121,15 @@ impl GroupMetaResult {
pub fn is_member(&self) -> bool {
self.is_member
}
/// Date and time of when the group was created
pub fn created(&self) -> &DateTime<Utc> {
&self.created
}
/// Date and time of when the group was last updated
pub fn last_updated(&self) -> &DateTime<Utc> {
&self.updated
}
/// Public key for encrypting to the group
pub fn group_master_public_key(&self) -> &PublicKey {
&self.group_master_public_key
}
Expand All @@ -136,6 +140,61 @@ impl GroupMetaResult {
}
}

pub struct GroupCreateResult {
id: GroupId,
name: Option<GroupName>,
group_master_public_key: PublicKey,
is_admin: bool,
is_member: bool,
admins: Vec<UserId>,
members: Vec<UserId>,
created: DateTime<Utc>,
updated: DateTime<Utc>,
needs_rotation: Option<bool>,
}
impl GroupCreateResult {
giarc3 marked this conversation as resolved.
Show resolved Hide resolved
/// A single document grant/revoke failure for a user or group.
pub fn id(&self) -> &GroupId {
&self.id
}
/// Name of the group
pub fn name(&self) -> Option<&GroupName> {
self.name.as_ref()
}
/// Public key for encrypting to the group
pub fn group_master_public_key(&self) -> &PublicKey {
&self.group_master_public_key
}
/// true if the calling user is a group administrator
pub fn is_admin(&self) -> bool {
self.is_admin
}
/// true if the calling user is a group member
pub fn is_member(&self) -> bool {
self.is_member
}
/// List of all group admins. Group admins can change group membership.
pub fn admins(&self) -> &Vec<UserId> {
self.admins.as_ref()
}
/// List of group members. Members of a group can decrypt values encrypted to the group.
pub fn members(&self) -> &Vec<UserId> {
self.members.as_ref()
}
/// Date and time of when the group was created
pub fn created(&self) -> &DateTime<Utc> {
&self.created
}
/// Date and time of when the group was last updated
pub fn last_updated(&self) -> &DateTime<Utc> {
&self.updated
}
/// `Some(boolean)` indicating if the group needs rotation if the calling user is a group admin.
/// `None` if the calling user is not a group admin.
pub fn needs_rotation(&self) -> Option<bool> {
self.needs_rotation
}
}
/// Group information.
#[derive(Debug)]
pub struct GroupGetResult {
Expand All @@ -156,9 +215,11 @@ impl GroupGetResult {
pub fn id(&self) -> &GroupId {
&self.id
}
/// Name of the group
pub fn name(&self) -> Option<&GroupName> {
self.name.as_ref()
}
/// Public key for encrypting to the group
pub fn group_master_public_key(&self) -> &PublicKey {
&self.group_master_public_key
}
Expand All @@ -170,9 +231,11 @@ impl GroupGetResult {
pub fn is_member(&self) -> bool {
self.is_member
}
/// Date and time of when the group was created
pub fn created(&self) -> &DateTime<Utc> {
&self.created
}
/// Date and time of when the group was last updated
pub fn last_updated(&self) -> &DateTime<Utc> {
&self.updated
}
Expand Down Expand Up @@ -222,7 +285,6 @@ impl GroupAccessEditResult {
pub fn failed(&self) -> &Vec<GroupAccessEditErr> {
&self.failed
}

/// Users whose access was modified.
pub fn succeeded(&self) -> &Vec<UserId> {
&self.succeeded
Expand Down Expand Up @@ -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`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// so the calling user will be added as a member if their id is in this list even if `add_as_member` is false.
/// `needs_rotation` - true if the group private key should be rotated by an admin, else false
pub fn group_create<'a, CR: rand::CryptoRng + rand::RngCore>(
recrypt: &'a Recrypt<Sha256, Ed25519, RandomBytes<CR>>,
auth: &'a RequestAuth,
user_master_pub_key: &'a PublicKey,
group_id: Option<GroupId>,
name: Option<GroupName>,
add_as_member: bool,
members: &'a Vec<UserId>,
needs_rotation: bool,
) -> impl Future<Item = GroupMetaResult, Error = IronOxideErr> + 'a {
transform::gen_group_keys(recrypt)
.and_then(move |(plaintext, group_priv_key, group_pub_key)| {
// encrypt the group secret to the current user as they will be the group admin
Ok({
let encrypted_group_key = recrypt.encrypt(
&plaintext,
&user_master_pub_key.into(),
&auth.signing_private_key().into(),
)?;

// compute a transform key if we are adding the caller as a group member as well
let transform_key: Option<crate::internal::TransformKey> = if add_as_member {
Some(
recrypt
.generate_transform_key(
&group_priv_key.into(),
&user_master_pub_key.into(),
&auth.signing_private_key().into(),
)?
.into(),
)
} else {
None
};
(group_pub_key, encrypted_group_key, transform_key)
})
) -> impl Future<Item = GroupCreateResult, Error = IronOxideErr> + 'a {
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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// figure out which user ids could not be found in the database and include them in the error message.
use std::{collections::HashSet, iter::FromIterator};
let desired_members_set: HashSet<&UserId> = HashSet::from_iter(members);
let found_members: Vec<UserId> =
member_ids_and_keys.into_iter().map(|(x, _)| x).collect();
let found_members_set: HashSet<&UserId> = HashSet::from_iter(&found_members);
let diff: HashSet<&&UserId> =
desired_members_set.difference(&found_members_set).collect();
futures::future::err(IronOxideErr::UserDoesNotExist(format!(
"Failed to find the following users from the `members` list: {:?}",
diff
)))
} else {
transform::gen_group_keys(recrypt)
.and_then(move |(plaintext, group_priv_key, group_pub_key)| {
// encrypt the group secret to the current user as they will be the group admin
let encrypted_group_key = recrypt.encrypt(
&plaintext,
&user_master_pub_key.into(),
&auth.signing_private_key().into(),
)?;
// Map from UserId to (PublicKey, Optional TransformKey)
// TransformKey is optional because we need it for members, but won't need it
// when we expand this to include admins
let member_info_result: Result<
HashMap<UserId, (PublicKey, Option<TransformKey>)>,
_,
> = member_ids_and_keys
.into_iter()
.map(|(id, user_pub_key)| {
let maybe_transform_key = recrypt.generate_transform_key(
&group_priv_key.clone().into(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

&user_pub_key.clone().into(),
&auth.signing_private_key().into(),
);
maybe_transform_key.map(|transform_key| {
(id, (user_pub_key, Some(transform_key.into())))
})
})
.collect();
let member_info = member_info_result?;

let maybe_member_info = if !member_info.is_empty() {
Some(member_info)
} else {
None
};
Ok((group_pub_key, encrypted_group_key, maybe_member_info))
})
.into_future()
}
})
.into_future()
.and_then(move |(group_pub_key, encrypted_group_key, transform_key)| {
.and_then(move |(group_pub_key, encrypted_group_key, member_info)| {
requests::group_create::group_create(
&auth,
user_master_pub_key,
Expand All @@ -346,7 +436,7 @@ pub fn group_create<'a, CR: rand::CryptoRng + rand::RngCore>(
encrypted_group_key,
group_pub_key,
auth.account_id(),
transform_key,
member_info,
needs_rotation,
)
})
Expand Down
Loading