Skip to content

Commit

Permalink
[PF-2937] Validate group admin role before making Sam calls (#530)
Browse files Browse the repository at this point in the history
* valid group admin before making Sam calls

* fix tests
  • Loading branch information
zloery authored Aug 29, 2023
1 parent d047381 commit ccc46d9
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 7 deletions.
24 changes: 20 additions & 4 deletions src/main/java/bio/terra/cli/businessobject/Group.java
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ private static Map<String, Group> listGroupsInMap() {

/** Delete a SAM group. */
public void delete() {
validateGroupAdmin();
SamService.fromContext().deleteGroup(name);
logger.info("Deleted group: name={}", name);
}
Expand All @@ -93,6 +94,7 @@ public void delete() {
* @param policy policy to assign the user
*/
public Member addPolicyToMember(String email, GroupPolicy policy) {
validateGroupAdmin();
// call SAM to add a policy + email to the group
SamService.fromContext().addUserToGroup(name, policy, email);
logger.info("Added user to group: group={}, email={}, policy={}", name, email, policy);
Expand All @@ -108,6 +110,7 @@ public Member addPolicyToMember(String email, GroupPolicy policy) {
* @param policy policy to remove from the user
*/
public void removePolicyFromMember(String email, GroupPolicy policy) {
validateGroupAdmin();
// check that the email is a group member
getMember(email);

Expand All @@ -121,7 +124,7 @@ public void removePolicyFromMember(String email, GroupPolicy policy) {
*
* @throws UserActionableException if the member is not found
*/
public Member getMember(String email) {
private Member getMember(String email) {
// lowercase the email so there is a consistent way of looking up the email address
// the email address casing in SAM may not match the case of what is provided by the user
Member foundMember = listMembersByEmail().get(email.toLowerCase());
Expand All @@ -133,6 +136,7 @@ public Member getMember(String email) {

/** List the members of the group. */
public List<Member> getMembers() {
validateGroupAdmin();
return new ArrayList<>(listMembersByEmail().values());
}

Expand Down Expand Up @@ -163,13 +167,25 @@ private Map<String, Member> listMembersByEmail() {
return groupMembers;
}

public String getName() {
return name;
/**
* Throw a user-actionable exception if the current user is not an admin of this group, which is
* required for some operations.
*/
private void validateGroupAdmin() {
if (!currentUserPolicies.contains(GroupPolicy.ADMIN)) {
throw new UserActionableException(
String.format(
"Cannot view or modify membership of group %s, user is not an administrator of this group.",
name));
}
}

// ====================================================
// Property getters.

public String getName() {
return name;
}

public String getEmail() {
return email;
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/unit/Group.java
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,15 @@ void onlyAdminCanModifyGroup() throws IOException {

// `terra group delete --name=$name` as member
groupMember.login();
TestCommand.runCommandExpectExitCode(2, "group", "delete", "--name=" + name, "--quiet");
TestCommand.runCommandExpectExitCode(1, "group", "delete", "--name=" + name, "--quiet");

// `terra group add-user --email=$email --policy=ADMIN` as member
TestCommand.runCommandExpectExitCode(
2, "group", "add-user", "--name=" + name, "--email=" + groupMember.email, "--policy=ADMIN");
1, "group", "add-user", "--name=" + name, "--email=" + groupMember.email, "--policy=ADMIN");

// `terra group remove-user --email=$email --policy=MEMBER` as member
TestCommand.runCommandExpectExitCode(
2,
1,
"group",
"remove-user",
"--name=" + name,
Expand Down

0 comments on commit ccc46d9

Please sign in to comment.