[api] do not accept new requests with foreign persons anymore

In the past we just reset the request creator to the logged-in
user, now we throw an error.

Just admins can still put request for other people

src/api/app/controllers/request_controller.rb

@@ -133,6 +133,7 @@ def update
 
       req = BsRequest.new_from_xml(body)
       req.id = params[:id]
+      req.skip_sanitize
       req.save!
 
       notify = oldrequest.notify_parameters

src/api/app/controllers/webui/package_controller.rb

@@ -213,8 +213,6 @@ def submit_request
     begin
       BsRequest.transaction do
         req = BsRequest.new
-        req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = { source_project: params[:project],

src/api/app/controllers/webui/project_controller.rb

@@ -231,8 +231,6 @@ def new_incident_request
     begin
       BsRequest.transaction do
         req = BsRequest.new
-        req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         action = BsRequestActionMaintenanceIncident.new({source_project: params[:project]})
@@ -262,8 +260,6 @@ def new_release_request
         req=nil
         BsRequest.transaction do
           req = BsRequest.new
-          req.state = "new"
-          req.creator = User.current
           req.description = params[:description]
 
           action = BsRequestActionMaintenanceRelease.new({source_project: params[:project]})
@@ -842,8 +838,6 @@ def remove_target_request
     begin
       BsRequest.transaction do
         req = BsRequest.new
-        req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = {target_project: params[:project]}

src/api/app/controllers/webui/request_controller.rb

@@ -184,7 +184,6 @@ def forward_request_to(fwd)
       BsRequest.transaction do
         req = BsRequest.new
         req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = { source_project: target.value(:project),
@@ -250,7 +249,6 @@ def delete_request
       BsRequest.transaction do
         req = BsRequest.new
         req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = {target_project: params[:project]}
@@ -285,7 +283,6 @@ def add_role_request
       BsRequest.transaction do
         req = BsRequest.new
         req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = {target_project: params[:project],
@@ -319,7 +316,6 @@ def set_bugowner_request
       BsRequest.transaction do
         req = BsRequest.new
         req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = {target_project: params[:project]}
@@ -359,7 +355,6 @@ def change_devel_request
       BsRequest.transaction do
         req = BsRequest.new
         req.state = "new"
-        req.creator = User.current
         req.description = params[:description]
 
         opts = {target_project: params[:project],

src/api/app/jobs/project_create_auto_cleanup_requests.rb

@@ -53,7 +53,7 @@ def autoclean_project(prj)
                                           <target project="' + prj.name + '" />
                                        </action>
                                        <description>'+Description+'</description>
-                                       <state who="Admin" name="new"/>
+                                       <state />
                                        <accept_at>' + @cleanupTime.to_s + '</accept_at>
                                      </request>')
     req.save!

src/api/app/models/bs_request.rb

@@ -33,7 +33,7 @@ class SaveError < APIException
 
   def save!
     new = self.created_at ? nil : 1
-    sanitize! if new
+    sanitize! if new and not @skip_sanitize
     super
     notify if new
   end
@@ -46,6 +46,10 @@ def set_ignore_build_state
     @ignore_build_state = true
   end
 
+  def skip_sanitize
+    @skip_sanitize = true
+  end
+
   def check_supersede_state
     if self.state == :superseded and ( not self.superseded_by.is_a?(Numeric) or not self.superseded_by > 0 )
       errors.add(:superseded_by, 'Superseded_by should be set')
@@ -130,7 +134,7 @@ def self.new_from_xml(xml)
       end
 
       state = hashed.delete('state') || Xmlhash::XMLHash.new({'name' => 'new'})
-      request.state = state.delete('name') { raise ArgumentError, 'state without name' }
+      request.state = state.delete('name') || User.current.login
       request.state = :declined if request.state.to_s == 'rejected'
       request.state = :accepted if request.state.to_s == 'accept'
       request.state = request.state.to_sym
@@ -482,7 +486,6 @@ def change_review_state(state, opts = {})
         self.comment = opts[:comment]
         self.comment = 'All reviewers accepted request' if go_new_state == :accepted
       end
-
       self.save!
     end
   end
@@ -769,10 +772,16 @@ def is_target_maintainer?(user = User.current)
 
   def sanitize!
     # apply default values, expand and do permission checks
+    self.creator ||= User.current.login
+    self.commenter ||= User.current.login
+    unless self.creator == User.current.login or User.current.is_admin?
+      raise SaveError, 'Admin permissions required to set request creator to foreign user'
+    end
+    unless self.commenter == User.current.login or User.current.is_admin?
+      raise SaveError, 'Admin permissions required to set request commenter to foreign user'
+    end
 
     # ensure correct initial values, no matter what has been sent to us
-    self.commenter = User.current.login
-    self.creator = User.current.login
     self.state = :new
 
     # expand release and submit request targets if not specified
@@ -807,23 +816,21 @@ def sanitize!
     reviewers.uniq.each do |r|
       if r.class == User
         next if self.reviews.select{|a| a.by_user == r.login}.length > 0
-        self.reviews.new by_user: r.login 
+        self.reviews.new(by_user: r.login, state: :new)
       elsif r.class == Group
         next if self.reviews.select{|a| a.by_group == r.title}.length > 0
-        self.reviews.new by_group: r.title
+        self.reviews.new(by_group: r.title, state: :new)
       elsif r.class == Project
         next if self.reviews.select{|a| a.by_project == r.name and a.by_package.nil? }.length > 0
-        self.reviews.new by_project: r.name
+        self.reviews.new(by_project: r.name, state: :new)
       elsif r.class == Package
         next if self.reviews.select{|a| a.by_project == r.project.name and a.by_package == r.name }.length > 0
-        rev = self.reviews.new by_project: r.project.name
-        rev.by_package = r.name
+        self.reviews.new(by_project: r.project.name, by_package: r.name, state: :new)
       else
         raise 'Unknown review type'
       end
     end
-
-    self.state = :review if self.reviews.length > 0
+    self.state = :review if self.reviews.select{|a| a.state == :new}.length > 0
   end
 
   def notify

src/api/test/fixtures/backend/request/1

@@ -4,5 +4,5 @@
     <target project='kde4' package='wpa_supplicant'/>
   </action>
   <description/>
-  <state name='new' who='coolo' when='2011-12-02T17:20:42'/>
+  <state name='new' who='tom' when='2011-12-02T17:20:42'/>
 </request>

src/api/test/fixtures/backend/request/from_source_protected_valid

@@ -1,4 +1,4 @@
-  <request>
+<request>
   <action type='submit'>
     <source project='SourceprotectedProject' package='pack' rev='1' />
     <target project='home:tom' package='pack' />
@@ -7,6 +7,6 @@
     </options>
   </action>
   <description/>
-  <state who="adrian" name="new"/>
-  </request>
+  <state />
+</request>
 

src/api/test/fixtures/backend/request/no_such_group

@@ -4,6 +4,6 @@
     <group name="DoesNotExist" role="maintainer" />
   </action>
   <description/>
-  <state who="DoesNotExist" name="new"/>
+  <state />
   </request>
 

src/api/test/fixtures/backend/request/no_such_package

@@ -1,8 +1,8 @@
-  <request>
+<request>
   <action type='submit'>
     <source project='home:Iggy' package='mypackage' rev='1' />
     <target project='kde4' package='mypackage' />
   </action>
   <description/>
-  </request>
+</request>
 

src/api/test/fixtures/backend/request/no_such_target_package

@@ -1,9 +1,9 @@
-  <request>
+<request>
   <action type="add_role">
     <target project='home:Iggy' package='DoesNotExist' />
     <person name="Iggy" role="maintainer" />
   </action>
   <description/>
-  <state who="DoesNotExist" name="new"/>
-  </request>
+  <state />
+</request>
 

src/api/test/fixtures/backend/request/no_such_user

@@ -4,6 +4,6 @@
     <person name="DoesNotExist" role="maintainer" />
   </action>
   <description/>
-  <state who="DoesNotExist" name="new"/>
+  <state />
   </request>
 

src/api/test/fixtures/backend/request/submit_with_review

@@ -3,8 +3,8 @@
     <source project='home:Iggy' package='TestPack' rev='1' />
     <target project='kde4' package="Testing" />
   </action>
-  <review state="new" by_user="adrian" />
-  <review state="new" by_group="test_group" />
+  <review state="new" by_user="Iggy" />
+  <review state="new" by_group="test_group_b" />
   <description/>
   <state who="Iggy" name="new"/>
 </request>

src/api/test/functional/group_request_test.rb

@@ -162,6 +162,10 @@ def test_set_and_get
     assert_response :success
     post "/request/#{withr2}?cmd=changereviewstate&by_group=test_group&newstate=accepted"
     assert_response :success
+    post "/request/#{withr2}?cmd=changereviewstate&by_user=Iggy&newstate=accepted"
+    assert_response :success
+    post "/request/#{withr2}?cmd=changereviewstate&by_group=test_group_b&newstate=accepted"
+    assert_response :success
     get "/request/#{withr2}"
     # now it would be new - but as #{withhr} is still in review, the group blocks it
     assert_xml_tag(:tag => "state", :attributes => {:name => "review"})
@@ -171,6 +175,10 @@ def test_set_and_get
     assert_response :success
     post "/request/#{withr}?cmd=changereviewstate&by_group=test_group&newstate=accepted"
     assert_response :success
+    post "/request/#{withr}?cmd=changereviewstate&by_user=Iggy&newstate=accepted"
+    assert_response :success
+    post "/request/#{withr}?cmd=changereviewstate&by_group=test_group_b&newstate=accepted"
+    assert_response :success
     get "/request/#{withr}"
     # should be new as no other review in the group blocked it
     assert_xml_tag(:tag => "state", :attributes => {:name => "new"})