diff --git a/spec/controllers/users_spec.cr b/spec/controllers/users_spec.cr index 0b0c0bde..1b3c750f 100644 --- a/spec/controllers/users_spec.cr +++ b/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 diff --git a/src/placeos-rest-api/controllers/users.cr b/src/placeos-rest-api/controllers/users.cr index b9aad311..98c4e871 100644 --- a/src/placeos-rest-api/controllers/users.cr +++ b/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