Skip to content

[fix] Fixed authorization bug in get_default_templates view #420 - #422

Merged
nemesifier merged 4 commits into
masterfrom
issues/default-template-urls
Apr 8, 2021
Merged

[fix] Fixed authorization bug in get_default_templates view #420#422
nemesifier merged 4 commits into
masterfrom
issues/default-template-urls

Conversation

@pandafy

@pandafy pandafy commented Apr 6, 2021

Copy link
Copy Markdown
Member

Optimized fetching default templates using URL schema.
A non-superuser can only request default templates for organizations it manages

Closes #420

@pandafy
pandafy force-pushed the issues/default-template-urls branch from 9648543 to cfd2add Compare April 6, 2021 18:31

@nemesifier nemesifier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good! Here's a speedy review, will test later today.

Comment thread openwisp_controller/config/views.py Outdated
if (
not authenticated
or not user.is_staff
or (not user.is_superuser and organization_id not in user.organizations_managed)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would use https://github.com/openwisp/openwisp-users/#is-manager-org since in this case should make it more readable

django.jQuery(function ($) {
var firstRun = true,
addChangeEventToBackend = function (urls) {
addChangeEventToBackend = function (url_schema) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in JS let's use camelCase, urlSchema

Comment thread openwisp_controller/config/admin.py Outdated
ctx.update({'default_template_urls': self._get_default_template_urls()})
ctx.update(
{
'default_template_urls': reverse(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can call it default_template_url (singular)

Comment thread openwisp_controller/config/admin.py Outdated
ctx.update({'default_template_urls': self._get_default_template_urls()})
ctx.update(
{
'default_template_url_schema': reverse(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can just call it default_template_url

@coveralls

coveralls commented Apr 6, 2021

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.04%) to 98.981% when pulling 3b145fa on issues/default-template-urls into 31044e0 on master.

@pandafy
pandafy force-pushed the issues/default-template-urls branch from cfd2add to 0929ccf Compare April 6, 2021 19:05
Comment thread openwisp_controller/config/views.py Outdated
if (
not authenticated
or not user.is_staff
or (not user.is_superuser and user.is_manager(organization_id))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't it be as follows?

or (not user.is_superuser and not user.is_manager(organization_id))

I did some manual testing with a staff user, non superuser, which has access only to one org, but I was still able to access the URL for the other orgs in the system.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes you are right. This was an overlook.

I did some manual testing with a staff user, non superuser, which has access only to one org, but I was still able to access the URL for the other orgs in the system.

Did you also test after making that suggested change? After making this change, it worked fine as expected.

Comment thread openwisp_controller/config/tests/test_views.py
@pandafy
pandafy force-pushed the issues/default-template-urls branch 2 times, most recently from a1c2d98 to 5f6e7e8 Compare April 7, 2021 10:29
Instead of creating a dict of URLs for all organization,
substitute organization_id in URL Schema.
@pandafy
pandafy force-pushed the issues/default-template-urls branch from 5f6e7e8 to 67752a1 Compare April 7, 2021 10:32
@pandafy
pandafy force-pushed the issues/default-template-urls branch from 67752a1 to 22dfca9 Compare April 7, 2021 19:31

@nemesifier nemesifier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested thoroughly, works right, thanks! 👍

@nemesifier
nemesifier merged commit 3d82c1e into master Apr 8, 2021
@nemesifier
nemesifier deleted the issues/default-template-urls branch April 8, 2021 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

[bug] Able to query default templates of other organizations

3 participants