fix(users): stop leaking password digests and SSO tokens

spec/controllers/users_spec.cr

@@ -783,6 +783,205 @@ module PlaceOS::Api
           resp.headers["Link"]?.should be_nil
         end
       end
+
+      describe "sensitive fields" do
+        secret_fields = %w(password password_digest access_token refresh_token expires_at expires)
+        admin_only_fields = %w(sys_admin support misc login_name card_number ui_theme)
+
+        # A user with a local password and Microsoft SSO tokens, tagged with
+        # unique metadata so the search only returns this user
+        searchable_secret_user = -> do
+          tag = "leak-#{Random::Secure.hex(6)}"
+          user = Model::Generator.user
+          user.password = "s3cr3t-p@ssw0rd"
+          user.access_token = "ms-access-token-#{tag}"
+          user.refresh_token = "ms-refresh-token-#{tag}"
+          user.expires_at = 1.hour.from_now.to_unix
+          user.expires = true
+          user.misc = "admin notes"
+          user.login_name = "login-#{Random::Secure.hex(6)}"
+          user.card_number = "card-#{Random::Secure.hex(6)}"
+          user.save!
+          user.password_digest.should_not be_nil
+
+          metadata = Model::Generator.metadata(name: random_name, parent: user.id)
+          metadata.details = JSON.parse({tag: tag}.to_json)
+          metadata.save!
+
+          {user, tag}
+        end
+
+        search = ->(tag : String, headers : HTTP::Headers) do
+          filter = %($.tag ? (@ == "#{tag}"))
+          resp = client.get(
+            path: "#{Users.base_route}metadata/search?filter=#{URI.encode_path_segment(filter)}",
+            headers: headers,
+          )
+          resp.status_code.should eq 200
+          resp.body
+        end
+
+        assert_no_secrets = ->(body : String, user : Model::User) do
+          results = JSON.parse(body).as_a
+          results.size.should eq 1
+          (results.first.as_h.keys & secret_fields).should be_empty
+          body.should_not contain(user.password_digest.as(String))
+          body.should_not contain(user.access_token.as(String))
+          body.should_not contain(user.refresh_token.as(String))
+          results.first.as_h
+        end
+
+        {
+          {"non-admin", false, false},
+          {"support", false, true},
+          {"admin", true, false},
+        }.each do |(role, sys_admin, support)|
+          it "never returns password digests or SSO tokens to #{role} users" do
+            user, tag = searchable_secret_user.call
+            body = search.call(tag, Spec::Authentication.headers(sys_admin: sys_admin, support: support))
+            assert_no_secrets.call(body, user)
+            user.destroy
+          end
+        end
+
+        it "hides admin only fields from non-admin users" do
+          user, tag = searchable_secret_user.call
+          body = search.call(tag, Spec::Authentication.headers(sys_admin: false, support: false))
+          result = assert_no_secrets.call(body, user)
+          (result.keys & admin_only_fields).should be_empty
+          result["id"].should eq user.id
+          user.destroy
+        end
+
+        {
+          {"support", false, true},
+          {"admin", true, false},
+        }.each do |(role, sys_admin, support)|
+          it "returns admin only fields to #{role} users" do
+            user, tag = searchable_secret_user.call
+            body = search.call(tag, Spec::Authentication.headers(sys_admin: sys_admin, support: support))
+            result = assert_no_secrets.call(body, user)
+            admin_only_fields.each { |field| result.has_key?(field).should be_true }
+            result["misc"].should eq "admin notes"
+            user.destroy
+          end
+        end
+      end
+    end
+
+    describe "index / show views" do
+      secret_fields = %w(password password_digest access_token refresh_token expires_at expires)
+      admin_only_fields = %w(sys_admin support misc login_name card_number ui_theme)
+
+      secret_user = -> do
+        user = Model::Generator.user
+        user.password = "s3cr3t-p@ssw0rd"
+        user.access_token = "ms-access-token-#{Random::Secure.hex(6)}"
+        user.refresh_token = "ms-refresh-token-#{Random::Secure.hex(6)}"
+        user.misc = "admin notes"
+        user.login_name = "login-#{Random::Secure.hex(6)}"
+        user.card_number = "card-#{Random::Secure.hex(6)}"
+        user.save!
+        user
+      end
+
+      check_view = ->(result : JSON::Any, user : Model::User, admin_view : Bool) do
+        keys = result.as_h.keys
+        (keys & secret_fields).should be_empty
+        result.to_json.should_not contain(user.access_token.as(String))
+        result.to_json.should_not contain(user.refresh_token.as(String))
+        result.to_json.should_not contain(user.password_digest.as(String))
+        if admin_view
+          admin_only_fields.each { |field| keys.includes?(field).should be_true }
+        else
+          (keys & admin_only_fields).should be_empty
+        end
+      end
+
+      {
+        {"non-admin", false, false, false},
+        {"support", false, true, true},
+        {"admin", true, false, true},
+      }.each do |(role, sys_admin, support, admin_view)|
+        it "renders the #{admin_view ? "admin" : "public"} view to #{role} users via show" do
+          user = secret_user.call
+          headers = Spec::Authentication.headers(sys_admin: sys_admin, support: support)
+
+          {false, true}.each do |include_metadata|
+            resp = client.get(
+              path: "#{File.join(Users.base_route, user.id.as(String))}?include_metadata=#{include_metadata}",
+              headers: headers,
+            )
+            resp.status_code.should eq 200
+            check_view.call(JSON.parse(resp.body), user, admin_view)
+          end
+
+          user.destroy
+        end
+
+        it "renders the #{admin_view ? "admin" : "public"} view to #{role} users via index" do
+          user = secret_user.call
+          headers = Spec::Authentication.headers(sys_admin: sys_admin, support: support)
+
+          {false, true}.each do |include_metadata|
+            params = HTTP::Params.encode({"q" => user.email.to_s, "include_metadata" => include_metadata.to_s})
+            resp = client.get(path: "#{Users.base_route}?#{params}", headers: headers)
+            resp.status_code.should eq 200
+            found = JSON.parse(resp.body).as_a.find { |entry| entry["id"] == user.id }
+            found.should_not be_nil
+            check_view.call(found.not_nil!, user, admin_view)
+          end
+
+          user.destroy
+        end
+      end
+    end
+
+    describe "create / update responses" do
+      secret_fields = %w(password password_digest access_token refresh_token expires_at expires)
+
+      it "does not return the password digest when creating a user" do
+        authority = Model::Authority.find_by_domain("localhost").not_nil!
+        body = JSON.parse(Model::Generator.user(authority).to_json).as_h
+        body["password"] = JSON::Any.new("s3cr3t-p@ssw0rd")
+
+        result = client.post(
+          path: Users.base_route,
+          body: body.to_json,
+          headers: Spec::Authentication.headers,
+        )
+        result.status_code.should eq 201
+        created = JSON.parse(result.body).as_h
+        (created.keys & secret_fields).should be_empty
+        created["sys_admin"]?.should_not be_nil
+
+        Model::User.find!(created["id"].as_s).password_digest.should_not be_nil
+        Model::User.find!(created["id"].as_s).destroy
+      end
+
+      it "does not return SSO tokens when a user updates their own profile" do
+        user, headers = Spec::Authentication.authentication(sys_admin: false, support: false)
+        user.access_token = "ms-access-token-update"
+        user.refresh_token = "ms-refresh-token-update"
+        user.save!
+
+        result = client.patch(
+          path: File.join(Users.base_route, user.id.as(String)),
+          body: {nickname: "nick-#{Random.rand(9999)}"}.to_json,
+          headers: headers,
+        )
+        result.status_code.should eq 200
+        (JSON.parse(result.body).as_h.keys & secret_fields).should be_empty
+        result.body.should_not contain("ms-access-token-update")
+        result.body.should_not contain("ms-refresh-token-update")
+      ensure
+        # the authentication user is shared across specs
+        if user
+          user.access_token = nil
+          user.refresh_token = nil
+          user.save!
+        end
+      end
     end
   end
 end

