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.

Sign up for GitHub

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

Warn users about unrestricted roles being assignable by a restricted role #34499

Merged
merged 4 commits into from
May 8, 2024
Merged

Warn users about unrestricted roles being assignable by a restricted role #34499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16c3aa9
warn users about unrestricted roles being assinable by a restricted role
AddisonDunn Apr 24, 2024
757d8bb
use proper Bootstrap classes
AddisonDunn Apr 25, 2024
ea3a593
spell recommend correctly
AddisonDunn Apr 25, 2024
9361080
Merge branch 'master' of github.com:dimagi/commcare-hq into ad/warn-l…
AddisonDunn May 8, 2024
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
20 changes: 17 additions & 3 deletions corehq/apps/users/static/users/js/roles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict';

hqDefine('users/js/roles',[
'jquery',
'underscore',
Expand Down Expand Up @@ -77,7 +79,6 @@ hqDefine('users/js/roles',[
};

var RolesViewModel = function (o) {
'use strict';
var self, root;
self = root = {};

Expand Down Expand Up @@ -142,14 +143,14 @@ hqDefine('users/js/roles',[
};
}),
};

data.manageRoleAssignments = {
all: data.is_non_admin_editable,
specific: ko.utils.arrayMap(o.nonAdminRoles, function (role) {
return {
path: role._id,
name: role.name,
value: data.assignable_by.indexOf(role._id) !== -1,
value: ko.observable(data.assignable_by.indexOf(role._id) !== -1),
access_all_locations: role.permissions.access_all_locations,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an observable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is, actually. Seems likeo is just a dictionary that was passed straight through.

Copy link
Contributor

Choose a reason for hiding this comment

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

huh, I see. I had thought it would need to be in order for the new markup to appear/disappear as needed when someone checks/unchecks full org access. You've confirmed that works, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware that it does not update without reloading the page.

Copy link
Contributor Author

@AddisonDunn AddisonDunn Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I kind of accepted that as a defect--if role's name is changed in a role's model that isn't updated in other modals, either. Generally I think that's true for other modal data. I had trouble figuring out a reasonable high-level change for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The js already rewraps the saved role with the data sent back from the server. I think you'd need to rewrap the other roles and/or recreate the RolesViewModel based on the new data.

That said, I checked out the branch and played around with the interaction, and I think it's fine to call that out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that makes sense. Also maybe I could create a new array on each model that points to the self.permissions.access_all_locations observables on other models?

But yeah, was also thinking it might be out of scope here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't think you'd need to add any data - each role has the proper info about the other roles on page load, that data just isn't getting refreshed properly somewhere.

But yeah, out of scope is fine too.

};
}),
};
Expand Down Expand Up @@ -191,6 +192,19 @@ hqDefine('users/js/roles',[
};
self.preventRoleDelete = data.preventRoleDelete;
self.hasUnpermittedLocationRestriction = data.has_unpermitted_location_restriction || false;

self.restrictRoleChecked = ko.computed(function () {
return data.manageRoleAssignments.specific.some(role => role.value() && !role.access_all_locations);
});
self.showRestrictedLocationRoleAssignmentWarning = ko.computed(function () {
return self.permissions.access_all_locations() && self.restrictRoleChecked();
});
self.cantAccessAllLocations = ko.computed(function () {
return !self.hasUnpermittedLocationRestriction && !self.permissions.access_all_locations();
});
self.unrestrictedButRestrictedRoleCanAssign = ko.computed(function () {
return self.permissions.access_all_locations() && self.restrictRoleChecked();
});
if (self.hasUnpermittedLocationRestriction) {
self.permissions.access_all_locations(true);
}
Expand Down
24 changes: 18 additions & 6 deletions corehq/apps/users/templates/users/partials/edit_role_modal.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{% load i18n %}
{% load hq_shared_tags %}


<div data-bind="modal: roleBeingEdited" tabindex="-1" role="dialog" class="modal fade">
<div data-bind="with: roleBeingEdited" class="modal-dialog">
<form data-bind="submit: $root.submitNewRole">
Expand Down Expand Up @@ -299,7 +298,8 @@ <h4 class="modal-title" data-bind="text: modalTitle"></h4>
</div>
</div>
<div class="form-group"
data-bind="css: { 'has-error': hasUnpermittedLocationRestriction },
data-bind="css: { 'has-error': hasUnpermittedLocationRestriction || unrestrictedButRestrictedRoleCanAssign,
'has-warning': cantAccessAllLocations },
visible: $root.canRestrictAccessByLocation || hasUnpermittedLocationRestriction">
<label class="col-sm-4 control-label">
{% trans "Full Organization Access" %}
Expand All @@ -314,8 +314,8 @@ <h4 class="modal-title" data-bind="text: modalTitle"></h4>
{% trans "Allow role to access data from all locations." %}
</label>
</div>
<div class="help-block alert alert-warning"
data-bind="visible: !hasUnpermittedLocationRestriction && !permissions.access_all_locations()">
<div class="help-block"
data-bind="visible: cantAccessAllLocations">
{% blocktrans %}
Make sure any users assigned this role also have a location assigned to them.
Users without assigned locations will not be permitted to log in.
Expand All @@ -328,6 +328,12 @@ <h4 class="modal-title" data-bind="text: modalTitle"></h4>
full organization access for the assigned users.
{% endblocktrans %}
</div>
<div class="help-block" data-bind="visible: unrestrictedButRestrictedRoleCanAssign">
{% blocktrans %}
With the current configuration, this role will have full organization access and roles that can assign it will not.
This is not recommended.
{% endblocktrans %}
</div>
</div>
</div>
<div class="form-group">
Expand Down Expand Up @@ -457,14 +463,14 @@ <h4 class="modal-title" data-bind="text: modalTitle"></h4>
<label class="col-sm-4 control-label">
{% trans "Manage Role Assignment" %}
</label>
<div class="col-sm-8 controls">
<div class="col-sm-8 controls", data-bind="css: { 'has-error': showRestrictedLocationRoleAssignmentWarning }">
<div class="panel panel-default">
<div class="panel-heading">
{% trans "Select which other roles can assign this role:" %}
</div>
<div class="panel-body"
data-bind="foreach: manageRoleAssignments.specific, slideVisible: !manageRoleAssignments.all()">
<div class="form-check">
<div class="form-check" data-bind="css: {'bg-danger': !access_all_locations() && $parent.permissions.access_all_locations()}">
<input type="checkbox"
data-bind="checked: value, attr: { 'id': 'role-' + path() + '-checkbox' }" />
<label data-bind="attr: { 'for': 'role-' + path() + '-checkbox' }">
Expand All @@ -473,6 +479,12 @@ <h4 class="modal-title" data-bind="text: modalTitle"></h4>
</div>
</div>
</div>
<div class="help-block" data-bind="visible: showRestrictedLocationRoleAssignmentWarning">
{% blocktrans %}
If the highlighted roles are selected, this role will have full organization access and be assignable
by a role that does not. This is not recommended configuration.
{% endblocktrans %}
</div>
</div>
</div>
{% if request|request_has_privilege:"CUSTOM_DOMAIN_ALERTS" %}
Expand Down
Loading