[webui] make sure to check if source access is allowed in #show

src/api/app/models/package.rb

@@ -346,19 +346,23 @@ def source_path(file = nil, opts = {})
     Package.source_path(self.project.name, self.name, file, opts)
   end
 
-  def source_file(file)
-    Suse::Backend.get(source_path(file)).body
+  def source_file(file, opts = {})
+    Suse::Backend.get(source_path(file, opts)).body
   end
 
-  def dir_hash(opts = {})
+  def self.dir_hash(project, package, opts = {})
     begin
-      directory = Suse::Backend.get(self.source_path(nil, opts)).body
+      directory = Suse::Backend.get(source_path(project, package, nil, opts)).body
       Xmlhash.parse(directory)
     rescue ActiveXML::Transport::Error => e
       Xmlhash::XMLHash.new error: e.summary
     end
   end
 
+  def dir_hash(opts = {})
+    Package.dir_hash(self.project.name, self.name, opts)
+  end
+
   def private_set_package_kind( dir )
     raise ArgumentError.new 'need a xmlhash' unless dir.is_a? Xmlhash::XMLHash
     kinds = detect_package_kinds( dir )
@@ -385,15 +389,15 @@ def update_issue_list
         xml = Patchinfo.new.read_patchinfo_xmlhash(self)
         Project.transaction do
           self.package_issues.delete_all
-          xml.elements('issue') { |i|
+          xml.elements('issue') do |i|
             begin
               issue = Issue.find_or_create_by_name_and_tracker(i['id'], i['tracker'])
               self.package_issues.create(issue: issue, change: 'kept')
-	    rescue IssueTracker::NotFoundError => e
+            rescue IssueTracker::NotFoundError => e
               # if the issue is invalid, we ignore it
               Rails.logger.debug e
-	    end
-          }
+            end
+          end
         end
       else
         # onlyissues gets the issues from .changes files
@@ -661,9 +665,11 @@ def open_requests_with_by_package_review
   end
 
   def linkinfo
-    dir = Directory.find( :project => self.project.name, :package => self.name )
-    return nil unless dir
-    return dir.to_hash['linkinfo']
+    dir_hash['linkinfo']
+  end
+
+  def rev
+    dir_hash['rev']
   end
 
   def channels

src/api/test/unit/code_quality_test.rb

@@ -154,7 +154,6 @@ def setup
       'Webui::BsRequest::make_stub' => 233.83,
       'Webui::ProjectController#status_check_package' => 187.14,
       'Webui::ProjectController#monitor' => 154.83,
-      'Webui::RequestController#changerequest' => 140.03,
       'Webui::PackageController#save_new_link' => 136.05,
       'Webui::WebuiHelper#flag_status' => 93.0,
       'Webui::ProjectController#save_targets' => 127.29,
@@ -174,7 +173,6 @@ def setup
       'Webui::WebuiController#validate_xhtml' => 78.83,
       'Webui::RequestController#show' => 72.45,
       'Webui::PackageController#save_new' => 77.0,
-      'Webui::Package#commit' => 71.56,
       'Webui::PatchinfoController#new_tracker' => 68.43,
       'Webui::ProjectController#status' => 67.37,
       'Webui::MonitorController#events' => 59.75,

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

@@ -181,16 +181,14 @@ def commit
   end
 
   def revisions
-    begin
-      @max_revision = Webui::Package.current_rev(@project, @package.name).to_i
-    rescue ActiveXML::Transport::ForbiddenError => e
-      flash[:error] = "Could not access revisions: #{e.summary}"
+    unless @package.api_obj.check_source_access?
+      flash[:error] = 'Could not access revisions'
       redirect_to :action => :show, :project => @project.name, :package => @package.name and return
     end
+    @max_revision = @package.api_obj.rev.to_i
     @upper_bound = @max_revision
     if params[:showall]
-      p = Webui::Package.find( @package.name, :project => @project)
-      p.cacheAllCommits # we need to fetch commits alltogether for the cache and not each single one
+      @package.cacheAllCommits # we need to fetch commits alltogether for the cache and not each single one
       @visible_commits = @max_revision
     else
       @upper_bound = params[:rev].to_i if params[:rev]
