Change group creation to be PUT /groups/{name}

Now that the groups collection also accepts group names as
identifiers for members, it makes sense to accept PUT for
entity creation. The collection will check if the group already
exists and fail as there is currently no binding for PUT on
a GROUP_KIND. Creation can use the annotation to check the
user has the correct capability. Accept most simple group
properties as part of the creation request.

Use HTTP If-None-Match: * to test for the group already existing
and fail if it is present. This protects both the client and the
server against a future binding of PUT on GROUP_KIND.

Change-Id: If226274f873b32b0df5278cceab452e2a51c5536

Documentation/rest-api.txt

@@ -21,6 +21,14 @@ Gerrit by default uses HTTP digest authentication. To authenticate,
 prefix the endpoint URL with `/a/`. For example to authenticate to
 `/projects/` request URL `/a/projects/`.
 
+[[preconditions]]
+Preconditions
+~~~~~~~~~~~~~
+Clients can request PUT to create a new resource and not overwrite
+an existing one by adding `If-None-Match: *` to the request HTTP
+headers. If the named resource already exists the server will respond
+with HTTP 412 Precondition Failed.
+
 [[output]]
 Output Format
 ~~~~~~~~~~~~~

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/PreconditionFailedException.java

@@ -0,0 +1,30 @@
+// Copyright (C) 2012 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.restapi;
+
+/**
+ * Resource state does not match request state (HTTP 412 Precondition failed).
+ */
+public class PreconditionFailedException extends RestApiException {
+  private static final long serialVersionUID = 1L;
+
+  public PreconditionFailedException() {
+  }
+
+  /** @param msg message to return to the client describing the error. */
+  public PreconditionFailedException(String msg) {
+    super(msg);
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/groups/GroupApi.java

@@ -33,9 +33,9 @@ public class GroupApi {
 
   /** Create a new group */
   public static void createGroup(String groupName, AsyncCallback<GroupInfo> cb) {
-    GroupInput in = GroupInput.create();
-    in.name(groupName);
-    new RestApi("/groups/").data(in).post(cb);
+    JavaScriptObject in = JavaScriptObject.createObject();
+    String n = URL.encodePathSegment(groupName);
+    new RestApi("/groups/" + n).ifNoneMatch().data(in).put(cb);
   }
 
   /** Add member to a group. */
@@ -108,15 +108,4 @@ public class GroupApi {
     protected MemberInput() {
     }
   }
-
-  private static class GroupInput extends JavaScriptObject {
-    final native void name(String n) /*-{ if(n)this.name=n; }-*/;
-
-    static GroupInput create() {
-      return (GroupInput) createObject();
-    }
-
-    protected GroupInput() {
-    }
-  }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java

@@ -91,6 +91,7 @@ public class RestApi {
       case 404: // Not Found
       case 405: // Method Not Allowed
       case 409: // Conflict
+      case 412: // Precondition Failed
       case 429: // Too Many Requests (RFC 6585)
         return true;
 
@@ -181,6 +182,7 @@ public class RestApi {
   private boolean hasQueryParams;
   private String contentType;
   private String contentData;
+  private String ifNoneMatch;
 
   /**
    * Initialize a new API call.
@@ -242,6 +244,15 @@ public class RestApi {
     return this;
   }
 
+  public RestApi ifNoneMatch() {
+    return ifNoneMatch("*");
+  }
+
+  public RestApi ifNoneMatch(String etag) {
+    ifNoneMatch = etag;
+    return this;
+  }
+
   public RestApi data(JavaScriptObject obj) {
     return data(new JSONObject(obj));
   }
@@ -283,6 +294,9 @@ public class RestApi {
       Method method,
       final AsyncCallback<T> cb) {
     RequestBuilder req = new RequestBuilder(method, url());
+    if (ifNoneMatch != null) {
+      req.setHeader("If-None-Match", ifNoneMatch);
+    }
     req.setHeader("Accept", JSON_TYPE);
     if (Gerrit.getAuthorization() != null) {
       req.setHeader("Authorization", Gerrit.getAuthorization());

gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@@ -24,9 +24,11 @@ import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
 import static javax.servlet.http.HttpServletResponse.SC_METHOD_NOT_ALLOWED;
 import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
 import static javax.servlet.http.HttpServletResponse.SC_OK;
+import static javax.servlet.http.HttpServletResponse.SC_PRECONDITION_FAILED;
 
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
+import com.google.common.base.Objects;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMultimap;
@@ -46,6 +48,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.BinaryResult;
 import com.google.gerrit.extensions.restapi.DefaultInput;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.PreconditionFailedException;
 import com.google.gerrit.extensions.restapi.PutInput;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -195,6 +198,7 @@ public class RestApiServlet extends HttpServlet {
         String id = path.remove(0);
         try {
           rsrc = rc.parse(rsrc, id);
+          checkPreconditions(req, rsrc);
         } catch (ResourceNotFoundException e) {
           if (rc instanceof AcceptsCreate
               && ("POST".equals(req.getMethod())
@@ -233,6 +237,7 @@ public class RestApiServlet extends HttpServlet {
           String id = path.remove(0);
           try {
             rsrc = c.parse(rsrc, id);
+            checkPreconditions(req, rsrc);
             view = null;
           } catch (ResourceNotFoundException e) {
             if (c instanceof AcceptsCreate
@@ -299,6 +304,9 @@ public class RestApiServlet extends HttpServlet {
       replyError(res, SC_METHOD_NOT_ALLOWED, "Method not allowed");
     } catch (ResourceConflictException e) {
       replyError(res, SC_CONFLICT, e.getMessage());
+    } catch (PreconditionFailedException e) {
+      replyError(res, SC_PRECONDITION_FAILED,
+          Objects.firstNonNull(e.getMessage(), "Precondition failed"));
     } catch (ResourceNotFoundException e) {
       replyError(res, SC_NOT_FOUND, "Not found");
     } catch (AmbiguousViewException e) {
@@ -310,6 +318,13 @@ public class RestApiServlet extends HttpServlet {
     }
   }
 
+  private void checkPreconditions(HttpServletRequest req, RestResource rsrc)
+      throws PreconditionFailedException {
+    if ("*".equals(req.getHeader("If-None-Match"))) {
+      throw new PreconditionFailedException();
+    }
+  }
+
   private static Type inputType(RestModifyView<RestResource, Object> m) {
     Type inputType = extractInputType(m.getClass());
     if (inputType == null) {

gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java

@@ -14,72 +14,108 @@
 
 package com.google.gerrit.server.group;
 
+import com.google.common.base.Objects;
 import com.google.common.base.Strings;
+import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.data.GroupDescriptions;
 import com.google.gerrit.common.errors.NameAlreadyUsedException;
 import com.google.gerrit.common.errors.PermissionDeniedException;
+import com.google.gerrit.extensions.annotations.RequiresCapability;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.DefaultInput;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.PerformCreateGroup;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.group.CreateGroup.Input;
 import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.assistedinject.Assisted;
 
 import org.eclipse.jgit.lib.Config;
 
 import java.util.Collections;
 
-public class CreateGroup implements RestModifyView<TopLevelResource, Input> {
+@RequiresCapability(GlobalCapability.CREATE_GROUP)
+class CreateGroup implements RestModifyView<TopLevelResource, Input> {
   static class Input {
-    @DefaultInput
     String name;
+    String description;
+    Boolean visibleToAll;
+    String ownerId;
   }
 
-  private final PerformCreateGroup.Factory performCreateGroupFactory;
-  private final GroupCache groupCache;
-  private final Provider<CurrentUser> self;
-  final boolean visibleToAll;
+  static interface Factory {
+    CreateGroup create(@Assisted String name);
+  }
 
-  CreateGroup(final PerformCreateGroup.Factory performCreateGroupFactory,
-      final GroupCache groupCache, final Provider<CurrentUser> self,
-      final Config cfg) {
-    this.performCreateGroupFactory = performCreateGroupFactory;
-    this.groupCache = groupCache;
+  private final Provider<IdentifiedUser> self;
+  private final GroupsCollection groups;
+  private final PerformCreateGroup.Factory op;
+  private final boolean defaultVisibleToAll;
+  private final String name;
+
+  @Inject
+  CreateGroup(
+      Provider<IdentifiedUser> self,
+      GroupsCollection groups,
+      PerformCreateGroup.Factory performCreateGroupFactory,
+      @GerritServerConfig Config cfg,
+      @Assisted String name) {
     this.self = self;
-    this.visibleToAll = cfg.getBoolean("groups", "newGroupsVisibleToAll", false);
+    this.groups = groups;
+    this.op = performCreateGroupFactory;
+    this.defaultVisibleToAll = cfg.getBoolean("groups", "newGroupsVisibleToAll", false);
+    this.name = name;
   }
 
   @Override
   public GroupInfo apply(TopLevelResource resource, Input input)
       throws AuthException, BadRequestException, OrmException,
       NameAlreadyUsedException {
-    final IdentifiedUser me = ((IdentifiedUser) self.get());
-    if (!me.getCapabilities().canCreateGroup()) {
-      throw new AuthException("Cannot create group");
-    }
-
-    if (input == null || Strings.isNullOrEmpty(input.name)) {
-      throw new BadRequestException("group name missing");
-    }
-
-    AccountGroup group = groupCache.get(new AccountGroup.NameKey(input.name));
-    if (group != null) {
-      return new GroupInfo(GroupDescriptions.forAccountGroup(group));
+    if (input == null) {
+      input = new Input();
+    }
+    if (input.name != null && !name.equals(input.name)) {
+      throw new BadRequestException("name must match URL");
     }
 
+    AccountGroup.Id ownerId = owner(input);
+    AccountGroup group;
     try {
-      group = performCreateGroupFactory.create().createGroup(input.name, null,
-          visibleToAll, null, Collections.singleton(me.getAccountId()), null);
+      group = op.create().createGroup(
+          name,
+          Strings.emptyToNull(input.description),
+          Objects.firstNonNull(input.visibleToAll, defaultVisibleToAll),
+          ownerId,
+          ownerId == null
+            ? Collections.singleton(self.get().getAccountId())
+            : Collections.<Account.Id> emptySet(),
+          null);
     } catch (PermissionDeniedException e) {
       throw new AuthException(e.getMessage());
     }
     return new GroupInfo(GroupDescriptions.forAccountGroup(group));
   }
+
+  private AccountGroup.Id owner(Input input) throws BadRequestException {
+    if (input.ownerId != null) {
+      try {
+        GroupResource rsrc = groups.parse(input.ownerId);
+        AccountGroup owner = GroupDescriptions.toAccountGroup(rsrc.getGroup());
+        if (owner == null) {
+          throw new BadRequestException("ownerId must be internal group");
+        }
+        return owner.getId();
+      } catch (ResourceNotFoundException e) {
+        throw new BadRequestException("ownerId cannot be resolved");
+      }
+    }
+    return null;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java

@@ -17,7 +17,7 @@ package com.google.gerrit.server.group;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.registration.DynamicMap;
-import com.google.gerrit.extensions.restapi.AcceptsPost;
+import com.google.gerrit.extensions.restapi.AcceptsCreate;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestCollection;
@@ -29,44 +29,34 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupBackends;
-import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupControl;
-import com.google.gerrit.server.account.PerformCreateGroup;
-import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.util.Url;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
-import org.eclipse.jgit.lib.Config;
-
 public class GroupsCollection implements
     RestCollection<TopLevelResource, GroupResource>,
-    AcceptsPost<TopLevelResource> {
+    AcceptsCreate<TopLevelResource> {
   private final DynamicMap<RestView<GroupResource>> views;
   private final Provider<ListGroups> list;
+  private final CreateGroup.Factory createGroup;
   private final GroupControl.Factory groupControlFactory;
   private final GroupBackend groupBackend;
   private final Provider<CurrentUser> self;
-  private final PerformCreateGroup.Factory performCreateGroupFactory;
-  private final GroupCache groupCache;
-  private final Config cfg;
 
   @Inject
   GroupsCollection(final DynamicMap<RestView<GroupResource>> views,
       final Provider<ListGroups> list,
+      final CreateGroup.Factory createGroup,
       final GroupControl.Factory groupControlFactory,
       final GroupBackend groupBackend,
-      final Provider<CurrentUser> self,
-      final PerformCreateGroup.Factory performCreateGroupFactory,
-      final GroupCache groupCache, @GerritServerConfig final Config cfg) {
+      final Provider<CurrentUser> self) {
     this.views = views;
     this.list = list;
+    this.createGroup = createGroup;
     this.groupControlFactory = groupControlFactory;
     this.groupBackend = groupBackend;
     this.self = self;
-    this.performCreateGroupFactory = performCreateGroupFactory;
-    this.groupCache = groupCache;
-    this.cfg = cfg;
   }
 
   @Override
@@ -134,8 +124,8 @@ public class GroupsCollection implements
 
   @SuppressWarnings("unchecked")
   @Override
-  public CreateGroup post(TopLevelResource parent) {
-    return new CreateGroup(performCreateGroupFactory, groupCache, self, cfg);
+  public CreateGroup create(TopLevelResource root, String name) {
+    return createGroup.create(name);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/group/Module.java

@@ -20,8 +20,9 @@ import static com.google.gerrit.server.group.MemberResource.MEMBER_KIND;
 
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.RestApiModule;
-import com.google.gerrit.server.group.DeleteMembers.DeleteMember;
 import com.google.gerrit.server.group.AddMembers.UpdateMember;
+import com.google.gerrit.server.group.DeleteMembers.DeleteMember;
+import com.google.inject.assistedinject.FactoryModuleBuilder;
 
 public class Module extends RestApiModule {
   @Override
@@ -43,5 +44,7 @@ public class Module extends RestApiModule {
 
     child(GROUP_KIND, "groups").to(IncludedGroupsCollection.class);
     get(INCLUDED_GROUP_KIND).to(GetIncludedGroup.class);
+
+    install(new FactoryModuleBuilder().build(CreateGroup.Factory.class));
   }
 }