Fix admins without "admin_manage" privilege accessing thier own profile.

Admins without the admin management privilege weren't able to view their own account page, so they couldn't find their admin api token. This fixes it by allowing admins to view a read-only view of their own account, regardless of whether they have the admin_manage privilege. Admins should also be able to set their own password (if using local authentication), but this part isn't yet implemented. See: 18F/api.data.gov#451 18F/api.data.gov#443
NREL · Nov 16, 2018 · 1409949 · 1409949
1 parent 921747d
commit 1409949
Show file tree

Hide file tree

Showing 4 changed files with 126 additions and 40 deletions.
diff --git a/src/api-umbrella/admin-ui/app/templates/components/admins/record-form.hbs b/src/api-umbrella/admin-ui/app/templates/components/admins/record-form.hbs
@@ -5,32 +5,45 @@
     <fieldset class="form-horizontal condensed">
       <legend>User Info</legend>
 
-      {{f.text-field "username" label=(t "mongoid.attributes.admin.username")}}
-      {{#unless session.data.authenticated.username_is_email}}
-        {{f.text-field "email" label=(t "mongoid.attributes.admin.email")}}
-      {{/unless}}
+      {{#if currentAdmin.permissions.admin_manage}}
+        {{f.text-field "username" label=(t "mongoid.attributes.admin.username")}}
+        {{#unless session.data.authenticated.username_is_email}}
+          {{f.text-field "email" label=(t "mongoid.attributes.admin.email")}}
+        {{/unless}}
+      {{else}}
+        {{f.static-field "username" label=(t "mongoid.attributes.admin.username")}}
+        {{#unless session.data.authenticated.username_is_email}}
+          {{f.static-field "email" label=(t "mongoid.attributes.admin.email")}}
+        {{/unless}}
+      {{/if}}
       {{#if model.name}}
         {{f.static-field "name" label=(t "mongoid.attributes.admin.name")}}
       {{/if}}
-      {{f.textarea-field "notes" label=(t "mongoid.attributes.admin.notes")}}
-      {{#if model.id}}
-        {{#unless model.currentSignInAt}}
-          {{f.checkbox-field "sendInviteEmail" label="Resend invite email"}}
-        {{/unless}}
+      {{#if currentAdmin.permissions.admin_manage}}
+        {{f.textarea-field "notes" label=(t "mongoid.attributes.admin.notes")}}
+        {{#if model.id}}
+          {{#unless model.currentSignInAt}}
+            {{f.checkbox-field "sendInviteEmail" label="Resend invite email"}}
+          {{/unless}}
+        {{else}}
+          {{f.checkbox-field "sendInviteEmail" label="Send invite email"}}
+        {{/if}}
       {{else}}
-        {{f.checkbox-field "sendInviteEmail" label="Send invite email"}}
+        {{f.static-field "notes" label=(t "mongoid.attributes.admin.notes")}}
       {{/if}}
     </fieldset>
 
     {{#if model.authenticationToken}}
       {{#if session.data.authenticated.local_auth_enabled}}
-        <fieldset class="form-horizontal condensed">
-          <legend>{{t "devise.passwords.edit.change_your_password"}}</legend>
+        {{#if currentAdmin.permissions.admin_manage}}
+          <fieldset class="form-horizontal condensed">
+            <legend>{{t "devise.passwords.edit.change_your_password"}}</legend>
 
-          {{f.password-field "currentPassword" label=(t "mongoid.attributes.admin.current_password")}}
-          {{f.password-field "password" label=(t "devise.passwords.edit.new_password") hint=(t "devise.passwords.password_length_hint" min=session.data.authenticated.password_length_min)}}
-          {{f.password-field "passwordConfirmation" label=(t "devise.passwords.edit.confirm_new_password")}}
-        </fieldset>
+            {{f.password-field "currentPassword" label=(t "mongoid.attributes.admin.current_password")}}
+            {{f.password-field "password" label=(t "devise.passwords.edit.new_password") hint=(t "devise.passwords.password_length_hint" min=session.data.authenticated.password_length_min)}}
+            {{f.password-field "passwordConfirmation" label=(t "devise.passwords.edit.confirm_new_password")}}
+          </fieldset>
+        {{/if}}
       {{/if}}
 
       <fieldset class="form-horizontal condensed">
@@ -42,18 +55,22 @@
       </fieldset>
     {{/if}}
 
-    <fieldset class="form-horizontal condensed">
-      <legend>Permissions</legend>
+    {{#if currentAdmin.permissions.admin_manage}}
+      <fieldset class="form-horizontal condensed">
+        <legend>Permissions</legend>
 
-      {{f.checkboxes-field "groupIds" label=(t "mongoid.attributes.admin.groups") options=groupOptions}}
-      {{#if currentAdmin.superuser}}
-        {{f.checkbox-field "superuser" label=(t "mongoid.attributes.admin.superuser")}}
-      {{/if}}
-    </fieldset>
+        {{f.checkboxes-field "groupIds" label=(t "mongoid.attributes.admin.groups") options=groupOptions}}
+        {{#if currentAdmin.superuser}}
+          {{f.checkbox-field "superuser" label=(t "mongoid.attributes.admin.superuser")}}
+        {{/if}}
+      </fieldset>
+    {{/if}}
 
     <div class="row">
       <div class="col-sm-6">
-        <button type="submit" class="btn btn-lg btn-primary save-button"><span class="btn-label">Save</span><span class="btn-loading-label">{{fa-icon "sync-alt" spin=true}}Saving...</span></button>
+        {{#if currentAdmin.permissions.admin_manage}}
+          <button type="submit" class="btn btn-lg btn-primary save-button"><span class="btn-label">Save</span><span class="btn-loading-label">{{fa-icon "sync-alt" spin=true}}Saving...</span></button>
+        {{/if}}
       </div>
       <div class="col-sm-6 record-details">
         {{#if model.id}}
@@ -64,10 +81,12 @@
         {{/if}}
       </div>
     </div>
-    {{#if model.id}}
-      <div class="form-extra-actions">
-        <a href="#" class="remove-action" {{action "delete"}}>{{fa-icon "times"}}Delete Admin</a>
-      </div>
+    {{#if currentAdmin.permissions.admin_manage}}
+      {{#if model.id}}
+        <div class="form-extra-actions">
+          <a href="#" class="remove-action" {{action "delete"}}>{{fa-icon "times"}}Delete Admin</a>
+        </div>
+      {{/if}}
     {{/if}}
   {{/fields-for}}
 </form>
diff --git a/src/api-umbrella/web-app/app/policies/admin_policy.rb b/src/api-umbrella/web-app/app/policies/admin_policy.rb
@@ -21,6 +21,29 @@ def resolve
   end
 
   def show?
+    # Allow admins to always view their own record, even if they don't have the
+    # admin_manage privilege (so they can view their admin token).
+    #
+    # TODO: An admin should also be able to update their own password if using
+    # local password authentication, but that is not yet implemented.
+    manage? || (user.id == record.id)
+  end
+
+  def update?
+    manage?
+  end
+
+  def create?
+    update?
+  end
+
+  def destroy?
+    update?
+  end
+
+  private
+
+  def manage?
     allowed = false
     if(user.superuser?)
       allowed = true
@@ -42,16 +65,4 @@ def show?
 
     allowed
   end
-
-  def update?
-    show?
-  end
-
-  def create?
-    update?
-  end
-
-  def destroy?
-    update?
-  end
 end
diff --git a/test/admin_ui/test_admins.rb b/test/admin_ui/test_admins.rb
@@ -72,4 +72,31 @@ def test_removes_groups_when_checked
     admin = Admin.find(admin.id)
     assert_equal([@group3.id].sort, admin.group_ids.sort)
   end
+
+  def test_non_admin_manager_views_own_profile
+    # An admin without the "admin_manage" role.
+    admin = FactoryBot.create(:limited_admin, :groups => [
+      FactoryBot.create(:google_admin_group, :analytics_permission),
+    ])
+    admin_login(admin)
+
+    visit "/admin/#/admins/#{admin.id}/edit"
+
+    refute_field("Email")
+    assert_text("Email")
+    assert_text(admin.username)
+    refute_field("Notes")
+    assert_text("Notes")
+    # TODO: Admins should be able to set their own password, even if they lack
+    # the admin_manage role.
+    refute_text("Change Your Password")
+    refute_field("Current Password")
+    refute_field("New Password")
+    refute_text("Permissions")
+    assert_text("Admin API Access")
+    assert_text("Admin API Token")
+    assert(admin.authentication_token)
+    assert_text(admin.authentication_token)
+    refute_button("Save")
+  end
 end
diff --git a/test/apis/v1/admins/test_admin_permissions.rb b/test/apis/v1/admins/test_admin_permissions.rb
@@ -239,6 +239,35 @@ def test_permits_superuser_removing_superuser_from_existing_admin
     assert_equal(false, record.superuser)
   end
 
+  def test_permits_any_admin_to_view_but_not_edit_own_record
+    # An admin without the "admin_manage" role.
+    admin = FactoryBot.create(:limited_admin, :groups => [
+      FactoryBot.create(:google_admin_group, :analytics_permission),
+    ])
+
+    response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/admins/#{admin.id}.json", http_options.deep_merge(admin_token(admin)))
+    assert_response_code(200, response)
+    data = MultiJson.load(response.body)
+    assert_equal(["admin"], data.keys)
+
+    attributes = admin.serializable_hash
+    attributes["username"] += rand(999_999).to_s
+    response = Typhoeus.put("https://127.0.0.1:9081/api-umbrella/v1/admins/#{admin.id}.json", http_options.deep_merge(admin_token(admin)).deep_merge({
+      :headers => { "Content-Type" => "application/json" },
+      :body => MultiJson.dump(:admin => attributes),
+    }))
+    assert_response_code(403, response)
+    data = MultiJson.load(response.body)
+    assert_equal(["errors"], data.keys)
+
+    initial_count = active_count
+    response = Typhoeus.delete("https://127.0.0.1:9081/api-umbrella/v1/admins/#{admin.id}.json", http_options.deep_merge(admin_token(admin)))
+    assert_response_code(403, response)
+    data = MultiJson.load(response.body)
+    assert_equal(["errors"], data.keys)
+    assert_equal(0, active_count - initial_count)
+  end
+
   private
 
   def assert_admin_permitted(factory, admin)