@@ -203,7 +201,7 @@ def submit_request_dialog
     if params[:revision]
       @revision = params[:revision]
     else
-      @revision = Webui::Package.current_rev(@project, @package)
+      @revision = @package.api_obj.rev
     end
     @cleanup_source = @project.value('name').include?(':branches:') # Rather ugly decision finding...
     render_dialog
@@ -250,8 +248,11 @@ def submit_request
   def set_file_details
     @forced_unexpand ||= ''
 
+    # check source access
+    return false unless @package.api_obj.check_source_access?
+
     begin
-      @current_rev = Webui::Package.current_rev(@project.name, @package.name)
+      @current_rev = @package.api_obj.rev
       if not @revision and not @srcmd5
         # on very first page load only
         @revision = @current_rev
@@ -511,8 +512,8 @@ def save_new_link
       redirect_to :controller => :project, :action => 'new_package_branch', :project => @project and return
     end
 
-    revision = Webui::Package.current_xsrcmd5(@linked_project, @linked_package)
-    revision = Webui::Package.current_rev(@linked_project, @linked_package) unless revision
+    dirhash = linked_package.api_obj.dir_hash
+    revision = dirhash['xsrcmd5'] || dirhash['rev']
     unless revision
       flash[:error] = "Unable to branch package '#{@target_package}', it has no source revision yet"
       redirect_to :controller => :project, :action => 'new_package_branch', :project => @project and return
@@ -523,7 +524,7 @@ def save_new_link
     if @use_branch
       logger.debug "link params doing branch: #{@linked_project}, #{@linked_package}"
       begin
-        path = "/source/#{CGI.escape(@linked_project)}/#{CGI.escape(@linked_package)}?cmd=branch&target_project=#{CGI.escape(@project.name)}&target_package=#{CGI.escape(@target_package)}"
+        path = linked_package.api_obj.source_path('', { cmd: :branch, target_project: @project.name, target_package: @target_package})
         path += "&rev=#{CGI.escape(@revision)}" if @revision
         frontend.transport.direct_http( URI(path), :method => 'POST', :data => '')
         flash[:success] = "Branched package #{@project.name} / #{@target_package}"

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

@@ -16,11 +16,15 @@ def add_reviewer
     begin
       opts = {}
       case params[:review_type]
-        when 'user' then opts[:user] = params[:review_user]
-        when 'group' then opts[:group] = params[:review_group]
-        when 'project' then opts[:project] = params[:review_project]
-        when 'package' then opts[:project] = params[:review_project]
-                            opts[:package] = params[:review_package]
+        when 'user' then
+          opts[:user] = params[:review_user]
+        when 'group' then
+          opts[:group] = params[:review_group]
+        when 'project' then
+          opts[:project] = params[:review_project]
+        when 'package' then
+          opts[:project] = params[:review_project]
+          opts[:package] = params[:review_package]
       end
       opts[:comment] = params[:review_comment] if params[:review_comment]
 
@@ -83,7 +87,7 @@ def show
 
     request_list = session[:requests]
     @request_before = nil
-    @request_after  = nil
+    @request_after = nil
     index = request_list.index(@id) if request_list
     if index and index > 0
       @request_before = request_list[index-1]
@@ -103,7 +107,7 @@ def sourcediff
 
   def changerequest
     required_parameters :id
-    @req = Webui::BsRequest.find params[:id] 
+    @req = Webui::BsRequest.find params[:id]
     unless @req
       flash[:error] = "Can't find request #{params[:id]}"
       redirect_back_or_to :controller => 'home', :action => 'requests' and return
@@ -134,38 +138,46 @@ def changerequest
         end
       end
     end
-    if changestate == 'accepted'
-      flash[:notice] = "Request #{params[:id]} accepted"
-
-      # Check if we have to forward this request to other projects / packages
-      params.keys.grep(/^forward_.*/).each do |fwd|
-        tgt_prj, tgt_pkg = params[fwd].split('_#_') # split off 'forward_' and split into project and package
-        description = @req.description.text
-        if @req.has_element? 'state'
-          who = @req.state.value('who')
-          description += ' (forwarded request %d from %s)' % [params[:id], who]
-        end
 
