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.

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

feat: improve permissions checks #292

Merged
merged 8 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion shard.lock
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ shards:

placeos-models:
git: https://github.com/placeos/models.git
version: 9.16.3
version: 9.17.0

pool:
git: https://github.com/ysbaddaden/pool.git
Expand Down
7 changes: 5 additions & 2 deletions spec/controllers/bookings_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,9 @@ describe Bookings do

it "should return a list of bookings when filtered by user" do
tenant = get_tenant
user = Mock::Token.generate_auth_user(false, false)

booking1 = BookingsHelper.create_booking(tenant.id.not_nil!, "toby@redant.com.au")
booking1 = BookingsHelper.create_booking(tenant.id.not_nil!, user.email.to_s)
booking2 = BookingsHelper.create_booking(tenant.id.not_nil!)

starting = 5.minutes.from_now.to_unix
Expand All @@ -145,8 +146,10 @@ describe Bookings do
end

it "should return a list of bookings filtered current user when no zones or user is specified" do
user = Mock::Token.generate_auth_user(false, false)

tenant = get_tenant
booking = BookingsHelper.create_booking(tenant_id: tenant.id.not_nil!, user_email: "toby@redant.com.au")
booking = BookingsHelper.create_booking(tenant_id: tenant.id.not_nil!, user_email: user.email.to_s)
BookingsHelper.create_booking(tenant.id.not_nil!)

starting = 5.minutes.from_now.to_unix
Expand Down
18 changes: 9 additions & 9 deletions spec/controllers/calendars_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ describe Calendars do
it "#index should return a list of calendars" do
WebMock.stub(:post, "https://login.microsoftonline.com/bb89674a-238b-4b7d-91ec-6bebad83553a/oauth2/v2.0/token")
.to_return(body: File.read("./spec/fixtures/tokens/o365_token.json"))
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendars")
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendars")
.to_return(body: File.read("./spec/fixtures/calendars/o365/show.json"))

# instantiate the controller
Expand Down Expand Up @@ -45,13 +45,13 @@ describe Calendars do
end

it "#free_busy should return free busy data of calendars" do
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendars")
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendars")
.to_return(body: File.read("./spec/fixtures/calendars/o365/show.json"))
WebMock.stub(:post, "#{ENV["PLACE_URI"]}/auth/oauth/token")
.to_return(body: File.read("./spec/fixtures/tokens/placeos_token.json"))
WebMock.stub(:get, "#{ENV["PLACE_URI"]}/api/engine/v2/systems?limit=1000&offset=0&zone_id=zone-EzcsmWbvUG6")
.to_return(body: File.read("./spec/fixtures/placeos/systems.json"))
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendar/getSchedule")
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendar/getSchedule")
.to_return(body: File.read("./spec/fixtures/events/o365/get_schedule.json"))
WebMock.stub(:post, "https://login.microsoftonline.com/bb89674a-238b-4b7d-91ec-6bebad83553a/oauth2/v2.0/token")
.to_return(body: File.read("./spec/fixtures/tokens/o365_token.json"))
Expand All @@ -66,13 +66,13 @@ describe Calendars do
end

it "#free_busy request with a duration < 30mins" do
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendars")
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendars")
.to_return(body: File.read("./spec/fixtures/calendars/o365/show.json"))
WebMock.stub(:post, "#{ENV["PLACE_URI"]}/auth/oauth/token")
.to_return(body: File.read("./spec/fixtures/tokens/placeos_token.json"))
WebMock.stub(:get, "#{ENV["PLACE_URI"]}/api/engine/v2/systems?limit=1000&offset=0&zone_id=zone-EzcsmWbvUG6")
.to_return(body: File.read("./spec/fixtures/placeos/systems.json"))
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendar/getSchedule")
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendar/getSchedule")
.to_return(body: File.read("./spec/fixtures/events/o365/get_schedule.json"))
WebMock.stub(:post, "https://login.microsoftonline.com/bb89674a-238b-4b7d-91ec-6bebad83553a/oauth2/v2.0/token")
.to_return(body: "")
Expand All @@ -85,13 +85,13 @@ describe Calendars do
end