src/placeos-rest-api/controllers/users.cr

@@ -139,6 +139,17 @@ module PlaceOS::Api
 
     alias UserDetails = ::PlaceOS::Model::User::AdminResponse | ::PlaceOS::Model::User::PublicResponse | ::PlaceOS::Model::User::AdminMetadataResponse | ::PlaceOS::Model::User::PublicMetadataResponse
 
+    # Never render a raw `Model::User`: its default JSON includes the password
+    # digest and SSO access / refresh tokens. The admin view adds fields such
+    # as `sys_admin` and `support` to the public view, neither includes secrets.
+    protected def user_details(user : ::PlaceOS::Model::User, admin_view : Bool, include_metadata : Bool = false) : UserDetails
+      if admin_view
+        include_metadata ? user.to_admin_metadata_struct : user.to_admin_struct
+      else
+        include_metadata ? user.to_public_metadata_struct : user.to_public_struct
+      end
+    end
+
     # returns a list of users
     @[AC::Route::GET("/")]
     def index(
@@ -167,19 +178,8 @@ module PlaceOS::Api
 
       results = paginate_search(query, ::PlaceOS::Model::User.table_name)
 
-      if user_admin?
-        if include_metadata
-          results.map &.to_admin_metadata_struct.as(UserDetails)
-        else
-          results.map &.to_admin_struct.as(UserDetails)
-        end
-      else
-        if include_metadata
-          results.map &.to_public_metadata_struct.as(UserDetails)
-        else
-          results.map &.to_public_struct.as(UserDetails)
-        end
-      end
+      admin_view = user_support?
+      results.map { |found| user_details(found, admin_view, include_metadata) }
     end
 
     # returns the profile of the selected user
@@ -188,17 +188,13 @@ module PlaceOS::Api
       @[AC::Param::Info(description: "include user metadata in the response", example: "true")]
       include_metadata : Bool = false,
     ) : UserDetails