-        rev = Webui::Package.current_rev(@req.action.target.project, @req.action.target.package)
-        req = Webui::BsRequest.new(:type => 'submit', :targetproject => tgt_prj, :targetpackage => tgt_pkg,
-                             :project => @req.action.target.project, :package => @req.action.target.package,
-                             :rev => rev, :description => description)
-        req.save(:create => true)
-        Rails.cache.delete('requests_new')
-        # link_to isn't available here, so we have to write some HTML. Uses url_for to not hardcode URLs.
-        flash[:notice] += " and forwarded to <a href='#{url_for(:controller => 'package', :action => 'show', :project => tgt_prj, :package => tgt_pkg)}'>#{tgt_prj} / #{tgt_pkg}</a> (request <a href='#{url_for(:action => 'show', :id => req.value('id'))}'>#{req.value('id')}</a>)"
-      end
+    accept_request if changestate == 'accepted'
 
-      # Cleanup prj/pkg cache after auto-removal of source projects / packages (mostly from branches).
-      # To keep things simple, we don't check if the src prj had more pkgs, etc..
-      @req.each_action do |action|
-        if action.value('type') == 'submit' and action.has_element?('options') and action.options.value('sourceupdate') == 'cleanup'
-          Rails.cache.delete("#{action.source.project}_packages_mainpage")
-          Rails.cache.delete("#{action.source.project}_problem_packages")
-        end
+    redirect_to :action => 'show', :id => params[:id]
+  end
+
+  def accept_request
+    flash[:notice] = "Request #{params[:id]} accepted"
+
+    # Check if we have to forward this request to other projects / packages
+    params.keys.grep(/^forward_.*/).each do |fwd|
+      forward_request_to(fwd)
+    end
+
+    # Cleanup prj/pkg cache after auto-removal of source projects / packages (mostly from branches).
+    # To keep things simple, we don't check if the src prj had more pkgs, etc..
+    @req.each_action do |action|
+      if action.value('type') == 'submit' and action.has_element?('options') and action.options.value('sourceupdate') == 'cleanup'
+        Rails.cache.delete("#{action.source.project}_packages_mainpage")
+        Rails.cache.delete("#{action.source.project}_problem_packages")
       end
     end
-    redirect_to :action => 'show', :id => params[:id]
+  end
+
+  def forward_request_to(fwd)
+    tgt_prj, tgt_pkg = params[fwd].split('_#_') # split off 'forward_' and split into project and package
+    description = @req.description.text
+    if @req.has_element? 'state'
+      who = @req.state.value('who')
+      description += ' (forwarded request %d from %s)' % [params[:id], who]
+    end
+
+    rev = Package.dir_hash(@req.action.target.project, @req.action.target.package)['rev']
+    req = Webui::BsRequest.new(:type => 'submit', :targetproject => tgt_prj, :targetpackage => tgt_pkg,
+                               :project => @req.action.target.project, :package => @req.action.target.package,
+                               :rev => rev, :description => description)
+    req.save(:create => true)
+    Rails.cache.delete('requests_new')
+                                                # link_to isn't available here, so we have to write some HTML. Uses url_for to not hardcode URLs.
+    flash[:notice] += " and forwarded to <a href='#{url_for(:controller => 'package', :action => 'show', :project => tgt_prj, :package => tgt_pkg)}'>#{tgt_prj} / #{tgt_pkg}</a> (request <a href='#{url_for(:action => 'show', :id => req.value('id'))}'>#{req.value('id')}</a>)"
   end
 
   def diff
@@ -174,7 +186,7 @@ def diff
   end
 
   def list
-    redirect_to :controller => :home, :action => :requests and return unless request.xhr?  # non ajax request
+    redirect_to :controller => :home, :action => :requests and return unless request.xhr? # non ajax request
     requests = Webui::BsRequest.list_ids(params)
     elide_len = 44
     elide_len = params[:elide_len].to_i if params[:elide_len]
@@ -184,7 +196,7 @@ def list
   end
 
   def list_small
-    redirect_to :controller => :home, :action => :requests and return unless request.xhr?  # non ajax request
+    redirect_to :controller => :home, :action => :requests and return unless request.xhr? # non ajax request
     requests = Webui::BsRequest.list(params)
     render :partial => 'shared/requests_small', :locals => {:requests => requests}
   end