it "#free_busy should not allow an interval of less than 5 minutes" do
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendars")
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendars")
.to_return(body: File.read("./spec/fixtures/calendars/o365/show.json"))
WebMock.stub(:post, "#{ENV["PLACE_URI"]}/auth/oauth/token")
.to_return(body: File.read("./spec/fixtures/tokens/placeos_token.json"))
WebMock.stub(:get, "#{ENV["PLACE_URI"]}/api/engine/v2/systems?limit=1000&offset=0&zone_id=zone-EzcsmWbvUG6")
.to_return(body: File.read("./spec/fixtures/placeos/systems.json"))
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendar/getSchedule")
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendar/getSchedule")
.to_return(body: File.read("./spec/fixtures/events/o365/get_schedule.json"))
WebMock.stub(:post, "https://login.microsoftonline.com/bb89674a-238b-4b7d-91ec-6bebad83553a/oauth2/v2.0/token")
.to_return(body: "")
Expand Down Expand Up @@ -123,13 +123,13 @@ module CalendarsHelper
def stub_cal_endpoints
WebMock.stub(:post, "https://login.microsoftonline.com/bb89674a-238b-4b7d-91ec-6bebad83553a/oauth2/v2.0/token")
.to_return(body: File.read("./spec/fixtures/tokens/o365_token.json"))
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendars")
WebMock.stub(:get, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendars")
.to_return(body: File.read("./spec/fixtures/calendars/o365/show.json"))
WebMock.stub(:post, "#{ENV["PLACE_URI"]}/auth/oauth/token")
.to_return(body: File.read("./spec/fixtures/tokens/placeos_token.json"))
WebMock.stub(:get, "#{ENV["PLACE_URI"]}/api/engine/v2/systems?limit=1000&offset=0&zone_id=zone-EzcsmWbvUG6")
.to_return(body: File.read("./spec/fixtures/placeos/systems.json"))
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.com/calendar/getSchedule")
WebMock.stub(:post, "https://graph.microsoft.com/v1.0/users/dev%40acaprojects.onmicrosoft.com/calendar/getSchedule")
.to_return(body: File.read("./spec/fixtures/events/o365/get_schedule_avail.json"))

WebMock.stub(:get, ENV["PLACE_URI"].to_s + "/api/engine/v2/systems/sys-rJQQlR4Cn7")
Expand Down
21 changes: 13 additions & 8 deletions spec/controllers/events_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -106,19 +106,19 @@ describe Events do

# Should have created attendees records
# 2 guests + 1 host
evt_meta.attendees.count.should eq(3)
evt_meta.attendees.count.should eq(2)

# Should have created guests records
guests = evt_meta.attendees.map(&.guest.not_nil!)
guests.map(&.name).should eq(["Amit", "John", "dev@acaprojects.onmicrosoft.com"])
guests.compact_map(&.email).should eq(["amit@redant.com.au", "jon@example.com", "dev@acaprojects.onmicrosoft.com"])
guests.map(&.name).should eq(["John", "dev@acaprojects.onmicrosoft.com"])
guests.compact_map(&.email).should eq(["jon@example.com", "dev@acaprojects.onmicrosoft.com"])
guests.compact_map(&.preferred_name).should eq(["Jon"])
guests.compact_map(&.phone).should eq(["012334446"])
guests.compact_map(&.organisation).should eq(["Google inc"])
guests.compact_map(&.notes).should eq(["some notes"])
guests.compact_map(&.photo).should eq(["http://example.com/first.jpg"])
guests.compact_map(&.searchable).should eq(["amit@redant.com.au amit ", "jon@example.com john jon google inc 012334446", "dev@acaprojects.onmicrosoft.com dev@acaprojects.onmicrosoft.com "])
guests.compact_map(&.extension_data).should eq([{} of String => String?, {"fizz" => "buzz"}, {} of String => String?])
guests.compact_map(&.searchable).should eq(["jon@example.com john jon google inc 012334446", "dev@acaprojects.onmicrosoft.com dev@acaprojects.onmicrosoft.com "])
guests.compact_map(&.extension_data).should eq([{"fizz" => "buzz"}, {} of String => String?])
end
end

Expand Down Expand Up @@ -247,8 +247,9 @@ describe Events do
req_body = EventsHelper.update_event_input
system_id = "sys-rJQQlR4Cn7"
EventsHelper.stub_permissions_check(system_id)
resp = client.patch("#{EVENTS_BASE}/#{created_event["id"]}?system_id=#{system_id}", headers: headers, body: req_body)

updated_event = JSON.parse(client.patch("#{EVENTS_BASE}/#{created_event["id"]}?system_id=#{system_id}", headers: headers, body: req_body).body)
updated_event = JSON.parse(resp.body)
updated_event["event_start"].should eq(1598504460)
updated_event["event_end"].should eq(1598508120)
end
Expand Down Expand Up @@ -550,7 +551,7 @@ describe Events do

evt_meta = EventMetadata.find_by(event_id: created_event_id)
guests = evt_meta.attendees.map(&.guest.not_nil!)
guests.map(&.name).should eq(["Amit", "John", "dev@acaprojects.onmicrosoft.com"])
guests.map(&.name).should eq(["John", "dev@acaprojects.onmicrosoft.com"])

# guest_checkin via system
resp = client.post("#{EVENTS_BASE}/#{created_event_id}/guests/jon@example.com/checkin?system_id=sys-rJQQlR4Cn7", headers: headers).body
Expand Down Expand Up @@ -652,7 +653,11 @@ describe Events do
"count" => 2,
}.to_json

