diff --git a/spec/admin_consent_flow_spec.cr b/spec/admin_consent_flow_spec.cr index e0e45c09..c9174593 100644 --- a/spec/admin_consent_flow_spec.cr +++ b/spec/admin_consent_flow_spec.cr @@ -8,7 +8,7 @@ module PlaceOS::Api loaded = AdminConsentFlow.load(flow.id).not_nil! loaded.state.should eq "running" - loaded.steps.size.should eq 4 + loaded.steps.size.should eq 5 loaded.steps.all? { |step| step.state == "pending" }.should be_true loaded.redirect.should eq "/backoffice/#/domains/authority-test/authentication" diff --git a/spec/tenant_consent_spec.cr b/spec/tenant_consent_spec.cr new file mode 100644 index 00000000..eba7ab17 --- /dev/null +++ b/spec/tenant_consent_spec.cr @@ -0,0 +1,114 @@ +require "./helper" + +module PlaceOS::Api + describe TenantConsent do + describe ".consent_state" do + # `state` comes back from Microsoft through the user's browser and is the + # only thing telling the callback which authority to reconfigure. The + # callback cannot be authenticated - Microsoft redirects to it - so these + # properties are the whole of its protection. + it "redeems a token for the authority it was issued to" do + authority_id = "authority-#{Random::Secure.hex(4)}" + token = ConsentState.issue(authority_id) + + token.should_not eq authority_id + ConsentState.consume(token).should eq authority_id + end + + it "refuses a token a second time" do + # A captured callback URL must be worthless once it has been used. + authority_id = "authority-#{Random::Secure.hex(4)}" + token = ConsentState.issue(authority_id) + + ConsentState.consume(token).should eq authority_id + ConsentState.consume(token).should be_nil + end + + it "refuses a token it never issued" do + # i.e. an attacker naming an authority directly, which is what the + # parameter used to be. + ConsentState.consume("authority-1").should be_nil + ConsentState.consume(UUID.random.to_s).should be_nil + end + end + + describe ".upsert_calendar_tenant" do + app = {client_id: UUID.v4.to_s, client_secret: "sup3r-s3cret"} + azure_tenant = UUID.v4.to_s + + it "creates a staff-api tenant holding the app-only credential when the domain has none" do + domain = "consent-create-#{Random::Secure.hex(4)}.example.com" + begin + tenant = TenantConsent.upsert_calendar_tenant(domain, azure_tenant, app, "Test Org") + + tenant.persisted?.should be_true + tenant.name.should eq "Test Org" + tenant.platform.should eq "office365" + tenant.delegated.should be_false + + stored = ::PlaceOS::Model::Tenant.find_by?(domain: domain).not_nil! + # encrypted at rest, and decrypts back to exactly the credential written + stored.credentials.starts_with?('\e').should be_true + creds = JSON.parse(PlaceOS::Encryption.decrypt(string: stored.credentials, id: domain, level: :never_display)) + creds["tenant"].as_s.should eq azure_tenant + creds["client_id"].as_s.should eq app[:client_id] + creds["client_secret"].as_s.should eq app[:client_secret] + + # the app-only calendar client builds from the stored credential + stored.place_calendar_client.should be_a(PlaceCalendar::Client) + ensure + ::PlaceOS::Model::Tenant.find_by?(domain: domain).try &.destroy + end + end + + # the flow can be driven from Backoffice on any host, so it must key off + # the authority being integrated, never the host in the request + it "writes to the authority's own domain, leaving another domain's tenant alone" do + authority_domain = "consent-authority-#{Random::Secure.hex(4)}.example.com" + other_domain = "consent-bystander-#{Random::Secure.hex(4)}.example.com" + begin + bystander = ::PlaceOS::Model::Tenant.create!( + name: "Bystander", domain: other_domain, platform: "office365", + delegated: true, credentials: {conference_type: "teamsForBusiness"}.to_json, + ) + before = ::PlaceOS::Model::Tenant.find!(bystander.id).credentials + + TenantConsent.upsert_calendar_tenant(authority_domain, azure_tenant, app, "Authority Org") + + ::PlaceOS::Model::Tenant.find_by?(domain: authority_domain).should_not be_nil + untouched = ::PlaceOS::Model::Tenant.find!(bystander.id) + untouched.delegated.should be_true + untouched.credentials.should eq before + ensure + ::PlaceOS::Model::Tenant.find_by?(domain: authority_domain).try &.destroy + ::PlaceOS::Model::Tenant.find_by?(domain: other_domain).try &.destroy + end + end + + it "switches an existing delegated tenant onto the app-only credential" do + domain = "consent-update-#{Random::Secure.hex(4)}.example.com" + begin + existing = ::PlaceOS::Model::Tenant.create!( + name: "Existing", domain: domain, platform: "office365", + delegated: true, credentials: {conference_type: "teamsForBusiness"}.to_json, + ) + + tenant = TenantConsent.upsert_calendar_tenant(domain, azure_tenant, app, "ignored - tenant exists") + + # same row updated, not a duplicate + tenant.id.should eq existing.id + ::PlaceOS::Model::Tenant.where(domain: domain).count.should eq 1 + + stored = ::PlaceOS::Model::Tenant.find_by?(domain: domain).not_nil! + stored.name.should eq "Existing" + stored.delegated.should be_false + creds = JSON.parse(PlaceOS::Encryption.decrypt(string: stored.credentials, id: domain, level: :never_display)) + creds["client_id"].as_s.should eq app[:client_id] + stored.place_calendar_client.should be_a(PlaceCalendar::Client) + ensure + ::PlaceOS::Model::Tenant.find_by?(domain: domain).try &.destroy + end + end + end + end +end diff --git a/src/placeos-rest-api/controllers/tenant_consent.cr b/src/placeos-rest-api/controllers/tenant_consent.cr index 11b44097..2554506f 100644 --- a/src/placeos-rest-api/controllers/tenant_consent.cr +++ b/src/placeos-rest-api/controllers/tenant_consent.cr @@ -6,8 +6,15 @@ module PlaceOS::Api class TenantConsent < Application base "/api/engine/v2/admin_consent" - skip_action :authorize!, only: [:index, :azure_admin_consent_callback, :flow_status] - skip_action :set_user_id, only: [:index, :azure_admin_consent_callback, :flow_status] + # Only the callback is unauthenticated, and it has to be: Microsoft + # redirects the browser here, so there is no session to present. It is + # guarded instead by the single-use `state` token minted below — see + # `ConsentState`. Starting a flow requires an administrator, because that + # is what decides which authority the callback is allowed to reconfigure. + skip_action :authorize!, only: [:azure_admin_consent_callback, :flow_status] + skip_action :set_user_id, only: [:azure_admin_consent_callback, :flow_status] + + before_action :check_admin, only: [:index] @[AC::Route::Filter(:before_action)] def get_host @@ -29,7 +36,11 @@ module PlaceOS::Api authority = ::PlaceOS::Model::Authority.find!(id) update_app_redirect_uri callback_url = URI.encode_www_form(redirect_url) - consent_url = "https://login.microsoftonline.com/common/adminconsent?client_id=#{PLACE_APP_CLIENT_ID}&redirect_uri=#{callback_url}&state=#{authority.id.as(String)}" + # Not the authority id. `state` comes back from Microsoft through the + # user's browser and is the only thing telling the callback which + # authority to reconfigure, so it has to be unguessable and single use. + state = ConsentState.issue(authority.id.as(String)) + consent_url = "https://login.microsoftonline.com/common/adminconsent?client_id=#{PLACE_APP_CLIENT_ID}&redirect_uri=#{callback_url}&state=#{URI.encode_www_form(state)}" render json: {"url": consent_url} end @@ -46,7 +57,17 @@ module PlaceOS::Api @[AC::Param::Info(description: "Description of the error", example: "The admin denied the request")] error_description : String? = nil, ) : Nil - if ((consent = admin_consent) && consent) && (tenant_id = tenant) && (authority_id = state) + if ((consent = admin_consent) && consent) && (tenant_id = tenant) && (consent_state = state) + # Redeem the token rather than trusting the parameter. This is what + # stops an unauthenticated caller naming an authority of their choosing + # and having the rest of this method reconfigure it. Redeeming is + # single use, so a captured callback URL cannot be replayed either. + authority_id = ConsentState.consume(consent_state) + unless authority_id + Log.warn { "Rejected admin consent callback with an unknown, expired or already used state" } + raise Error::NotFound.new("Invalid state value returned in admin consent") + end + Log.info { "Received admin consent for tenant #{tenant_id} under authority #{authority_id}" } authority = ::PlaceOS::Model::Authority.find?(authority_id) raise Error::NotFound.new("Invalid state value returned in admin consent") unless authority @@ -84,17 +105,25 @@ module PlaceOS::Api @flow : AdminConsentFlow? = nil private def run_consent_flow(flow : AdminConsentFlow, tenant_id : String, authority : ::PlaceOS::Model::Authority) : Nil + # Everything this flow configures belongs to the authority being + # integrated, which is not necessarily the host the admin happens to be + # browsing - Backoffice can drive the flow for any domain. + authority_domain = authority.domain + flow.start_step("visualiser") - create_app(tenant_id) + visualiser_app = create_app(tenant_id) + + flow.start_step("calendar") + self.class.upsert_calendar_tenant(authority_domain, tenant_id, visualiser_app, authority.name) flow.start_step("auth_app") strat = create_strat(tenant_id, authority.id.as(String)) - auth_app = create_delegated_app(tenant_id, authority.domain, strat.id.as(String)) + auth_app = create_delegated_app(tenant_id, authority_domain, strat.id.as(String)) flow.start_step("outlook") create_outlook_repo - add_outlook_plugin_auth(auth_app[:client_id]) - create_outlook_config(auth_app[:client_id]) + add_outlook_plugin_auth(auth_app[:client_id], authority_domain) + create_outlook_config(auth_app[:client_id], authority_domain) flow.start_step("saving") strat.update!(client_id: auth_app[:client_id], client_secret: auth_app[:client_secret]) @@ -102,9 +131,9 @@ module PlaceOS::Api flow.complete! Log.info { {message: "admin consent flow complete", flow_id: flow.id, authority_id: flow.authority_id} } - rescue flow_error - Log.error(exception: flow_error) { {message: "admin consent flow failed", flow_id: flow.id, authority_id: flow.authority_id} } - flow.fail!(flow_error.message || flow_error.class.name) + rescue error + Log.error(exception: error) { {message: "admin consent flow failed", flow_id: flow.id, authority_id: flow.authority_id} } + flow.fail!(error.message || error.class.name) end # surfaces replication-retry waits on the progress page @@ -225,6 +254,7 @@ module PlaceOS::Api ra << {id: "ef54d2bf-783f-4e0f-bca1-3210c0444d99", type: "Role"} # Calendars.ReadWrite ra << {id: "5b567255-7703-4780-807c-7be8301ae99b", type: "Role"} # Group.Read.All ra << {id: "df021288-bdef-4463-88db-98f22de89214", type: "Role"} # User.Read.All + ra << {id: "913b9306-0ce1-42b8-9137-6a7df690a760", type: "Role"} # Place.Read.All - room discovery via the places API client = get_client(tenant_id) @@ -240,7 +270,8 @@ module PlaceOS::Api end end - created_app.app_id.as(String) + secret = GraphReplicationRetry.run(on_retry: replication_progress) { client.application_add_pwd(created_app.app_id.as(String), "PlaceOS Bookings Visualiser Secret") } + {client_id: created_app.app_id.as(String), client_secret: secret.secret_text.as(String)} end private def create_delegated_app(tenant_id : String, domain : String, strat_id : String) @@ -296,16 +327,16 @@ module PlaceOS::Api end end - private def add_outlook_plugin_auth(app_id : String) : Nil + private def add_outlook_plugin_auth(app_id : String, domain : String) : Nil client = get_client app = GraphReplicationRetry.run(on_retry: replication_progress) { client.get_application(app_id) } app_redirect_uris = app.web.try &.redirect_uris || [] of String - app_redirect_uris.push("#{domain_url}/outlook/#/book/spaces") + app_redirect_uris.push("https://#{domain}/outlook/#/book/spaces") scope_id = UUID.v4.to_s updated = { - "identifierUris": ["api://#{domain_host}/#{app_id}"], + "identifierUris": ["api://#{domain}/#{app_id}"], "web": { "redirectUris": app_redirect_uris, "implicitGrantSettings": {"enableAccessTokenIssuance" => true, "enableIdTokenIssuance" => true}, @@ -342,6 +373,36 @@ module PlaceOS::Api GraphReplicationRetry.run(on_retry: replication_progress) { client.update_application(app_id, updated.to_json) } end + # Store the visualiser app's app-only Graph credential in the staff-api + # tenant for this domain - this is what gives PlaceOS calendar access + # without a signed-in user. The flow owns the domain's Microsoft + # configuration (like login_url and outlook_config), so an existing + # tenant is switched to these credentials; delegated mode can be + # re-enabled afterwards in Backoffice if a customer prefers it. + def self.upsert_calendar_tenant(domain : String, azure_tenant_id : String, app : NamedTuple(client_id: String, client_secret: String), name : String?) : ::PlaceOS::Model::Tenant + credentials = { + tenant: azure_tenant_id, + client_id: app[:client_id], + client_secret: app[:client_secret], + }.to_json + + if tenant = ::PlaceOS::Model::Tenant.find_by?(domain: domain) + tenant.platform = "office365" + tenant.delegated = false + tenant.credentials = credentials + tenant.save! + tenant + else + ::PlaceOS::Model::Tenant.create!( + name: name, + domain: domain, + platform: "office365", + delegated: false, + credentials: credentials, + ) + end + end + private def create_outlook_repo : Nil return if on_primary { ::PlaceOS::Model::Repository.where(name: "Outlook Plugin", uri: "https://github.com/placeos/user-interfaces", branch: "build/outlook-rooms-addin/prod", folder_name: "outlookplugin", repo_type: ::PlaceOS::Model::Repository::Type::Interface.value).count } > 0 @@ -352,16 +413,16 @@ module PlaceOS::Api ) end - private def create_outlook_config(app_id : String) : Nil - tenant = ::PlaceOS::Model::Tenant.find_by?(domain: domain_host) + private def create_outlook_config(app_id : String, domain : String) : Nil + tenant = ::PlaceOS::Model::Tenant.find_by?(domain: domain) unless tenant - Log.error { {message: "Tenant not found", domain: domain_host} } + Log.error { {message: "Tenant not found", domain: domain} } return end outlook_config = { - app_id: app_id, base_path: "outlook", app_domain: "#{domain_url}/outlook/", - app_resource: "api://#{domain_host}/#{app_id}", source_location: "", + app_id: app_id, base_path: "outlook", app_domain: "https://#{domain}/outlook/", + app_resource: "api://#{domain}/#{app_id}", source_location: "", } tenant.outlook_config = ::PlaceOS::Model::Tenant::OutlookConfig.from_json(outlook_config.to_json) tenant.save! diff --git a/src/placeos-rest-api/utilities/admin-consent-flow.cr b/src/placeos-rest-api/utilities/admin-consent-flow.cr index b3a33ded..994825f2 100644 --- a/src/placeos-rest-api/utilities/admin-consent-flow.cr +++ b/src/placeos-rest-api/utilities/admin-consent-flow.cr @@ -14,6 +14,7 @@ module PlaceOS::Api STEP_DEFINITIONS = [ {"visualiser", "Register the Bookings Visualiser application"}, + {"calendar", "Connect room calendar access"}, {"auth_app", "Register the User Authentication application"}, {"outlook", "Configure the Outlook add-in"}, {"saving", "Save the authentication configuration"}, @@ -62,7 +63,7 @@ module PlaceOS::Api end def complete! : Nil - @steps.each { |step| step.state = "done" } + @steps.each(&.state=("done")) @state = "complete" @detail = nil save diff --git a/src/placeos-rest-api/utilities/consent-state.cr b/src/placeos-rest-api/utilities/consent-state.cr new file mode 100644 index 00000000..f351715e --- /dev/null +++ b/src/placeos-rest-api/utilities/consent-state.cr @@ -0,0 +1,57 @@ +require "uuid" + +module PlaceOS::Api + # The `state` parameter for an Azure admin-consent round trip. + # + # Microsoft redirects the browser to `/admin_consent/callback`, so that route + # cannot carry a session and cannot be authenticated. Everything the callback + # goes on to do — registering applications, writing an oauth strat, replacing + # the staff API tenant's calendar credentials, and overwriting the authority's + # `login_url` — is therefore driven entirely by its query parameters. + # + # Passing the authority id there directly, as this flow used to, means anyone + # who can reach the deployment can point any authority at an identity provider + # they control, because `state` was echoed rather than verified. + # + # So `state` is an opaque single-use token instead. It exists only because an + # authenticated administrator asked to start a flow for a specific authority, + # it names that authority server side rather than carrying it in the URL, it + # expires, and redeeming it destroys it. A captured callback URL is worthless + # once used, and a guessed one is worth nothing at all. + module ConsentState + # Long enough for an admin to read Microsoft's consent screen and decide. + TTL_SECONDS = 900 + + # Records a pending flow for `authority_id` and returns the token to send + # to Microsoft as `state`. + def self.issue(authority_id : String) : String + token = UUID.random.to_s + ::PlaceOS::Driver::RedisStorage.with_redis(&.set( + redis_key(token), authority_id, ex: TTL_SECONDS + )) + token + end + + # Redeems `token`, returning the authority it was issued for, or `nil` if it + # is unknown, expired, or already used. + def self.consume(token : String) : String? + key = redis_key(token) + ::PlaceOS::Driver::RedisStorage.with_redis do |redis| + authority_id = redis.get(key) + next nil unless authority_id + + # `del` reports how many keys it removed. Anything other than one means + # a concurrent request redeemed this token first, and only that request + # may proceed — otherwise a replayed callback would still be honoured + # in the window before the delete lands. + next nil unless redis.del(key) == 1 + + authority_id + end + end + + def self.redis_key(token : String) : String + "placeos:admin_consent:state:#{token}" + end + end +end