@@ -238,11 +250,11 @@ def set_bugowner_request
     begin
       if params[:group] == 'False'
         req = Webui::BsRequest.new(:type => 'set_bugowner', :targetproject => params[:project], :targetpackage => params[:package],
-                            :person => params[:user], :description => params[:description])
+                                   :person => params[:user], :description => params[:description])
       end
       if params[:user] == 'False'
         req = Webui::BsRequest.new(:type => 'set_bugowner', :targetproject => params[:project], :targetpackage => params[:package],
-                            :group => params[:group], :description => params[:description])
+                                   :group => params[:group], :description => params[:description])
       end
       req.save(:create => true)
       Rails.cache.delete 'requests_new'
@@ -292,16 +304,16 @@ def set_incident
     redirect_to :controller => :request, :action => 'show', :id => params[:id]
   end
 
-  # used by HasComments mixin
+# used by HasComments mixin
   def comment_object
     Webui::BsRequest.find params[:id]
   end
 
-private
+  private
 
   def change_request(changestate, params)
     begin
-      if Webui::BsRequest.modify( params[:id], changestate, :reason => params[:reason], :force => true )
+      if Webui::BsRequest.modify(params[:id], changestate, :reason => params[:reason], :force => true)
         flash[:notice] = "Request #{changestate}!"
         return true
       else

src/api/webui/app/models/webui/package.rb

@@ -153,72 +153,31 @@ def serviceinfo
     @serviceinfo
   end
 
-  def self.current_xsrcmd5(project, package )
-    dir = Directory.find_hashed( :project => project, :package => package )
-    return dir['xsrcmd5']
-  end
-
-  def self.current_rev(project, package )
-    dir = Directory.find_hashed( :project => project, :package => package )
-    return dir['rev']
-  end
+  def parse_all_history
+    answer = api_obj.source_file('_history')
 
-  def cacheAllCommits
-    commit( nil, true )
-    return true
+    doc = Xmlhash.parse(answer)
+    doc.elements('revision') do |s|
+      Rails.cache.write(["history", api_obj, s['rev']], s)
+    end
   end
 
-  def commit( rev = nil, cacheAll = nil )
+  def commit( rev = nil )
     if rev and rev.to_i < 0
       # going backward from not yet known current revision, find out ...
-      r = Package.current_rev(project, name).to_i + rev.to_i + 1
+      r = api_obj.rev.to_i + rev.to_i + 1
       rev = r.to_s
       return nil if rev.to_i < 1
     end
-    rev = Package.current_rev(project, name) unless rev
-
-    cache_key = nil
-    if rev and not cacheAll
-      path = "/source/#{CGI.escape(project)}/#{CGI.escape(name)}/_history?rev=#{CGI.escape(rev)}"
-      cache_key = "Commit/#{project}/#{name}/#{rev}"
-      c = Rails.cache.read(cache_key, :expires_in => 30.minutes)
-      if c
-        return c
-      end
-    else
-      path = "/source/#{CGI.escape(project)}/#{CGI.escape(name)}/_history"
-    end
-
+    rev ||= api_obj.rev
 
-    frontend = ActiveXML::api
-    begin
-      answer = frontend.direct_http URI(path), :method => 'GET'
-    rescue
-      return nil
-    end
-
-    c = {}
-    doc = ActiveXML::Node.new(answer)
-    doc.each('/revisionlist/revision') do |s|
-         c[:revision]= s.value('rev')
-         c[:user]    = s.find_first('user').text
-         c[:version] = s.find_first('version').text
-         c[:time]    = s.find_first('time').text
-         c[:srcmd5]  = s.find_first('srcmd5').text
-         c[:comment] = nil
-         c[:requestid] = nil
-         comment=s.find_first('comment')
-         if comment
-           c[:comment] = comment.text
-         end
-         requestid=s.find_first('requestid')
-         if requestid
-           c[:requestid] = requestid.text
-         end
-    end
+    cache_key = ["history", api_obj, rev]
+    c = Rails.cache.read(cache_key)
+    return c if c
 
-    return nil unless c[:revision]
-    return c
+    parse_all_history
+    # now it has to be in cache
+    Rails.cache.read(cache_key)
   end
 
   def files( rev = nil, expand = nil )