request = client.patch("#{EVENTS_BASE}/#{event.event_id}/metadata/#{event.system_id}", headers: headers, body: update_body)
request = client.patch(
"#{EVENTS_BASE}/#{event.event_id}/metadata/#{event.system_id}",
headers: headers,
body: update_body
)
request.status_code.should eq(200)

request_body = JSON.parse(request.body)
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/helpers/event_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,15 @@ module EventsHelper
}.to_json
end

def create_event_input
def create_event_input(user = Mock::Token.generate_auth_user(false, false))
%({
"event_start": 1598503500,
"event_end": 1598507160,
"recurrence": {"range_start":1637825922,"range_end":1639035522,"interval":2,"pattern":"daily"},
"attendees": [
{
"name": "Amit",
"email": "amit@redant.com.au",
"email": "#{user.email}",
"response_status": "accepted",
"resource": false,
"organizer": true,
Expand Down
65 changes: 56 additions & 9 deletions spec/spec_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,23 @@ module Mock
module Token
extend self

CREATION_LOCK = Mutex.new(protection: :reentrant)

# office_mock_token
def office
def office(sys_admin = false, support = false, groups = ["manage", "admin"])
user = generate_auth_user(sys_admin, support, groups)
UserJWT.new(
iss: "staff-api",
iat: Time.local,
exp: Time.local + 1.week,
domain: "toby.staff-api.dev",
id: "toby@redant.com.au",
id: user.id.as(String),
scope: [PlaceOS::Model::UserJWT::Scope::PUBLIC],
user: UserJWT::Metadata.new(
name: "Toby Carvan",
email: "dev@acaprojects.com",
email: user.email.to_s,
permissions: UserJWT::Permissions::Admin,
roles: ["manage", "admin"]
roles: groups
)
).encode
end
Expand All @@ -75,7 +78,7 @@ module Mock
iat: Time.local,
exp: Time.local + 1.week,
domain: "toby.staff-api.dev",
id: "toby@redant.com.au",
id: "amit@redant.com.au",
scope: [PlaceOS::Model::UserJWT::Scope::GUEST],
user: UserJWT::Metadata.new(
name: "Jon Jon",
Expand All @@ -87,22 +90,66 @@ module Mock
end

# google_mock_token
def google
def google(sys_admin = false, support = false, groups = ["manage", "admin"])
user = generate_auth_user(sys_admin, support, groups)
UserJWT.new(
iss: "staff-api",
iat: Time.local,
exp: Time.local + 1.week,
domain: "google.staff-api.dev",
id: "amit@redant.com.au",
id: user.id.as(String),
scope: [PlaceOS::Model::UserJWT::Scope::PUBLIC, PlaceOS::Model::UserJWT::Scope::GUEST],
user: UserJWT::Metadata.new(
name: "Amit Gaur",
email: "amit@redant.com.au",
email: user.email.to_s,
permissions: UserJWT::Permissions::Admin,
roles: ["manage", "admin"]
roles: groups
)
).encode
end

def generate_auth_user(sys_admin, support, groups = [] of String)
CREATION_LOCK.synchronize do
org_zone
authority = PlaceOS::Model::Authority.find_by_domain("toby.staff-api.dev") || PlaceOS::Model::Generator.authority
authority.domain = "toby.staff-api.dev"
authority.config_will_change!
authority.config["org_zone"] = JSON::Any.new("zone-perm-org")
authority.save!

if sys_admin || support
group_list = groups.join('-')
test_user_email = PlaceOS::Model::Email.new("test-#{"admin-" if sys_admin}#{"supp-" if support}grp-#{group_list}-rest-api@place.tech")
else
test_user_email = PlaceOS::Model::Email.new("dev@acaprojects.onmicrosoft.com")
end

PlaceOS::Model::User.where(email: test_user_email.to_s, authority_id: authority.id.as(String)).first? || PlaceOS::Model::Generator.user(authority, support: support, admin: sys_admin).tap do |user|
user.email = test_user_email
user.groups = groups
user.save!
end
end
end

def org_zone
zone = PlaceOS::Model::Zone.find?("zone-perm-org")
return zone if zone

zone = PlaceOS::Model::Generator.zone
zone.id = "zone-perm-org"
zone.tags = Set.new ["org"]
zone.save!

metadata = PlaceOS::Model::Generator.metadata("permissions", zone)
metadata.details = JSON.parse({
admin: ["management"],
manage: ["concierge"],
}.to_json)

metadata.save!
zone
end
end

module Headers
Expand Down
8 changes: 4 additions & 4 deletions src/controllers/bookings.cr
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@

@[AC::Route::Filter(:before_action, only: [:update, :update_alt, :destroy, :update_state])]
private def confirm_access
if (user = user_token) &&
(booking && !({booking.user_id, booking.booked_by_id}.includes?(user.id) || (booking.user_email == user_token.user.email.downcase))) &&
!(user.is_admin? || user.is_support?) &&
!check_access(user.user.roles, booking.zones || [] of String).none?
return if is_support?
if (user = current_user)

Check notice on line 25 in src/controllers/bookings.cr

View workflow job for this annotation

GitHub Actions / Ameba

Style/ParenthesesAroundCondition

Redundant parentheses
Raw output
> if (user = current_user)
     ^
return if booking && ({booking.user_id, booking.booked_by_id}.includes?(user.id) || (booking.user_email == user.email.downcase))
return if check_access(user.groups, booking.zones || [] of String).can_manage?
head :forbidden
end
end
Expand Down
26 changes: 22 additions & 4 deletions src/controllers/events.cr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@ class Events < Application
raise Error::Forbidden.new unless is_support?
end

@[AC::Route::Filter(:before_action, only: [:extension_metadata])]
private def confirm_access(
system_id : String? = nil
)
return if is_support?

if system_id
system = PlaceOS::Model::ControlSystem.find!(system_id)
return if check_access(current_user.groups, system.zones || [] of String).can_manage?
end

raise Error::Forbidden.new("user not in appropriate user group")
end

# update includes a bunch of moving parts so we want to roll back if something fails
@[AC::Route::Filter(:around_action, only: [:update])]
def wrap_in_transaction(&)
Expand Down Expand Up @@ -352,7 +366,7 @@ class Events < Application
existing_attendees = event.attendees.try(&.map { |a| a.email.downcase }) || [] of String
unless user_email == host || user_email.in?(existing_attendees)
# may be able to edit on behalf of the user
raise Error::Forbidden.new("user #{user_email} not involved in meeting and no role is permitted to make this change") if !(system && !check_access(user.roles, [system.id] + system.zones).none?)
raise Error::Forbidden.new("user #{user_email} not involved in meeting and no role is permitted to make this change") if !(system && !check_access(user.roles, [system.id] + system.zones).forbidden?)
end

# Check if attendees need updating
Expand Down Expand Up @@ -651,7 +665,11 @@ class Events < Application
meta = if mdata = query.to_a.first?
if user_token.guest_scope?
raise Error::Forbidden.new("guest #{user_token.id} attempting to edit an event they are not associated with") unless merge && guest_event_id.in?({mdata.event_id, mdata.recurring_master_id, mdata.ical_uid})
elsif mdata.host_email.downcase != user.email.downcase
# ensure the user should be able to edit this metadata
confirm_access(system_id)
end

mdata
else
event = client.get_event(user_email, id: event_id, calendar_id: cal_id)
Expand Down Expand Up @@ -933,7 +951,7 @@ class Events < Application
existing_attendees = event.attendees.try(&.map { |a| a.email.downcase }) || [] of String
unless user_email == host || user_email.in?(existing_attendees)
# may be able to delete on behalf of the user
raise Error::Forbidden.new("user #{user_email} not involved in meeting and no role is permitted to make this change") if !(system && !check_access(user.roles, [system.id] + system.zones).none?)
raise Error::Forbidden.new("user #{user_email} not involved in meeting and no role is permitted to make this change") if !(system && !check_access(user.roles, [system.id] + system.zones).forbidden?)
end

# we don't need host details for delete / decline as we want it to occur on the calendar specified
Expand Down Expand Up @@ -978,7 +996,7 @@ class Events < Application
system = get_placeos_client.systems.fetch(system_id)
cal_id = system.email
raise AC::Route::Param::ValueError.new("system '#{system.name}' (#{system_id}) does not have a resource email address specified", "system_id") unless cal_id
client.accept_event(user.email, id: event_id, calendar_id: cal_id)
client.accept_event(cal_id, id: event_id, calendar_id: cal_id)
end

# rejects / declines the meeting on behalf of the event space
Expand All @@ -993,7 +1011,7 @@ class Events < Application
system = get_placeos_client.systems.fetch(system_id)
cal_id = system.email
raise AC::Route::Param::ValueError.new("system '#{system.name}' (#{system_id}) does not have a resource email address specified", "system_id") unless cal_id
client.decline_event(user.email, id: event_id, calendar_id: cal_id)
client.decline_event(cal_id, id: event_id, calendar_id: cal_id)
end

# Event Guest management
Expand Down
3 changes: 3 additions & 0 deletions src/controllers/utilities/current-user.cr
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ module Utils::CurrentUser
@user_token.as(UserJWT)
end

# Obtains user referenced by user_token id
getter current_user : PlaceOS::Model::User { PlaceOS::Model::User.find!(user_token.id) }

def user
user_token.user
end
Expand Down
Loading
Loading