-      # We only want to provide limited "public" information
-      if user_admin?
-        include_metadata ? user.to_admin_metadata_struct : user.to_admin_struct
-      else
-        include_metadata ? user.to_public_metadata_struct : user.to_public_struct
-      end
+      # Regular users only see limited "public" information
+      user_details(user, user_support?, include_metadata)
     end
 
     # add a new local user
     @[AC::Route::POST("/", body: :new_user, status_code: HTTP::Status::CREATED)]
-    def create(new_user : JSON::Any) : ::PlaceOS::Model::User
+    def create(new_user : JSON::Any) : UserDetails
       body = new_user.to_json
       new_user = ::PlaceOS::Model::User.from_json(body)
 
@@ -215,13 +211,13 @@ module PlaceOS::Api
       end
 
       raise Error::ModelValidation.new(new_user.errors) unless new_user.save
-      new_user
+      user_details(new_user, admin_view: user_support?)
     end
 
     # udpate a users profile
     @[AC::Route::PATCH("/:id", body: :new_user)]
     @[AC::Route::PUT("/:id", body: :new_user)]
-    def update(new_user : JSON::Any) : ::PlaceOS::Model::User
+    def update(new_user : JSON::Any) : UserDetails
       # Prevent modification of user email via this endpoint - [PPT-2459]
       new_user.as_h?.try &.delete("email")
 
@@ -235,7 +231,7 @@ module PlaceOS::Api
       the_user.assign_attributes_from_json(body)
 
       raise Error::ModelValidation.new(the_user.errors) unless the_user.save
-      the_user
+      user_details(the_user, admin_view: user_support? || the_user.id == current_user.id)
     end
 
     # Destroy user, revoke authentication.
@@ -304,7 +300,7 @@ module PlaceOS::Api
       limit : Int32 = 100,
       @[AC::Param::Info(description: "the starting offset of the result set. Used to implement pagination")]
       offset : Int32 = 0,
-    ) : Array(::PlaceOS::Model::User)
+    ) : Array(UserDetails)
       sql = <<-SQL
         from "user" u INNER JOIN "metadata" m ON m.parent_id = u.id AND
         jsonb_path_exists(m.details, $1) and  u.authority_id = $2
@@ -331,7 +327,8 @@ module PlaceOS::Api
         response.headers["Link"] = %(<#{base_route}?#{params}>; rel="next")
       end
 
-      result
+      admin_view = user_support?
+      result.map { |found| user_details(found, admin_view) }
     end
   end
 end