Allow groups to access queries (#36)

* [WIP] group ids saving on new reports

* Add groups to default queries, and added tab connector

* group_ids set to empty array for default queries

* group reports route (in & and) action

* [WIP] created group reports show route/controller

* Find correct query in show route

* Removed empty array for group_ids in query file

* Add report show view, where users can run queries

* Removed unneeded commas from queries.rb

* Allow non-admin group members to access reports

* query-result component dynamic download url based on location

* Removed accidental changes, and corrected tab size

* Group members can add params to queries

* Specs for new QueryController actions

* remove "Inlude query plan" from group reports

* Run prettier

* return and return -> return render

Co-Authored-By: Robin Ward <robin.ward@gmail.com>

* [WIP] changes from review

* Remove weird [-1] group_ids logic, for a simply check for [] in query update action

* Added integration tests for group report access

* Using guardian for securing endpoints, and much improved specs

* Update assets/javascripts/discourse/components/group-reports-nav-item.js.es6

Co-Authored-By: Robin Ward <robin.ward@gmail.com>
This commit is contained in:
Mark VanLandingham 2019-09-11 09:09:41 -05:00 committed by Robin Ward
parent 677722d676
commit 30fe9289b8
21 changed files with 817 additions and 215 deletions

View File

@ -0,0 +1,21 @@
import { ajax } from "discourse/lib/ajax";
export default Ember.Component.extend({
group: null,
showReportsTab: false,
checkForReports() {
return ajax(`/g/${this.group.name}/reports`).then(response => {
return this.set("showReportsTab", response.queries.length > 0);
});
},
init(args) {
this.set("group", args.group);
if (this.currentUser.groups.some(g => g.id === this.group.id)) {
// User is a part of the group. Now check if the group has reports
this.checkForReports();
}
this._super(args);
}
});

View File

@ -146,6 +146,12 @@ const QueryResultComponent = Ember.Component.extend({
return this.site.get("categoriesById")[id];
},
download_url() {
return this.group
? `/g/${this.group.name}/reports/`
: "/admin/plugins/explorer/queries/";
},
downloadResult(format) {
// Create a frame to submit the form in (?)
// to avoid leaving an about:blank behind
@ -161,7 +167,7 @@ const QueryResultComponent = Ember.Component.extend({
form.setAttribute(
"action",
Discourse.getURL(
"/admin/plugins/explorer/queries/" +
this.download_url() +
this.get("query.id") +
"/run." +
format +

View File

@ -54,6 +54,22 @@ export default Ember.Controller.extend({
return item || NoQuery;
},
@computed("selectedItem", "editing")
selectedGroupNames(selectedItem) {
const groupIds = this.selectedItem.group_ids || [];
const groupNames = groupIds.map(id => {
return this.groupOptions.find(groupOption => groupOption.id == id).name;
});
return groupNames.join(", ");
},
@computed("groups")
groupOptions(groups) {
return groups.arrangedContent.map(g => {
return { id: g.id.toString(), name: g.name };
});
},
@computed("selectedItem", "selectedItem.dirty")
othersDirty(selectedItem) {
return !!this.model.find(q => q !== selectedItem && q.dirty);
@ -81,6 +97,7 @@ export default Ember.Controller.extend({
this.set("loading", true);
if (this.get("selectedItem.description") === "")
this.set("selectedItem.description", "");
return this.selectedItem
.save()
.then(() => {
@ -183,6 +200,8 @@ export default Ember.Controller.extend({
.then(result => {
const query = this.get("selectedItem");
query.setProperties(result.getProperties(Query.updatePropertyNames));
if (!query.group_ids || !Array.isArray(query.group_ids))
query.set("group_ids", []);
query.markNotDirty();
this.set("editing", false);
})

View File

@ -0,0 +1,10 @@
import Query from "discourse/plugins/discourse-data-explorer/discourse/models/query";
import { ajax } from "discourse/lib/ajax";
import {
default as computed,
observes
} from "ember-addons/ember-computed-decorators";
export default Ember.Controller.extend({
queries: Ember.computed.alias("model.queries")
});

View File

@ -0,0 +1,44 @@
import Query from "discourse/plugins/discourse-data-explorer/discourse/models/query";
import { popupAjaxError } from "discourse/lib/ajax-error";
import { ajax } from "discourse/lib/ajax";
import {
default as computed,
observes
} from "ember-addons/ember-computed-decorators";
export default Ember.Controller.extend({
showResults: false,
explain: false,
loading: false,
results: Ember.computed.alias("model.results"),
hasParams: Ember.computed.gt("model.param_info.length", 0),
actions: {
run() {
this.setProperties({ loading: true, showResults: false });
ajax(`/g/${this.get("group.name")}/reports/${this.model.id}/run`, {
type: "POST",
data: {
params: JSON.stringify(this.model.params),
explain: this.explain
}
})
.then(result => {
this.set("results", result);
if (!result.success) {
return;
}
this.set("showResults", true);
})
.catch(err => {
if (err.jqXHR && err.jqXHR.status === 422 && err.jqXHR.responseJSON) {
this.set("results", err.jqXHR.responseJSON);
} else {
popupAjaxError(err);
}
})
.finally(() => this.set("loading", false));
}
}
});

View File

@ -0,0 +1,9 @@
export default {
resource: "group",
map() {
this.route("reports", function() {
this.route("show", { path: "/:query_id" });
});
}
};

View File

@ -23,7 +23,7 @@ const Query = RestModel.extend({
this.resetParams();
},
@observes("name", "description", "sql")
@observes("name", "description", "sql", "group_ids")
markDirty() {
this.set("dirty", true);
},
@ -85,6 +85,7 @@ Query.reopenClass({
"sql",
"created_by",
"created_at",
"group_ids",
"last_run_at"
]
});

View File

@ -4,19 +4,36 @@ export default Discourse.Route.extend({
controllerName: "admin-plugins-explorer",
model() {
const p1 = this.store.findAll("query");
const p2 = ajax("/admin/plugins/explorer/schema.json", { cache: true });
return p1
.then(model => {
model.forEach(query => query.markNotDirty());
const groupPromise = this.store.findAll("group");
const schemaPromise = ajax("/admin/plugins/explorer/schema.json", { cache: true });
const queryPromise = this.store.findAll("query");
return p2.then(schema => {
return { model, schema };
return groupPromise
.then(groups => {
let groupNames = {};
groups.forEach(g => {
groupNames[g.id] = g.name;
});
return schemaPromise.then(schema => {
return queryPromise.then(model => {
model.forEach(query => {
query.markNotDirty();
query.set(
"group_names",
query.group_ids
.map(id => groupNames[id])
.filter(n => n)
.join(", ")
);
});
return { model, schema, groups };
});
});
})
.catch(() => {
p2.catch(() => {});
return { model: null, schema: null, disallow: true };
schemaPromise.catch(() => {});
queryPromise.catch(() => {});
return { model: null, schema: null, disallow: true, groups: null };
});
},

View File

@ -0,0 +1,38 @@
import { ajax } from "discourse/lib/ajax";
export default Discourse.Route.extend({
controllerName: "group-reports-index",
model() {
const group = this.modelFor("group");
return ajax(`/g/${group.name}/reports`)
.then(queries => {
return {
model: queries,
group: group
};
})
.catch(() => {
this.transitionTo("group.members", group);
});
},
afterModel(model) {
if (
!model.group.get("is_group_user") &&
!(this.currentUser && this.currentUser.admin)
) {
this.transitionTo("group.members", group);
}
},
setupController(controller, model) {
controller.setProperties(model);
},
actions: {
refreshModel() {
this.refresh();
return false;
}
}
});

View File

@ -0,0 +1,30 @@
import { ajax } from "discourse/lib/ajax";
export default Discourse.Route.extend({
controllerName: "group-reports-show",
model(params) {
const group = this.modelFor("group");
return ajax(`/g/${group.name}/reports/${params.query_id}`)
.then(response => {
return {
model: Object.assign({ params: {} }, response.query),
group: group
};
})
.catch(err => {
this.transitionTo("group.members", group);
});
},
setupController(controller, model) {
controller.setProperties(model);
},
actions: {
refreshModel() {
this.refresh();
return false;
}
}
});

View File

@ -38,6 +38,7 @@
<div class="desc">
{{textarea value=selectedItem.description placeholder=(i18n "explorer.description_placeholder")}}
</div>
{{else}}
<div class="name">
{{d-button action=(action "goHome") icon="chevron-left" class="previous"}}
@ -52,6 +53,20 @@
</div>
{{/if}}
<div class="groups">
<span class="label">Allow groups to acess this query</span>
<span>
{{multi-select values=selectedItem.group_ids content=groupOptions}}
</span>
{{#if runDisabled}}
{{#unless editing}}
<span class='setting-controls'>
{{d-button class="ok" action=(action "save") icon="check"}}
{{d-button class="cancel" action=(action "discard") icon="times"}}
</span>
{{/unless}}
{{/if}}
</div>
{{! the SQL editor will show the first time you }}
{{#if everEditing}}
<div class="query-editor {{if hideSchema "no-schema"}}">
@ -158,6 +173,11 @@
{{directory-toggle field="username" labelKey="explorer.query_user" order=order asc=asc}}
</div>
</th>
<th class='col heading group-names'>
<div class='group-names-header'>
{{i18n "explorer.query_groups"}}
</div>
</th>
<th class="col heading created-at">
<div class="heading-toggle" {{action "sortByProperty" "last_run_at"}}>
{{directory-toggle field="last_run_at" labelKey="explorer.query_time" order=order asc=asc}}
@ -181,6 +201,11 @@
</a>
{{/if}}
</td>
<td class="query-group-names">
{{#if query.group_names}}
<medium>{{query.group_names}}</medium>
{{/if}}
</td>
<td class="query-created-at">
{{#if query.last_run_at}}
<medium>

View File

@ -0,0 +1,9 @@
{{#if showReportsTab}}
<ul class ='nav-pills'>
<li>
{{#link-to 'group.reports'}}
{{i18n 'group.reports'}}
{{/link-to}}
</li>
</ul>
{{/if}}

View File

@ -0,0 +1 @@
{{group-reports-nav-item group = group}}

View File

@ -1,6 +1,6 @@
<div class="result-info">
{{d-button action=(action "downloadResultJson") icon="download" label="explorer.download_json"}}
{{d-button action=(action "downloadResultCsv") icon="download" label="explorer.download_csv"}}
{{d-button action=(action "downloadResultJson") icon="download" label="explorer.download_json" group=group}}
{{d-button action=(action "downloadResultCsv") icon="download" label="explorer.download_csv" group=group}}
</div>
<div class="result-about">

View File

@ -0,0 +1,27 @@
<section class='user-content'>
<table class='group-reports'>
<thead>
<th>
{{i18n "explorer.report_name"}}
</th>
<th>
{{i18n "explorer.query_description"}}
</th>
<th>
{{i18n "explorer.query_time"}}
</th>
</thead>
<tr></tr>
<tbody>
{{#each queries as |query|}}
<tr>
<td>
{{#link-to 'group.reports.show' group.name query.id}}{{query.name}}{{/link-to}}
</td>
<td>{{query.description}}</td>
<td>{{bound-date query.last_run_at}}</td>
</tr>
{{/each}}
</tbody>
</table>
</section>

View File

@ -0,0 +1,26 @@
<section class='user-content'>
<h1>{{model.name}}</h1>
<p>{{model.description}}</p>
<form class="query-run" {{action "run" on="submit"}}>
{{#if hasParams}}
<div class="query-params">
{{#each model.param_info as |pinfo|}}
{{param-input params=model.params info=pinfo}}
{{/each}}
</div>
{{/if}}
{{d-button action=(action "run") icon="play" label="explorer.run" class="btn-primary" type="submit"}}
</form>
{{conditional-loading-spinner condition=loading}}
{{#if results}}
<div class="query-results">
{{#if showResults}}
{{query-result query=model content=results group=group}}
{{else}}
{{#each results.errors as |err|}}
<pre class="query-error"><code>{{~err}}</code></pre>
{{/each}}
{{/if}}
</div>
{{/if}}
</section>

View File

@ -1,3 +1,39 @@
table.group-reports {
width: 100%;
table-layout: fixed;
th:first-child {
width: 30%;
}
th:nth-child(2) {
width: 60%;
}
th:last-child {
width: 20%;
text-align: right;
}
tbody {
border-top: 3px solid #e3ebf2;
tr {
border-bottom: 1px solid #e3ebf2;
td {
color: #7499bd;
padding: 0.8em 0.5em;
}
td:first-child {
font-size: 16px;
}
td:last-child {
text-align: right;
}
}
}
}
.https-warning {
color: $danger;
}
@ -185,6 +221,16 @@
&:not(.editing) .desc {
margin: 10px 0;
}
.groups {
margin-bottom: 10px;
.label {
margin-right: 10px;
color: $primary-high;
}
.name {
display: inline;
}
}
}
.query-run {
@ -244,6 +290,16 @@
margin: 10px 0;
}
.query-results {
table {
width: 100%;
margin-top: 10px;
td {
padding: 8px;
}
}
}
.query-list {
display: flex;
max-height: 30px;
@ -262,7 +318,15 @@
.recent-queries {
thead {
.created-by {
width: 20%;
width: 15%;
}
.group-names {
width: 15%;
.group-names-header {
position: absolute;
bottom: 8px;
left: 6px;
}
}
.created-at {
width: 15%;
@ -295,6 +359,9 @@
.query-created-by {
color: $primary-high;
}
.query-group-names {
color: $primary-high;
}
.query-created-at {
color: $primary-medium;
}

View File

@ -59,6 +59,8 @@ en:
one: "Showing top %{count} results."
other: "Showing top %{count} results."
query_name: "Query"
query_groups: "Groups"
report_name: "Report"
query_description: "Description"
query_time: "Last run"
query_user: "Created by"
@ -68,3 +70,6 @@ en:
reset_params: "Reset"
search_placeholder: "Search..."
no_search_results: "Sorry, we couldn't find any results matching your text."
group:
reports: "Reports"

View File

@ -44,6 +44,19 @@ end
after_initialize do
add_to_class(:guardian, :user_is_a_member_of_group?) do |group|
return false if !current_user
return true if current_user.admin?
return current_user.group_ids.include?(group.id)
end
add_to_class(:guardian, :user_can_access_query?) do |group, query|
return false if !current_user
return true if current_user.admin?
return user_is_a_member_of_group?(group) &&
query.group_ids.include?(group.id.to_s)
end
module ::DataExplorer
class Engine < ::Rails::Engine
engine_name "data_explorer"
@ -595,12 +608,13 @@ SQL
# Reimplement a couple ActiveRecord methods, but use PluginStore for storage instead
require_dependency File.expand_path('../lib/queries.rb', __FILE__)
class DataExplorer::Query
attr_accessor :id, :name, :description, :sql, :created_by, :created_at, :last_run_at
attr_accessor :id, :name, :description, :sql, :created_by, :created_at, :group_ids, :last_run_at
def initialize
@name = 'Unnamed Query'
@description = ''
@sql = 'SELECT 1'
@group_ids = []
end
def slug
@ -624,6 +638,10 @@ SQL
result
end
def can_be_run_by(group)
@group_ids.include?(group.id.to_s)
end
# saving/loading functions
# May want to extract this into a library or something for plugins to use?
def self.alloc_id
@ -640,6 +658,7 @@ SQL
[:name, :description, :sql, :created_by, :created_at, :last_run_at].each do |sym|
query.send("#{sym}=", h[sym].strip) if h[sym]
end
query.group_ids = h[:group_ids]
query.id = h[:id].to_i if h[:id]
query
end
@ -652,6 +671,7 @@ SQL
sql: @sql,
created_by: @created_by,
created_at: @created_at,
group_ids: @group_ids,
last_run_at: @last_run_at
}
end
@ -672,7 +692,9 @@ SQL
def save
check_params!
@id = self.class.alloc_id unless @id && @id > 0
return save_default_query if @id && @id < 0
@id = @id ||self.class.alloc_id
DataExplorer.pstore_set "q:#{id}", to_hash
end
@ -682,6 +704,7 @@ SQL
query = Queries.default[id.to_s]
@id = query["id"]
@sql = query["sql"]
@group_ids = @group_ids || []
@name = query["name"]
@description = query["description"]
@ -958,11 +981,23 @@ SQL
requires_plugin DataExplorer.plugin_name
before_action :check_enabled
before_action :set_group, only: [:group_reports_index, :group_reports_show, :group_reports_run]
before_action :set_query, only: [:group_reports_show, :group_reports_run]
attr_reader :group, :query
def check_enabled
raise Discourse::NotFound unless SiteSetting.data_explorer_enabled?
end
def set_group
@group = Group.find_by(name: params["group_name"])
end
def set_query
@query = DataExplorer::Query.find(params[:id].to_i)
end
def index
# guardian.ensure_can_use_data_explorer!
queries = DataExplorer::Query.all
@ -996,6 +1031,36 @@ SQL
render_serialized query, DataExplorer::QuerySerializer, root: 'query'
end
def group_reports_index
return raise Discourse::NotFound unless guardian.user_is_a_member_of_group?(group)
respond_to do |format|
format.html { render 'groups/show' }
format.json do
queries = DataExplorer::Query.all
queries.select! { |query| query.group_ids.include?(group.id.to_s) }
render_serialized queries, DataExplorer::QuerySerializer, root: 'queries'
end
end
end
def group_reports_show
return raise Discourse::NotFound unless guardian.user_can_access_query?(group, query)
respond_to do |format|
format.html { render 'groups/show' }
format.json do
render_serialized query, DataExplorer::QuerySerializer, root: 'query'
end
end
end
def group_reports_run
return raise Discourse::NotFound unless guardian.user_can_access_query?(group, query)
run
end
def create
# guardian.ensure_can_create_explorer_query!
@ -1012,6 +1077,7 @@ SQL
def update
query = DataExplorer::Query.find(params[:id].to_i, ignore_deleted: true)
hash = params.require(:query)
hash[:group_ids] ||= []
# Undeleting
unless query.id
@ -1022,7 +1088,7 @@ SQL
end
end
[:name, :sql, :description, :created_by, :created_at, :last_run_at].each do |sym|
[:name, :sql, :description, :created_by, :created_at, :group_ids, :last_run_at].each do |sym|
query.send("#{sym}=", hash[sym]) if hash[sym]
end
@ -1157,7 +1223,7 @@ SQL
end
class DataExplorer::QuerySerializer < ActiveModel::Serializer
attributes :id, :sql, :name, :description, :param_info, :created_by, :created_at, :username, :last_run_at
attributes :id, :sql, :name, :description, :param_info, :created_by, :created_at, :username, :group_ids, :last_run_at
def param_info
object.params.map(&:to_hash) rescue nil
@ -1180,6 +1246,10 @@ SQL
end
Discourse::Application.routes.append do
get '/g/:group_name/reports' => 'data_explorer/query#group_reports_index'
get '/g/:group_name/reports/:id' => 'data_explorer/query#group_reports_show'
post '/g/:group_name/reports/:id/run' => 'data_explorer/query#group_reports_run'
mount ::DataExplorer::Engine, at: '/admin/plugins/explorer', constraints: AdminConstraint.new
end
end

View File

@ -3,8 +3,6 @@
require 'rails_helper'
describe DataExplorer::QueryController do
routes { ::DataExplorer::Engine.routes }
def response_json
MultiJson.load(response.body)
end
@ -13,18 +11,22 @@ describe DataExplorer::QueryController do
SiteSetting.data_explorer_enabled = true
end
let!(:admin) { log_in_user(Fabricate(:admin)) }
def make_query(sql, opts = {})
def make_query(sql, opts = {}, group_ids = [])
q = DataExplorer::Query.new
q.id = Fabrication::Sequencer.sequence("query-id", 1)
q.name = opts[:name] || "Query number #{q.id}"
q.description = "A description for query number #{q.id}"
q.group_ids = group_ids
q.sql = sql
q.save
q
end
describe "Admin" do
routes { ::DataExplorer::Engine.routes }
let!(:admin) { log_in_user(Fabricate(:admin)) }
describe "when disabled" do
before do
SiteSetting.data_explorer_enabled = false
@ -247,4 +249,117 @@ describe DataExplorer::QueryController do
expect(response.status).to eq(200)
end
end
end
describe "Non-Admin" do
routes { Discourse::Application.routes }
let(:user) { Fabricate(:user) }
let(:group) { Fabricate(:group, users: [user]) }
before do
log_in_user(user)
end
describe "when disabled" do
before do
SiteSetting.data_explorer_enabled = false
end
it 'denies every request' do
get :group_reports_index, params: { group_name: 1 }, format: :json
expect(response.status).to eq(404)
get :group_reports_show, params: { group_name: 1, id: 1 }, format: :json
expect(response.status).to eq(404)
post :group_reports_run, params: { group_name: 1, id: 1 }, format: :json
expect(response.status).to eq(404)
end
end
describe "#group_reports_index" do
it "only returns queries that the group has access to" do
group.add(user)
make_query('SELECT 1 as value', {name: 'A'}, ["#{group.id}"])
get :group_reports_index, params: { group_name: group.name }, format: :json
expect(response.status).to eq(200)
expect(response_json['queries'].length).to eq(1)
expect(response_json['queries'][0]['name']).to eq('A')
end
it "returns a 404 when the user should not have access to the query " do
user = Fabricate(:user)
log_in_user(user)
get :group_reports_index, params: { group_name: group.name }, format: :json
expect(response.status).to eq(404)
end
it "return a 200 when the user has access the the query" do
user = Fabricate(:user)
log_in_user(user)
group.add(user)
get :group_reports_index, params: { group_name: group.name }, format: :json
expect(response.status).to eq(200)
end
end
describe "#group_reports_run" do
it "calls run on QueryController" do
query = make_query('SELECT 1 as value', {name: 'B'}, ["#{group.id}"])
controller.expects(:run).at_least_once
get :group_reports_run, params: { group_name: group.name, id: query.id }, format: :json
end
it "returns a 404 when the user should not have access to the query " do
user = Fabricate(:user)
log_in_user(user)
group.add(user)
query = make_query('SELECT 1 as value', {}, [])
get :group_reports_run, params: { group_name: group.name, id: query.id }, format: :json
expect(response.status).to eq(404)
end
it "return a 200 when the user has access the the query" do
user = Fabricate(:user)
log_in_user(user)
group.add(user)
query = make_query('SELECT 1 as value', {}, [group.id.to_s])
get :group_reports_run, params: { group_name: group.name, id: query.id }, format: :json
expect(response.status).to eq(200)
end
end
describe "#group_reports_show" do
let(:group) { Fabricate(:group) }
it "returns a 404 when the user should not have access to the query " do
user = Fabricate(:user)
log_in_user(user)
group.add(user)
query = make_query('SELECT 1 as value', {}, [])
get :group_reports_show, params: { group_name: group.name, id: query.id }, format: :json
expect(response.status).to eq(404)
end
it "return a 200 when the user has access the the query" do
user = Fabricate(:user)
log_in_user(user)
group.add(user)
query = make_query('SELECT 1 as value', {}, [group.id.to_s])
get :group_reports_show, params: { group_name: group.name, id: query.id }, format: :json
expect(response.status).to eq(200)
end
end
end
end

62
spec/guardian_spec.rb Normal file
View File

@ -0,0 +1,62 @@
require 'rails_helper'
describe Guardian do
def make_query(group_ids = [])
q = DataExplorer::Query.new
q.id = Fabrication::Sequencer.sequence("query-id", 1)
q.name ="Query number #{q.id}"
q.sql = "SELECT 1"
q.group_ids = group_ids
q.save
q
end
let(:user) { build(:user) }
let(:admin) { build(:admin) }
describe "#user_is_a_member_of_group?" do
let(:group) { Fabricate(:group) }
it "is true when the user is an admin" do
expect(Guardian.new(admin).user_is_a_member_of_group?(group)).to eq(true)
end
it "is true when the user is not an admin, but is a member of the group" do
group.add(user)
expect(Guardian.new(user).user_is_a_member_of_group?(group)).to eq(true)
end
it "is false when the user is not an admin, and is not a member of the group" do
expect(Guardian.new(user).user_is_a_member_of_group?(group)).to eq(false)
end
end
describe "#user_can_access_query?" do
let(:group) { Fabricate(:group) }
it "is true if the user is an admin" do
expect(Guardian.new(admin).user_can_access_query?(group, make_query)).to eq(true)
end
it "is true if the user is a member of the group, and query contains the group id" do
query = make_query(["#{group.id}"])
group.add(user)
expect(Guardian.new(user).user_can_access_query?(group, query)).to eq(true)
end
it "is false if the query does not contain the group id" do
group.add(user)
expect(Guardian.new(user).user_can_access_query?(group, make_query)).to eq(false)
end
it "is false if the user is not member of the group" do
query = make_query(["#{group.id}"])
expect(Guardian.new(user).user_can_access_query?(group, query)).to eq(false)
end
end
end