Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sharing embeds with safe parameters #3495

Merged
merged 61 commits into from
Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
3535dfd
change has_access and require_access signatures to work with the obje…
Mar 19, 2019
fc02fe7
change has_access and require_access signatures to work with the obje…
Mar 19, 2019
eef39de
use the textless endpoint (/api/queries/:id/results) for pristine
Jan 30, 2019
3c1ed5c
Revert "use the textless endpoint (/api/queries/:id/results) for pris…
Feb 3, 2019
a3c543c
go to textless /api/queries/:id/results by default
Feb 3, 2019
b99f114
change `run_query`'s signature to accept a ParameterizedQuery instead of
Feb 3, 2019
2c48d36
raise HTTP 400 when receiving invalid parameter values. Fixes #3394
Feb 6, 2019
d03d9e1
support querystring params
Feb 25, 2019
637e985
extract coercing of numbers to function, along with a friendlier
Feb 26, 2019
8776b40
wire embeds to textless endpoint
Feb 26, 2019
7d9daa7
allow users with view_only permissions to execute queries on the
Feb 26, 2019
1eeeef8
enqueue jobs for ApiUsers
Feb 26, 2019
475f6f8
add parameters component for embeds
Feb 26, 2019
ca9e665
include existing parameters in embed code
Feb 26, 2019
82ff905
fetch correct values for json requests
Feb 27, 2019
f4c22ca
remove previous embed parameter code
Feb 27, 2019
18c1876
rename `id` to `user_id`
Mar 3, 2019
7542ece
support executing queries using Query api_keys by instantiating an Ap…
Mar 4, 2019
0eb1963
bring back ALLOW_PARAMETERS_IN_EMBEDS (with link on deprecation comin…
Mar 4, 2019
18d5b94
show deprecation messages for ALLOW_PARAMETERS_IN_EMBEDS. Also, move
Mar 4, 2019
54d7d82
add link to forum message on setting deprecation
Mar 4, 2019
34f7762
rephrase deprecation message
Mar 4, 2019
96d56ee
add link to forum message regarding embed deprecation
Mar 5, 2019
e19517c
change API to /api/queries/:id/dropdowns/:dropdown_id
Mar 6, 2019
ef286a7
split to 2 different dropdown endpoints and implement the second
Mar 6, 2019
87a9030
add test cases for /api/queries/:id/dropdowns/:id
Mar 6, 2019
b279b7a
use new /dropdowns endpoint in frontend
Mar 7, 2019
aa59a89
first e2e test for sharing embeds
Mar 11, 2019
9cc2de5
Pleasing the CodeClimate overlords
Mar 11, 2019
0fce9c3
All glory to CodeClimate
Mar 18, 2019
a40bc29
change has_access and require_access signatures to work with the obje…
Mar 19, 2019
353bb9f
split has_access between normal users and ApiKey users
Mar 24, 2019
e95546b
remove residues from bad rebase
Mar 24, 2019
735a829
allow access to safe queries via api keys
Mar 24, 2019
55eb7ce
Merge branch 'change-has-access-signature' into allow-parameters-on-e…
Mar 24, 2019
5ef2031
rename `object` to `obj`
Mar 27, 2019
1f16998
support both objects and group dicts in `has_access` and `require_acc…
Mar 27, 2019
029c395
simplify permission tests once `has_access` accepts groups
Mar 27, 2019
0ff0cc9
Merge branch 'change-has-access-signature' into allow-parameters-on-e…
Mar 27, 2019
ff4dfbb
change has_access and require_access signatures to work with the obje…
Mar 19, 2019
71452cd
rename `object` to `obj`
Mar 27, 2019
371c950
support both objects and group dicts in `has_access` and `require_acc…
Mar 27, 2019
90a8eac
simplify permission tests once `has_access` accepts groups
Mar 27, 2019
9c6ce65
Merge branch 'change-has-access-signature' into allow-parameters-on-e…
Mar 28, 2019
c88d58a
fix bad rebase
Mar 28, 2019
818fc27
send embed parameters through POST data
Mar 28, 2019
5770888
no need to log `is_api_key`
Mar 28, 2019
c860314
Merge branch 'master' into allow-parameters-on-embeds
Mar 29, 2019
2c78937
move query fetching by api_key to within the Query model
Mar 29, 2019
34404b4
fetch user by adding a get_by_id function on the User model
Mar 29, 2019
3d24835
pass parameters as POST data (fixes test failure introduced by switching
Mar 29, 2019
66251fd
test the right thing - queries with safe parameters should be embeddable
Mar 31, 2019
4a32cdb
introduce cy.clickThrough
Mar 31, 2019
304b178
add another Cypress test to make sure unsafe queries cannot be embedded
Mar 31, 2019
17636ec
serialize Parameters into query string
Mar 31, 2019
16a8a40
set is_api_key as the last parameter to (hopefully) avoid
Apr 1, 2019
1e7bd32
Update redash/models/parameterized_query.py
arikfr Apr 1, 2019
2efc9da
Merge branch 'master' into allow-parameters-on-embeds
Apr 1, 2019
8f81978
attempt to fix empty percy snapshots
Apr 1, 2019
e912010
snap percies after DOM is fully loaded
Apr 2, 2019
64f1170
Merge branch 'allow-parameters-on-embeds' of github.com:getredash/red…
Apr 2, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions client/app/components/EditParameterSettingsDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ function EditParameterSettingsDialog(props) {
footer={[(
<Button key="cancel" onClick={props.dialog.dismiss}>Cancel</Button>
), (
<Button key="submit" htmlType="submit" disabled={!isFulfilled()} type="primary" form="paramForm">
<Button key="submit" htmlType="submit" disabled={!isFulfilled()} type="primary" form="paramForm" data-test="SaveParameterSettings">
{isNew ? 'Add Parameter' : 'OK'}
</Button>
)]}
Expand All @@ -153,9 +153,9 @@ function EditParameterSettingsDialog(props) {
/>
</Form.Item>
<Form.Item label="Type" {...formItemProps}>
<Select value={param.type} onChange={type => setParam({ ...param, type })}>
<Option value="text">Text</Option>
<Option value="number">Number</Option>
<Select value={param.type} onChange={type => setParam({ ...param, type })} data-test="ParameterTypeSelect">
<Option value="text" data-test="TextParameterTypeOption">Text</Option>
<Option value="number" data-test="NumberParameterTypeOption">Number</Option>
<Option value="enum">Dropdown List</Option>
<Option value="query">Query Based Dropdown List</Option>
<Option disabled key="dv1">
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/ParameterValueInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export class ParameterValueInput extends React.Component {
<Input
className={'form-control ' + className}
defaultValue={value || ''}
data-test="TextParamInput"
onChange={event => onSelect(event.target.value)}
/>
);
Expand Down Expand Up @@ -179,7 +180,6 @@ export default function init(ngModule) {
parameter="$ctrl.param"
enum-options="$ctrl.param.enumOptions"
query-id="$ctrl.param.queryId"
parent-query-id="$ctrl.param.parentQueryId"
on-select="$ctrl.setValue"
></parameter-value-input-impl>
`,
Expand Down
8 changes: 7 additions & 1 deletion client/app/components/QueryEditor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,13 @@ class QueryEditor extends React.Component {
</select>
{this.props.canEdit ? (
<Tooltip placement="top" title={modKey + ' + S'}>
<button type="button" className="btn btn-default m-l-5" onClick={this.props.saveQuery} title="Save">
<button
type="button"
className="btn btn-default m-l-5"
onClick={this.props.saveQuery}
data-test="SaveButton"
title="Save"
>
<span className="fa fa-floppy-o" />
<span className="hidden-xs m-l-5">Save</span>
{this.props.isDirty ? '*' : null}
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/app-view/template.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<app-header ng-if="$ctrl.layout.showHeader"></app-header>
<div ng-if="$ctrl.handler.error" class="fixed-container">
<div ng-if="$ctrl.handler.error" class="fixed-container" data-test="ErrorMessage">
<div class="container">
<div class="col-md-8 col-md-push-2">
<div class="error-state bg-white tiled">
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/parameters.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
>
<div class="form-group m-r-10" ng-repeat="param in parameters" data-test="ParameterName{{ param.name }}">
<label class="parameter-label">{{param.title}}</label>
<button class="btn btn-default btn-xs" ng-if="editable" ng-click="showParameterSettings(param, $index)">
<button class="btn btn-default btn-xs" ng-if="editable" ng-click="showParameterSettings(param, $index)" data-test="ParameterSettings-{{ param.name }}">
<i class="zmdi zmdi-settings"></i>
</button>
<parameter-value-input param="param"></parameter-value-input>
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/queries/embed-code-dialog.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ <h4 class="modal-title">Embed Code</h4>
<div class="modal-body">
<h5>IFrame Embed</h5>
<div>
<code>&lt;iframe src="{{ $ctrl.embedUrl }}" width="720" height="391"&gt;&lt;/iframe&gt;</code>
<code data-test="EmbedIframe">&lt;iframe src="{{ $ctrl.embedUrl }}" width="720" height="391"&gt;&lt;/iframe&gt;</code>
</div>
<span class="text-muted">(height should be adjusted)</span>
<div ng-if="$ctrl.snapshotUrl">
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/queries/embed-code-dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const EmbedCodeDialog = {

this.embedUrl = `${clientConfig.basePath}embed/query/${this.query.id}/visualization/${
this.visualization.id
}?api_key=${this.query.api_key}`;
}?api_key=${this.query.api_key}&${this.query.getParameters().toUrlParams()}`;
if (window.snapshotUrlBuilder) {
this.snapshotUrl = window.snapshotUrlBuilder(this.query, this.visualization);
}
Expand Down
6 changes: 5 additions & 1 deletion client/app/components/queries/visualization-embed.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div class="tile m-l-10 m-r-10 embed__vis">
<div class="tile m-l-10 m-r-10 embed__vis" data-test="VisualizationEmbed">
<div class="embed-heading p-t-10 p-b-10 p-r-15 p-l-15">
<h3>
<img ng-src="{{$ctrl.logoUrl}}" style="height: 24px;"/>
Expand All @@ -12,6 +12,10 @@ <h3>
</div>

<div class="col-md-12 query__vis">
<div class="p-t-15 p-b-5" ng-if="$ctrl.query.getParametersDefs().length > 0">
<parameters parameters="$ctrl.query.getParametersDefs()"></parameters>
</div>

<visualization-renderer visualization="$ctrl.visualization" query-result="$ctrl.queryResult" class="t-body">
</visualization-renderer>
</div>
Expand Down
6 changes: 5 additions & 1 deletion client/app/components/queries/visualization-embed.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ const VisualizationEmbed = {
},
};

const queryStringAsObject = () => location.search.slice(1).split('&').map(p => p.split('='))
.reduce((obj, [key, value]) => ({ ...obj, [key.replace(/^p_/, '')]: value }), {});
Copy link
Member

@arikfr arikfr Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have code in that takes parameters from the query string, we should reuse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help me locate the piece of code you are mentioning? I did find similar behavior when constructing a Parameters instance, but that is also bound to the actual query text, and doesn't seem to have the built-in ability to convert to a simple key-value JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess we need to consolidate this. As a quick fix there's this, but we should really have this logic in one place. Could you let me know which piece of code you were mentioning in this comment?


export default function init(ngModule) {
ngModule.component('visualizationEmbed', VisualizationEmbed);

Expand All @@ -37,7 +40,8 @@ export default function init(ngModule) {
return session($http, $route, Auth).then(() => {
const queryId = $route.current.params.queryId;
const query = $http.get(`api/queries/${queryId}`).then(response => response.data);
const queryResult = $http.get(`api/queries/${queryId}/results.json${location.search}`).then(response => response.data);
const { api_key: apiKey, ...parameters } = queryStringAsObject();
const queryResult = $http.post(`api/queries/${queryId}/results?api_key=${apiKey}`, { parameters }).then(response => response.data);
return $q.all([query, queryResult]);
});
}
Expand Down
5 changes: 4 additions & 1 deletion client/app/pages/home/home.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
<div class="container">
<div ng-if="!$ctrl.isEmailVerified" class="alert alert-warning">
<div ng-if="$ctrl.messages.includes('using-deprecated-embed-feature')" class="alert alert-warning">
You have enabled <code>ALLOW_PARAMETERS_IN_EMBEDS</code>. This setting is now deprecated and should be turned off. Parameters in embeds are supported by default. <a href="https://discuss.redash.io/t/support-for-parameters-in-embedded-visualizations/3337" target="_blank">Read more</a>.
</div>
<div ng-if="$ctrl.messages.includes('email-not-verified')" class="alert alert-warning">
We have sent an email with a confirmation link to your email address. Please follow the link to verify your email address. <a ng-click="$ctrl.verifyEmail()">Resend email</a>.
</div>
<empty-state
Expand Down
4 changes: 2 additions & 2 deletions client/app/pages/home/index.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import template from './home.html';
import notification from '@/services/notification';

function HomeCtrl(Events, Dashboard, Query, $http, currentUser) {
function HomeCtrl(Events, Dashboard, Query, $http, messages) {
Events.record('view', 'page', 'personal_homepage');

this.noDashboards = false;
this.noQueries = false;

this.isEmailVerified = currentUser.is_email_verified;
this.messages = messages;

Dashboard.favorites().$promise.then((data) => {
this.favoriteDashboards = data.results;
Expand Down
2 changes: 1 addition & 1 deletion client/app/pages/queries/query.html
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ <h3>
</a>
</li>
<li>
<a ng-click="showEmbedDialog(query, selectedTab)" ng-if="!query.isNew()">
<a ng-click="showEmbedDialog(query, selectedTab)" ng-if="!query.isNew()" data-test="ShowEmbedDialogButton">
<span class="zmdi zmdi-code"></span> Embed elsewhere
</a>
</li>
Expand Down
5 changes: 4 additions & 1 deletion client/app/services/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const currentUser = {
};

export const clientConfig = {};
export const messages = [];

const logger = debug('redash:auth');
const session = { loaded: false };
Expand All @@ -29,6 +30,7 @@ function updateSession(sessionData) {
extend(session, sessionData, { loaded: true });
extend(currentUser, session.user);
extend(clientConfig, session.client_config);
extend(messages, session.messages);
}

function AuthService($window, $location, $q, $http) {
Expand Down Expand Up @@ -61,7 +63,7 @@ function AuthService($window, $location, $q, $http) {
loadConfig() {
logger('Loading config');
return $http.get('/api/config').then((response) => {
updateSession({ client_config: response.data.client_config, user: { permissions: [] } });
updateSession({ client_config: response.data.client_config, user: { permissions: [] }, messages: [] });
return response.data;
});
},
Expand Down Expand Up @@ -111,6 +113,7 @@ export default function init(ngModule) {
ngModule.factory('Auth', AuthService);
ngModule.value('currentUser', currentUser);
ngModule.value('clientConfig', clientConfig);
ngModule.value('messages', messages);
ngModule.factory('apiKeyHttpInterceptor', apiKeyHttpInterceptor);

ngModule.config(($httpProvider) => {
Expand Down
8 changes: 8 additions & 0 deletions client/app/services/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,14 @@ class Parameters {
const params = this.get();
return zipObject(map(params, i => i.name), map(params, i => i.getValue()));
}

toUrlParams() {
const params = Object.assign(...this.get().map(p => p.toUrlParams()));
return Object
.keys(params)
.map(k => `${encodeURIComponent(k)}=${encodeURIComponent(params[k])}`)
.join('&');
}
}

function QueryResultErrorFactory($q) {
Expand Down
54 changes: 54 additions & 0 deletions client/cypress/integration/embed/share_embed_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
describe('Embedded Queries', () => {
beforeEach(() => {
cy.login();
cy.visit('/queries/new');
});

it('are shared with safe parameters', () => {
cy.getByTestId('QueryEditor')
.get('.ace_text-input')
.type('SELECT * FROM organizations WHERE id=\'{{}{{}id}}\'{esc}', { force: true });

cy.getByTestId('TextParamInput').type('1');
cy.clickThrough(`
ParameterSettings-id
ParameterTypeSelect
NumberParameterTypeOption
SaveParameterSettings
ExecuteButton
SaveButton
`);
cy.getByTestId('ShowEmbedDialogButton').click({ force: true });
cy.getByTestId('EmbedIframe').invoke('text').then((iframe) => {
const embedUrl = iframe.match(/"(.*?)"/)[1];
cy.logout();
cy.visit(embedUrl);
cy.getByTestId('VisualizationEmbed', { timeout: 10000 }).should('exist');
cy.percySnapshot('Successfully Embedded Parameterized Query');
});
});

it('cannot be shared with unsafe parameters', () => {
cy.getByTestId('QueryEditor')
.get('.ace_text-input')
.type('SELECT * FROM organizations WHERE name=\'{{}{{}name}}\'{esc}', { force: true });

cy.getByTestId('TextParamInput').type('Redash');
cy.clickThrough(`
ParameterSettings-name
ParameterTypeSelect
TextParameterTypeOption
SaveParameterSettings
ExecuteButton
SaveButton
`);
cy.getByTestId('ShowEmbedDialogButton').click({ force: true });
cy.getByTestId('EmbedIframe').invoke('text').then((iframe) => {
const embedUrl = iframe.match(/"(.*?)"/)[1];
cy.logout();
cy.visit(embedUrl, { failOnStatusCode: false }); // prevent 403 from failing test
cy.getByTestId('ErrorMessage', { timeout: 10000 }).should('exist');
cy.percySnapshot('Unsuccessfully Embedded Parameterized Query');
});
});
});
8 changes: 8 additions & 0 deletions client/cypress/support/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,11 @@ Cypress.Commands.add('login', (email = 'admin@redash.io', password = 'password')

Cypress.Commands.add('logout', () => cy.request('/logout'));
Cypress.Commands.add('getByTestId', element => cy.get('[data-test="' + element + '"]'));
Cypress.Commands.add('clickThrough', (elements) => {
elements
.trim()
.split(/\s/)
.filter(Boolean)
.forEach(element => cy.getByTestId(element).click());
return undefined;
});
16 changes: 14 additions & 2 deletions redash/handlers/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,18 @@ def client_config():
return client_config


def messages():
messages = []

if not current_user.is_email_verified:
messages.append('email-not-verified')

if settings.ALLOW_PARAMETERS_IN_EMBEDS:
messages.append('using-deprecated-embed-feature')

return messages


@routes.route('/api/config', methods=['GET'])
def config(org_slug=None):
return json_response({
Expand All @@ -266,12 +278,12 @@ def session(org_slug=None):
'name': current_user.name,
'email': current_user.email,
'groups': current_user.group_ids,
'permissions': current_user.permissions,
'is_email_verified': current_user.is_email_verified
'permissions': current_user.permissions
}

return json_response({
'user': user,
'messages': messages(),
'org_slug': current_org.slug,
'client_config': client_config()
})
4 changes: 3 additions & 1 deletion redash/handlers/queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ def get(self):

return response


def require_access_to_dropdown_queries(user, query_def):
parameters = query_def.get('options', {}).get('parameters', [])
dropdown_query_ids = [str(p['queryId']) for p in parameters if p['type'] == 'query']
Expand All @@ -179,7 +180,8 @@ def require_access_to_dropdown_queries(user, query_def):
groups = models.Query.all_groups_for_query_ids(dropdown_query_ids)

if len(groups) < len(dropdown_query_ids):
abort(400, message='You are trying to associate a dropdown query that does not have a matching group. Please verify the dropdown query id you are trying to associate with this query.')
abort(400, message="You are trying to associate a dropdown query that does not have a matching group."
"Please verify the dropdown query id you are trying to associate with this query.")

require_access(dict(groups), user, view_only)

Expand Down
Loading