Add support for operator and manager roles #203

ChrisMacNaughton · 2021-02-23T08:35:44Z

Broadly, the new roles work as follows:

The manager role is a role that manages users and groups, with the
notable exception that a Manager cannot add or remove the Operator
and Admin attributes on a group, nor add those groups to a user.
In effect, this means that a Manager can only add or remove a useri
from non-permission granting groups, or to the Manager group.

The operator role is a role designed to help with management of
EyeDP itself, granting no control of users not groups, but allowing
changing Settings and Identity Service Providers. This means that
an Operator can create and edit SAML or OpenID Connect Applications,
and they can manage Certificates for the same, as well as updating
Settings such as the application's base URI or templates for emails.

Ad admin continues to have full permission for all activities.

Closes: #199

coveralls · 2021-02-23T08:38:11Z

Coverage increased (+0.5%) to 83.588% when pulling 1e2fb30 on feature/user-permissions into f1b4387 on master.

Broadly, the new roles work as follows: The manager role is a role that manages users and groups, with the notable exception that a Manager cannot add or remove the Operator and Admin attributes on a group, nor add those groups to a user. In effect, this means that a Manager can only add or remove a useri from non-permission granting groups, or to the Manager group. The operator role is a role designed to help with management of EyeDP itself, granting no control of users not groups, but allowing changing Settings and Identity Service Providers. This means that an Operator can create and edit SAML or OpenID Connect Applications, and they can manage Certificates for the same, as well as updating Settings such as the application's base URI or templates for emails. Ad admin continues to have full permission for all activities. Closes: #199

igalic

i need some clarification before i can actually review this, as i have never seen this construct before

igalic · 2021-02-23T22:09:06Z

app/controllers/admin/applications_controller.rb

  end
+
+  def ensure_user_is_authorized!
+    raise(ActionController::RoutingError, 'Not Found') and return \


what does raise and return do?

The return here is unnecessary but doesn't harm things, and it fits a general Rails pattern of ensuring that a web request isn't rendered twice.

The raise is basically saying: 404 not Found. When an ActionController::RoutingError is raised during a Rails request, the server will return with a 404 and the 404 page that's located in public/404.html.

Previously, this pattern was specifically used to guard the entire of the admin UI against any user who is not an admin, but is being expanded dramatically here to allow more kinds of users into parts of the admin UI.

…access

igalic

looks pretty good to me
thanks for all the explanations and for adding the documentation

igalic · 2021-02-25T10:55:32Z

README.md

+
+### Operators
+
+An operator is a type of user that can manage EyeDP itself, but cannot manage


can you give me a one sentence summary of what it means to manage EyeDP in this sense?

like, add and remove applications and links? auth backends? etc…

igalic · 2021-02-25T14:37:52Z

app/controllers/admin/groups_controller.rb


-  def model_params
-    p = params.require(:group).permit(
+  def model_params # rubocop:disable Metrics/MethodLength


seriously? this method is too long according to rubocop?? wtf.

igalic · 2021-02-25T14:54:49Z

app/models/group.rb

+  def roles
+    @roles ||= begin
+      %i[admin manager operator].filter do |name|
+        send(name)


can you explain what happens here?

you're sending each of :admin, :manager, :operator messages to… where exactly?

This is basically the same as:

@roles ||= begin roles = [] [:admin :manager :operator].each do |name| roles << name if self.send(name) end roles end

and, the thing.send method is a Ruby way of calling a method on a thing by a stringy name, it's effectively the same as saying:

roles << :admin if self.admin

tweidinger · 2021-02-27T06:00:17Z

README.md

+
+### Managers
+
+An operator is a type of user that can manage users and groups, but cannot


The word operator should be replaced with manager.

tweidinger · 2021-02-27T06:09:19Z

app/controllers/admin/users_controller.rb

      :password, :last_activity_at, group_ids: []
    )
    p[:group_ids] ||= []
+    # A Manager cannot add a user to an operator or admin group


Should this comment be moved inside the condition?
Not 100% sure how the coding/commenting style is defined, but the pluck action is more related to the comment than the manager condition itself.

Good catch, will tidy this

tweidinger · 2021-02-27T06:13:15Z

app/views/layouts/_admin_nav.html.erb

-  <%= nav_link "#{fa_icon("tasks")} Permissions".html_safe, admin_permissions_path %>
+    <%= nav_link "#{fa_icon("tasks")} Permissions".html_safe, admin_permissions_path %>
+  <%- end %>
  <%= nav_link "#{fa_icon("commenting")} Audit Log".html_safe, admin_audits_path %>


Is the audit log relevant for a manager?
Are there details leaked outside of the manager or operator scope?

That's a good point, it's probably worth restricting the audit log to just admins.

tweidinger

Looks good for me :)

Add support in Groups / Users for Operator and Manager role

e4d8570

ChrisMacNaughton force-pushed the feature/user-permissions branch from 1fc638e to 19070a5 Compare February 23, 2021 08:37

ChrisMacNaughton force-pushed the feature/user-permissions branch 2 times, most recently from bf64bc1 to a930b69 Compare February 23, 2021 08:44

igalic reviewed Feb 24, 2021

View reviewed changes

ChrisMacNaughton added 3 commits February 24, 2021 09:03

Add documentation about manager/operator/admin types

4e7b02a

Add guards to not show menu entries that different user types cannot …

7f4929d

…access

remove redundant and return from permissions checks in admin

629dcbd

igalic approved these changes Feb 25, 2021

View reviewed changes

tweidinger reviewed Feb 27, 2021

View reviewed changes

Update in response to review comments

1e2fb30

tweidinger approved these changes Feb 27, 2021

View reviewed changes

ChrisMacNaughton merged commit b878976 into master Mar 15, 2021

ChrisMacNaughton deleted the feature/user-permissions branch March 15, 2021 07:52


		### Operators

		An operator is a type of user that can manage EyeDP itself, but cannot manage


		### Managers

		An operator is a type of user that can manage users and groups, but cannot

Uh oh!

Add support for operator and manager roles #203

Add support for operator and manager roles #203

Uh oh!

Conversation

ChrisMacNaughton commented Feb 23, 2021

Uh oh!

coveralls commented Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tweidinger Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tweidinger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 23, 2021 •

edited

Loading

tweidinger Feb 27, 2021 •

edited

Loading