From 8906d2d5944189d4ed826ab9f5e202e047052fb4 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 22:33:02 +0100 Subject: [PATCH 1/2] ak-sysd: allow username hint for interactive async auth --- Cargo.lock | 2 +- Cargo.toml | 2 +- .../src/generated/sys_auth/sys_auth.rs | 19 +- .../src/generated/sys_auth/sys_auth.serde.rs | 91 ++++++++++ .../src/generated/sys_auth/sys_auth.tonic.rs | 13 +- .../auth/interactive/interactive_txn.rs | 2 +- .../src/components/auth/interactive/mod.rs | 3 +- ak-sysd/src/components/auth/mod.rs | 8 +- ee/psso/Bridge/Generated/sys_auth.grpc.swift | 29 ++-- ee/psso/Bridge/Generated/sys_auth.pb.swift | 153 +++++++++++------ ee/psso/Bridge/SysdBridge.swift | 3 +- ee/wcp/cef-host/src/app.rs | 17 +- ee/wcp/cef-host/src/main.rs | 5 +- ee/wcp/cef-host/src/sysd.rs | 11 +- ee/wcp/credprovider/src/credential.rs | 162 +++++++++++++++--- ee/wcp/credprovider/src/ipc.rs | 115 +++++++++++-- ee/wcp/e2e/src/mock_sysd.rs | 32 +++- ee/wcp/e2e/tests/sign_in_flow.rs | 13 +- protobuf/sys_auth.proto | 16 +- 19 files changed, 558 insertions(+), 138 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c8e0ad25..ab1224cf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -809,7 +809,7 @@ checksum = "1505bd5d3d116872e7271a6d4e16d81d0c8570876c8de68093a09ac269d8aac0" [[package]] name = "authentik-client" version = "2026.11.0-rc1" -source = "git+https://github.com/goauthentik/client-rust#529d7b8a5018ee030d6ad254acfa021469f27f20" +source = "git+https://github.com/goauthentik/client-rust?rev=24c227964b05ff1c3cc9487ccdd40751ac89f6b9#24c227964b05ff1c3cc9487ccdd40751ac89f6b9" dependencies = [ "chrono", "reqwest 0.13.4", diff --git a/Cargo.toml b/Cargo.toml index 64cf1179..787c8d29 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,7 +43,7 @@ strip = "debuginfo" [workspace.dependencies] ak-api-cli-gen = { path = "ak-api-cli-gen" } -authentik-client = { git = "https://github.com/goauthentik/client-rust", version = "2026.11.0-rc1" , default-features = false, features = ["rustls"] } +authentik-client = { git = "https://github.com/goauthentik/client-rust", version = "2026.11.0-rc1" , default-features = false, features = ["rustls"], rev = "24c227964b05ff1c3cc9487ccdd40751ac89f6b9" } chrono = { version = "0.4.45", features = ["serde"] } color-eyre = "= 0.6.5" eyre = "= 0.6.14" diff --git a/ak-platform/src/generated/sys_auth/sys_auth.rs b/ak-platform/src/generated/sys_auth/sys_auth.rs index 57bfc2fe..a263e003 100644 --- a/ak-platform/src/generated/sys_auth/sys_auth.rs +++ b/ak-platform/src/generated/sys_auth/sys_auth.rs @@ -58,13 +58,6 @@ pub mod interactive_auth_request { } } #[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] -pub struct InteractiveAuthAsyncResponse { - #[prost(string, tag="1")] - pub url: ::prost::alloc::string::String, - #[prost(string, tag="2")] - pub header_token: ::prost::alloc::string::String, -} -#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] pub struct InteractiveChallenge { #[prost(string, tag="1")] pub txid: ::prost::alloc::string::String, @@ -131,6 +124,18 @@ pub mod interactive_challenge { } } #[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct InteractiveAuthAsyncRequest { + #[prost(string, optional, tag="1")] + pub username: ::core::option::Option<::prost::alloc::string::String>, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct InteractiveAuthAsyncResponse { + #[prost(string, tag="1")] + pub url: ::prost::alloc::string::String, + #[prost(string, tag="2")] + pub header_token: ::prost::alloc::string::String, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] pub struct SystemAuthorizeRequest { #[prost(string, tag="1")] pub session_id: ::prost::alloc::string::String, diff --git a/ak-platform/src/generated/sys_auth/sys_auth.serde.rs b/ak-platform/src/generated/sys_auth/sys_auth.serde.rs index 4340e845..c963cf0a 100644 --- a/ak-platform/src/generated/sys_auth/sys_auth.serde.rs +++ b/ak-platform/src/generated/sys_auth/sys_auth.serde.rs @@ -1,4 +1,95 @@ // @generated +impl serde::Serialize for InteractiveAuthAsyncRequest { + #[allow(deprecated)] + fn serialize(&self, serializer: S) -> std::result::Result + where + S: serde::Serializer, + { + use serde::ser::SerializeStruct; + let mut len = 0; + if self.username.is_some() { + len += 1; + } + let mut struct_ser = serializer.serialize_struct("sys_auth.InteractiveAuthAsyncRequest", len)?; + if let Some(v) = self.username.as_ref() { + struct_ser.serialize_field("username", v)?; + } + struct_ser.end() + } +} +impl<'de> serde::Deserialize<'de> for InteractiveAuthAsyncRequest { + #[allow(deprecated)] + fn deserialize(deserializer: D) -> std::result::Result + where + D: serde::Deserializer<'de>, + { + const FIELDS: &[&str] = &[ + "username", + ]; + + #[allow(clippy::enum_variant_names)] + enum GeneratedField { + Username, + } + impl<'de> serde::Deserialize<'de> for GeneratedField { + fn deserialize(deserializer: D) -> std::result::Result + where + D: serde::Deserializer<'de>, + { + struct GeneratedVisitor; + + impl<'de> serde::de::Visitor<'de> for GeneratedVisitor { + type Value = GeneratedField; + + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(formatter, "expected one of: {:?}", FIELDS) + } + + #[allow(unused_variables)] + fn visit_str(self, value: &str) -> std::result::Result + where + E: serde::de::Error, + { + match value { + "username" => Ok(GeneratedField::Username), + _ => Err(serde::de::Error::unknown_field(value, FIELDS)), + } + } + } + deserializer.deserialize_identifier(GeneratedVisitor) + } + } + struct GeneratedVisitor; + impl<'de> serde::de::Visitor<'de> for GeneratedVisitor { + type Value = InteractiveAuthAsyncRequest; + + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("struct sys_auth.InteractiveAuthAsyncRequest") + } + + fn visit_map(self, mut map_: V) -> std::result::Result + where + V: serde::de::MapAccess<'de>, + { + let mut username__ = None; + while let Some(k) = map_.next_key()? { + match k { + GeneratedField::Username => { + if username__.is_some() { + return Err(serde::de::Error::duplicate_field("username")); + } + username__ = map_.next_value()?; + } + } + } + Ok(InteractiveAuthAsyncRequest { + username: username__, + }) + } + } + deserializer.deserialize_struct("sys_auth.InteractiveAuthAsyncRequest", FIELDS, GeneratedVisitor) + } +} impl serde::Serialize for InteractiveAuthAsyncResponse { #[allow(deprecated)] fn serialize(&self, serializer: S) -> std::result::Result diff --git a/ak-platform/src/generated/sys_auth/sys_auth.tonic.rs b/ak-platform/src/generated/sys_auth/sys_auth.tonic.rs index aec77da8..c3c9b23b 100644 --- a/ak-platform/src/generated/sys_auth/sys_auth.tonic.rs +++ b/ak-platform/src/generated/sys_auth/sys_auth.tonic.rs @@ -493,7 +493,7 @@ pub mod system_auth_interactive_client { } pub async fn interactive_auth_async( &mut self, - request: impl tonic::IntoRequest<()>, + request: impl tonic::IntoRequest, ) -> std::result::Result< tonic::Response, tonic::Status, @@ -544,7 +544,7 @@ pub mod system_auth_interactive_server { >; async fn interactive_auth_async( &self, - request: tonic::Request<()>, + request: tonic::Request, ) -> std::result::Result< tonic::Response, tonic::Status, @@ -679,14 +679,19 @@ pub mod system_auth_interactive_server { "/sys_auth.SystemAuthInteractive/InteractiveAuthAsync" => { #[allow(non_camel_case_types)] struct InteractiveAuthAsyncSvc(pub Arc); - impl tonic::server::UnaryService<()> + impl< + T: SystemAuthInteractive, + > tonic::server::UnaryService for InteractiveAuthAsyncSvc { type Response = super::InteractiveAuthAsyncResponse; type Future = BoxFuture< tonic::Response, tonic::Status, >; - fn call(&mut self, request: tonic::Request<()>) -> Self::Future { + fn call( + &mut self, + request: tonic::Request, + ) -> Self::Future { let inner = Arc::clone(&self.0); let fut = async move { ::interactive_auth_async( diff --git a/ak-sysd/src/components/auth/interactive/interactive_txn.rs b/ak-sysd/src/components/auth/interactive/interactive_txn.rs index 8066cdaa..54e794dc 100644 --- a/ak-sysd/src/components/auth/interactive/interactive_txn.rs +++ b/ak-sysd/src/components/auth/interactive/interactive_txn.rs @@ -179,7 +179,7 @@ impl InteractiveAuthTransaction { /// Exchanges the authenticated flow session for a one-time token via the /// finish redirect, then mints a real session from it. async fn finish_success(&mut self) -> Result { - let ia = endpoints_agents_connectors_auth_ia_create(&self.domain.api) + let ia = endpoints_agents_connectors_auth_ia_create(&self.domain.api, None) .await .map_err(|e| Status::internal(format!("failed to start interactive auth: {e}")))?; diff --git a/ak-sysd/src/components/auth/interactive/mod.rs b/ak-sysd/src/components/auth/interactive/mod.rs index 03b354ed..53f41a0d 100644 --- a/ak-sysd/src/components/auth/interactive/mod.rs +++ b/ak-sysd/src/components/auth/interactive/mod.rs @@ -123,6 +123,7 @@ async fn interactive_auth_continue( pub async fn interactive_auth_async( ctx: &SysdContext, + username: Option, ) -> Result { if !interactive_supported(ctx).await { return Err(Status::unavailable( @@ -130,7 +131,7 @@ pub async fn interactive_auth_async( )); } let active = ctx.domains.active().await.map_err(to_status)?; - let ia = endpoints_agents_connectors_auth_ia_create(&active.api) + let ia = endpoints_agents_connectors_auth_ia_create(&active.api, username.as_deref()) .await .map_err(|e| Status::internal(format!("failed to start interactive auth: {e}")))?; Ok(InteractiveAuthAsyncResponse { diff --git a/ak-sysd/src/components/auth/mod.rs b/ak-sysd/src/components/auth/mod.rs index a15da907..1b6b4498 100644 --- a/ak-sysd/src/components/auth/mod.rs +++ b/ak-sysd/src/components/auth/mod.rs @@ -1,7 +1,7 @@ use crate::components::{Component, SysdContext}; use ak_platform::generated::sys_auth::{ - InteractiveAuthAsyncResponse, InteractiveAuthRequest, InteractiveChallenge, - SystemAuthorizeRequest, SystemAuthorizeResponse, + InteractiveAuthAsyncRequest, InteractiveAuthAsyncResponse, InteractiveAuthRequest, + InteractiveChallenge, SystemAuthorizeRequest, SystemAuthorizeResponse, system_auth_authorize_server::{SystemAuthAuthorize, SystemAuthAuthorizeServer}, system_auth_interactive_server::{SystemAuthInteractive, SystemAuthInteractiveServer}, system_auth_token_server::SystemAuthTokenServer, @@ -107,9 +107,9 @@ impl SystemAuthInteractive for AuthComponent { async fn interactive_auth_async( &self, - _request: Request<()>, + request: Request, ) -> Result, Status> { - interactive::interactive_auth_async(&self.ctx) + interactive::interactive_auth_async(&self.ctx, request.into_inner().username) .await .map(Response::new) } diff --git a/ee/psso/Bridge/Generated/sys_auth.grpc.swift b/ee/psso/Bridge/Generated/sys_auth.grpc.swift index 2a6e6986..e7a99aab 100644 --- a/ee/psso/Bridge/Generated/sys_auth.grpc.swift +++ b/ee/psso/Bridge/Generated/sys_auth.grpc.swift @@ -10,7 +10,6 @@ internal import GRPCCore internal import GRPCProtobuf -internal import SwiftProtobuf // MARK: - sys_auth.SystemAuthToken @@ -360,7 +359,7 @@ internal enum SystemAuthInteractive: Sendable { /// Namespace for "InteractiveAuthAsync" metadata. internal enum InteractiveAuthAsync: Sendable { /// Request type for "InteractiveAuthAsync". - internal typealias Input = SwiftProtobuf.Google_Protobuf_Empty + internal typealias Input = InteractiveAuthAsyncRequest /// Response type for "InteractiveAuthAsync". internal typealias Output = InteractiveAuthAsyncResponse /// Descriptor for "InteractiveAuthAsync". @@ -423,8 +422,8 @@ extension SystemAuthInteractive { /// > Interactive auth which is handed of to a browser /// /// - Parameters: - /// - request: A request containing a single `SwiftProtobuf.Google_Protobuf_Empty` message. - /// - serializer: A serializer for `SwiftProtobuf.Google_Protobuf_Empty` messages. + /// - request: A request containing a single `InteractiveAuthAsyncRequest` message. + /// - serializer: A serializer for `InteractiveAuthAsyncRequest` messages. /// - deserializer: A deserializer for `InteractiveAuthAsyncResponse` messages. /// - options: Options to apply to this RPC. /// - handleResponse: A closure which handles the response, the result of which is @@ -432,8 +431,8 @@ extension SystemAuthInteractive { /// hasn't already finished. /// - Returns: The result of `handleResponse`. func interactiveAuthAsync( - request: GRPCCore.ClientRequest, - serializer: some GRPCCore.MessageSerializer, + request: GRPCCore.ClientRequest, + serializer: some GRPCCore.MessageSerializer, deserializer: some GRPCCore.MessageDeserializer, options: GRPCCore.CallOptions, onResponse handleResponse: @Sendable @escaping (GRPCCore.ClientResponse) async throws -> Result @@ -497,8 +496,8 @@ extension SystemAuthInteractive { /// > Interactive auth which is handed of to a browser /// /// - Parameters: - /// - request: A request containing a single `SwiftProtobuf.Google_Protobuf_Empty` message. - /// - serializer: A serializer for `SwiftProtobuf.Google_Protobuf_Empty` messages. + /// - request: A request containing a single `InteractiveAuthAsyncRequest` message. + /// - serializer: A serializer for `InteractiveAuthAsyncRequest` messages. /// - deserializer: A deserializer for `InteractiveAuthAsyncResponse` messages. /// - options: Options to apply to this RPC. /// - handleResponse: A closure which handles the response, the result of which is @@ -506,8 +505,8 @@ extension SystemAuthInteractive { /// hasn't already finished. /// - Returns: The result of `handleResponse`. internal func interactiveAuthAsync( - request: GRPCCore.ClientRequest, - serializer: some GRPCCore.MessageSerializer, + request: GRPCCore.ClientRequest, + serializer: some GRPCCore.MessageSerializer, deserializer: some GRPCCore.MessageDeserializer, options: GRPCCore.CallOptions = .defaults, onResponse handleResponse: @Sendable @escaping (GRPCCore.ClientResponse) async throws -> Result = { response in @@ -565,14 +564,14 @@ extension SystemAuthInteractive.ClientProtocol { /// > Interactive auth which is handed of to a browser /// /// - Parameters: - /// - request: A request containing a single `SwiftProtobuf.Google_Protobuf_Empty` message. + /// - request: A request containing a single `InteractiveAuthAsyncRequest` message. /// - options: Options to apply to this RPC. /// - handleResponse: A closure which handles the response, the result of which is /// returned to the caller. Returning from the closure will cancel the RPC if it /// hasn't already finished. /// - Returns: The result of `handleResponse`. internal func interactiveAuthAsync( - request: GRPCCore.ClientRequest, + request: GRPCCore.ClientRequest, options: GRPCCore.CallOptions = .defaults, onResponse handleResponse: @Sendable @escaping (GRPCCore.ClientResponse) async throws -> Result = { response in try response.message @@ -580,7 +579,7 @@ extension SystemAuthInteractive.ClientProtocol { ) async throws -> Result where Result: Sendable { try await self.interactiveAuthAsync( request: request, - serializer: GRPCProtobuf.ProtobufSerializer(), + serializer: GRPCProtobuf.ProtobufSerializer(), deserializer: GRPCProtobuf.ProtobufDeserializer(), options: options, onResponse: handleResponse @@ -639,14 +638,14 @@ extension SystemAuthInteractive.ClientProtocol { /// hasn't already finished. /// - Returns: The result of `handleResponse`. internal func interactiveAuthAsync( - _ message: SwiftProtobuf.Google_Protobuf_Empty, + _ message: InteractiveAuthAsyncRequest, metadata: GRPCCore.Metadata = [:], options: GRPCCore.CallOptions = .defaults, onResponse handleResponse: @Sendable @escaping (GRPCCore.ClientResponse) async throws -> Result = { response in try response.message } ) async throws -> Result where Result: Sendable { - let request = GRPCCore.ClientRequest( + let request = GRPCCore.ClientRequest( message: message, metadata: metadata ) diff --git a/ee/psso/Bridge/Generated/sys_auth.pb.swift b/ee/psso/Bridge/Generated/sys_auth.pb.swift index 525a9b8b..da489b83 100644 --- a/ee/psso/Bridge/Generated/sys_auth.pb.swift +++ b/ee/psso/Bridge/Generated/sys_auth.pb.swift @@ -185,20 +185,6 @@ nonisolated struct InteractiveAuthRequest: Sendable { init() {} } -nonisolated struct InteractiveAuthAsyncResponse: Sendable { - // SwiftProtobuf.Message conformance is added in an extension below. See the - // `Message` and `Message+*Additions` files in the SwiftProtobuf library for - // methods supported on all messages. - - var url: String = String() - - var headerToken: String = String() - - var unknownFields = SwiftProtobuf.UnknownStorage() - - init() {} -} - nonisolated struct InteractiveChallenge: Sendable { // SwiftProtobuf.Message conformance is added in an extension below. See the // `Message` and `Message+*Additions` files in the SwiftProtobuf library for @@ -283,6 +269,41 @@ nonisolated struct InteractiveChallenge: Sendable { init() {} } +nonisolated struct InteractiveAuthAsyncRequest: Sendable { + // SwiftProtobuf.Message conformance is added in an extension below. See the + // `Message` and `Message+*Additions` files in the SwiftProtobuf library for + // methods supported on all messages. + + var username: String { + get {_username ?? String()} + set {_username = newValue} + } + /// Returns true if `username` has been explicitly set. + var hasUsername: Bool {self._username != nil} + /// Clears the value of `username`. Subsequent reads from it will return its default value. + mutating func clearUsername() {self._username = nil} + + var unknownFields = SwiftProtobuf.UnknownStorage() + + init() {} + + fileprivate var _username: String? = nil +} + +nonisolated struct InteractiveAuthAsyncResponse: Sendable { + // SwiftProtobuf.Message conformance is added in an extension below. See the + // `Message` and `Message+*Additions` files in the SwiftProtobuf library for + // methods supported on all messages. + + var url: String = String() + + var headerToken: String = String() + + var unknownFields = SwiftProtobuf.UnknownStorage() + + init() {} +} + nonisolated struct SystemAuthorizeRequest: Sendable { // SwiftProtobuf.Message conformance is added in an extension below. See the // `Message` and `Message+*Additions` files in the SwiftProtobuf library for @@ -618,41 +639,6 @@ nonisolated extension InteractiveAuthRequest: SwiftProtobuf.Message, SwiftProtob } } -nonisolated extension InteractiveAuthAsyncResponse: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, SwiftProtobuf._ProtoNameProviding { - static let protoMessageName: String = _protobuf_package + ".InteractiveAuthAsyncResponse" - static let _protobuf_nameMap = SwiftProtobuf._NameMap(bytecode: "\0\u{1}url\0\u{3}header_token\0") - - mutating func decodeMessage(decoder: inout D) throws { - while let fieldNumber = try decoder.nextFieldNumber() { - // The use of inline closures is to circumvent an issue where the compiler - // allocates stack space for every case branch when no optimizations are - // enabled. https://github.com/apple/swift-protobuf/issues/1034 - switch fieldNumber { - case 1: try { try decoder.decodeSingularStringField(value: &self.url) }() - case 2: try { try decoder.decodeSingularStringField(value: &self.headerToken) }() - default: break - } - } - } - - func traverse(visitor: inout V) throws { - if !self.url.isEmpty { - try visitor.visitSingularStringField(value: self.url, fieldNumber: 1) - } - if !self.headerToken.isEmpty { - try visitor.visitSingularStringField(value: self.headerToken, fieldNumber: 2) - } - try unknownFields.traverse(visitor: &visitor) - } - - static func ==(lhs: InteractiveAuthAsyncResponse, rhs: InteractiveAuthAsyncResponse) -> Bool { - if lhs.url != rhs.url {return false} - if lhs.headerToken != rhs.headerToken {return false} - if lhs.unknownFields != rhs.unknownFields {return false} - return true - } -} - nonisolated extension InteractiveChallenge: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, SwiftProtobuf._ProtoNameProviding { static let protoMessageName: String = _protobuf_package + ".InteractiveChallenge" static let _protobuf_nameMap = SwiftProtobuf._NameMap(bytecode: "\0\u{1}txid\0\u{1}finished\0\u{1}result\0\u{1}prompt\0\u{3}prompt_meta\0\u{3}debug_info\0\u{3}session_id\0\u{1}component\0") @@ -722,6 +708,75 @@ nonisolated extension InteractiveChallenge.PromptMeta: SwiftProtobuf._ProtoNameP static let _protobuf_nameMap = SwiftProtobuf._NameMap(bytecode: "\0\u{2}\0UNSPECIFIED\0\u{1}PAM_PROMPT_ECHO_OFF\0\u{1}PAM_PROMPT_ECHO_ON\0\u{1}PAM_ERROR_MSG\0\u{1}PAM_TEXT_INFO\0\u{1}PAM_RADIO_TYPE\0\u{2}\u{2}PAM_BINARY_PROMPT\0\u{2}]\u{1}PASSWORD\0") } +nonisolated extension InteractiveAuthAsyncRequest: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, SwiftProtobuf._ProtoNameProviding { + static let protoMessageName: String = _protobuf_package + ".InteractiveAuthAsyncRequest" + static let _protobuf_nameMap = SwiftProtobuf._NameMap(bytecode: "\0\u{1}username\0") + + mutating func decodeMessage(decoder: inout D) throws { + while let fieldNumber = try decoder.nextFieldNumber() { + // The use of inline closures is to circumvent an issue where the compiler + // allocates stack space for every case branch when no optimizations are + // enabled. https://github.com/apple/swift-protobuf/issues/1034 + switch fieldNumber { + case 1: try { try decoder.decodeSingularStringField(value: &self._username) }() + default: break + } + } + } + + func traverse(visitor: inout V) throws { + // The use of inline closures is to circumvent an issue where the compiler + // allocates stack space for every if/case branch local when no optimizations + // are enabled. https://github.com/apple/swift-protobuf/issues/1034 and + // https://github.com/apple/swift-protobuf/issues/1182 + try { if let v = self._username { + try visitor.visitSingularStringField(value: v, fieldNumber: 1) + } }() + try unknownFields.traverse(visitor: &visitor) + } + + static func ==(lhs: InteractiveAuthAsyncRequest, rhs: InteractiveAuthAsyncRequest) -> Bool { + if lhs._username != rhs._username {return false} + if lhs.unknownFields != rhs.unknownFields {return false} + return true + } +} + +nonisolated extension InteractiveAuthAsyncResponse: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, SwiftProtobuf._ProtoNameProviding { + static let protoMessageName: String = _protobuf_package + ".InteractiveAuthAsyncResponse" + static let _protobuf_nameMap = SwiftProtobuf._NameMap(bytecode: "\0\u{1}url\0\u{3}header_token\0") + + mutating func decodeMessage(decoder: inout D) throws { + while let fieldNumber = try decoder.nextFieldNumber() { + // The use of inline closures is to circumvent an issue where the compiler + // allocates stack space for every case branch when no optimizations are + // enabled. https://github.com/apple/swift-protobuf/issues/1034 + switch fieldNumber { + case 1: try { try decoder.decodeSingularStringField(value: &self.url) }() + case 2: try { try decoder.decodeSingularStringField(value: &self.headerToken) }() + default: break + } + } + } + + func traverse(visitor: inout V) throws { + if !self.url.isEmpty { + try visitor.visitSingularStringField(value: self.url, fieldNumber: 1) + } + if !self.headerToken.isEmpty { + try visitor.visitSingularStringField(value: self.headerToken, fieldNumber: 2) + } + try unknownFields.traverse(visitor: &visitor) + } + + static func ==(lhs: InteractiveAuthAsyncResponse, rhs: InteractiveAuthAsyncResponse) -> Bool { + if lhs.url != rhs.url {return false} + if lhs.headerToken != rhs.headerToken {return false} + if lhs.unknownFields != rhs.unknownFields {return false} + return true + } +} + nonisolated extension SystemAuthorizeRequest: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, SwiftProtobuf._ProtoNameProviding { static let protoMessageName: String = _protobuf_package + ".SystemAuthorizeRequest" static let _protobuf_nameMap = SwiftProtobuf._NameMap(bytecode: "\0\u{3}session_id\0\u{1}authz\0") diff --git a/ee/psso/Bridge/SysdBridge.swift b/ee/psso/Bridge/SysdBridge.swift index 7e6ab047..ffe0bfad 100644 --- a/ee/psso/Bridge/SysdBridge.swift +++ b/ee/psso/Bridge/SysdBridge.swift @@ -93,7 +93,8 @@ public class SysdBridge { return try await self.withClient { client in let res = SystemAuthInteractive.Client(wrapping: client) let url = try await res.interactiveAuthAsync( - request: ClientRequest(message: Google_Protobuf_Empty()) + request: ClientRequest( + message: InteractiveAuthAsyncRequest.init()) ) return AKInteractiveAuth(URL: url.url, DTH: url.headerToken) } diff --git a/ee/wcp/cef-host/src/app.rs b/ee/wcp/cef-host/src/app.rs index 70d31f4a..78200695 100644 --- a/ee/wcp/cef-host/src/app.rs +++ b/ee/wcp/cef-host/src/app.rs @@ -10,11 +10,12 @@ wrap_app! { pub struct HostApp { result_pipe: usize, cancel_pipe: Option, + login_hint: Option, } impl App { fn browser_process_handler(&self) -> Option { - Some(HostBrowserProcessHandler::new(self.result_pipe, self.cancel_pipe)) + Some(HostBrowserProcessHandler::new(self.result_pipe, self.cancel_pipe, self.login_hint.clone())) } } } @@ -23,6 +24,7 @@ wrap_browser_process_handler! { struct HostBrowserProcessHandler { result_pipe: usize, cancel_pipe: Option, + login_hint: Option, } impl BrowserProcessHandler { @@ -34,7 +36,7 @@ wrap_browser_process_handler! { // re-entrantly, which also keeps a blocking gRPC call out of // `CefInitialize`. fn on_context_initialized(&self) { - let mut task = OpenSignInWindow::new(self.result_pipe, self.cancel_pipe); + let mut task = OpenSignInWindow::new(self.result_pipe, self.cancel_pipe, self.login_hint.clone()); post_task(ThreadId::UI, Some(&mut task)); } } @@ -44,22 +46,27 @@ wrap_task! { struct OpenSignInWindow { result_pipe: usize, cancel_pipe: Option, + login_hint: Option, } impl Task { fn execute(&self) { - open_sign_in_window(self.result_pipe, self.cancel_pipe); + open_sign_in_window(self.result_pipe, self.cancel_pipe, self.login_hint.clone()); } } } -fn open_sign_in_window(result_pipe: usize, cancel_pipe: Option) { +fn open_sign_in_window(result_pipe: usize, cancel_pipe: Option, login_hint: Option) { // Most of the gap between the spawn and a window existing is spent here — // a named-pipe round trip to `ak-sysd` and, behind it, a live call out to // the authentik API. That gap is what the foreground grant issued at spawn // has to survive, so it is worth knowing how long it actually was. let started = std::time::Instant::now(); - let start = match crate::sysd::sys_auth_start_async() { + log::info!( + "starting interactive auth with login hint {}", + login_hint.as_deref().unwrap_or("") + ); + let start = match crate::sysd::sys_auth_start_async(login_hint) { Ok(s) => s, Err(e) => { log::error!("sys_auth_start_async failed: {e}"); diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index a54b2039..3cdc30b4 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -143,6 +143,9 @@ fn main() { return; }; let cancel_pipe = arg_value("--cancel-pipe").and_then(|s| s.parse::().ok()); + // Absent whenever the credential provider had no username to offer; the + // sign-in page then asks for one as it always did. + let login_hint = arg_value("--login-hint").filter(|hint| !hint.is_empty()); wipe_browser_state(Path::new(ROOT_CACHE_PATH)); @@ -154,7 +157,7 @@ fn main() { log_severity: LogSeverity::VERBOSE, ..Default::default() }; - let mut app = app::HostApp::new(result_pipe, cancel_pipe); + let mut app = app::HostApp::new(result_pipe, cancel_pipe, login_hint); let initialized = initialize( Some(cef_args.as_main_args()), Some(&settings), diff --git a/ee/wcp/cef-host/src/sysd.rs b/ee/wcp/cef-host/src/sysd.rs index df26a366..fd3d5db5 100644 --- a/ee/wcp/cef-host/src/sysd.rs +++ b/ee/wcp/cef-host/src/sysd.rs @@ -7,9 +7,9 @@ use std::collections::HashMap; use url::Url; use ak_ee_wcp_wire::TOKEN_QUERY_PARAM; -use ak_platform::generated::sys_auth::TokenAuthRequest; use ak_platform::generated::sys_auth::system_auth_interactive_client::SystemAuthInteractiveClient; use ak_platform::generated::sys_auth::system_auth_token_client::SystemAuthTokenClient; +use ak_platform::generated::sys_auth::{InteractiveAuthAsyncRequest, TokenAuthRequest}; use ak_platform::grpc::grpc_request; pub struct AuthStartAsync { @@ -21,10 +21,15 @@ pub struct TokenResponse { pub username: String, } -pub fn sys_auth_start_async() -> Result { +/// `login_hint` is the username of the tile that was selected, so the sign-in +/// page can skip the identification stage. It is only ever a hint: what the +/// flow authenticates as is whatever `sys_auth_url` reports back. +pub fn sys_auth_start_async(login_hint: Option) -> Result { let response = grpc_request(async |ch| { Ok(SystemAuthInteractiveClient::new(ch) - .interactive_auth_async(()) + .interactive_auth_async(InteractiveAuthAsyncRequest { + username: login_hint.clone(), + }) .await?) })? .into_inner(); diff --git a/ee/wcp/credprovider/src/credential.rs b/ee/wcp/credprovider/src/credential.rs index 2182524a..644913a1 100644 --- a/ee/wcp/credprovider/src/credential.rs +++ b/ee/wcp/credprovider/src/credential.rs @@ -442,6 +442,12 @@ impl IConnectableCredentialProviderCredential_Impl for Credential_Impl { self.qualified_username, self.is_local_user ); + // The tile names who is signing in, so the sign-in page is told up + // front rather than asking again. Only a hint: the flow still reports + // whoever actually authenticated, and `usernames_match` below is what + // decides whether that is this tile's user. + let login_hint = expected_username(&self.qualified_username, self.is_local_user); + let login_hint = (!login_hint.is_empty()).then(|| login_hint.to_string()); if let Some(q) = pqcws.as_ref() { unsafe { let _ = q.SetStatusMessage(w!("Please sign in to your authentik account...")); @@ -455,7 +461,10 @@ impl IConnectableCredentialProviderCredential_Impl for Credential_Impl { } }; - let result = self.deps.auth_flow.run(&mut should_continue); + let result = self + .deps + .auth_flow + .run(login_hint.as_deref(), &mut should_continue); let outcome = match result { AuthResult::Completed { username } => { @@ -493,16 +502,26 @@ impl IConnectableCredentialProviderCredential_Impl for Credential_Impl { } } +/// The username the browser flow is expected to authenticate as. The tile's +/// qualified name is `domain\username` for a local account, of which only the +/// username portion is what authentik knows the person by; a domain account +/// is already qualified the way it signs in. +/// +/// Both the hint sent into the flow and the check on the way back out come +/// from here, so a tile can never suggest one username and accept another. +fn expected_username(qualified: &str, is_local_user: bool) -> &str { + if is_local_user { + qualified.rsplit('\\').next().unwrap_or(qualified) + } else { + qualified + } +} + /// The browser flow authenticates against the qualified username shown on /// the tile; for local accounts that's `domain\username`, so compare only /// the username portion. fn usernames_match(authenticated: &str, qualified: &str, is_local_user: bool) -> bool { - let expected = if is_local_user { - qualified.rsplit('\\').next().unwrap_or(qualified) - } else { - qualified - }; - expected.eq_ignore_ascii_case(authenticated) + expected_username(qualified, is_local_user).eq_ignore_ascii_case(authenticated) } #[cfg(test)] @@ -523,11 +542,40 @@ mod tests { const SID: &str = "S-1-5-21-1-2-3-1001"; const AUTH_PACKAGE: u32 = 7; - struct FakeAuthFlow(AuthResult); + #[derive(Clone)] + struct FakeAuthFlow { + result: AuthResult, + /// The hint each `run` was handed, so tests can assert what the tile + /// offered the sign-in page. + hints: Arc>>>, + } + + impl FakeAuthFlow { + fn completed(username: &str) -> Self { + Self { + result: AuthResult::Completed { + username: username.to_string(), + }, + hints: Arc::new(Mutex::new(Vec::new())), + } + } + + fn hints(&self) -> Vec> { + self.hints.lock().unwrap_or_else(|e| e.into_inner()).clone() + } + } impl AuthFlow for FakeAuthFlow { - fn run(&self, _should_continue: &mut dyn FnMut() -> bool) -> AuthResult { - self.0.clone() + fn run( + &self, + login_hint: Option<&str>, + _should_continue: &mut dyn FnMut() -> bool, + ) -> AuthResult { + self.hints + .lock() + .unwrap_or_else(|e| e.into_inner()) + .push(login_hint.map(str::to_string)); + self.result.clone() } } @@ -629,25 +677,20 @@ mod tests { } } - fn credential( + fn credential_for( + qualified: &str, is_local_user: bool, + flow: &FakeAuthFlow, password: &FakePassword, store: &FakeStore, ) -> ICredentialProviderCredential { - let qualified = if is_local_user { - r"COMPUTER\alice".to_string() - } else { - "alice".to_string() - }; Credential::new( SID.to_string(), - qualified, + qualified.to_string(), is_local_user, CPUS_LOGON, CredentialDeps { - auth_flow: Box::new(FakeAuthFlow(AuthResult::Completed { - username: "alice".to_string(), - })), + auth_flow: Box::new(flow.clone()), password: Box::new(password.clone()), auth_package: Box::new(FakeAuthPackage), store: Box::new(store.clone()), @@ -656,13 +699,36 @@ mod tests { .into() } + fn credential( + is_local_user: bool, + password: &FakePassword, + store: &FakeStore, + ) -> ICredentialProviderCredential { + let qualified = if is_local_user { + r"COMPUTER\alice" + } else { + "alice" + }; + credential_for( + qualified, + is_local_user, + &FakeAuthFlow::completed("alice"), + password, + store, + ) + } + + fn connect(credential: &ICredentialProviderCredential) { + let connectable: IConnectableCredentialProviderCredential = credential.cast().unwrap(); + unsafe { connectable.Connect(None) }.unwrap(); + } + /// Drives `Connect` then `GetSerialization`. The returned buffer is the /// caller's to free. fn submit( credential: &ICredentialProviderCredential, ) -> Option { - let connectable: IConnectableCredentialProviderCredential = credential.cast().unwrap(); - unsafe { connectable.Connect(None) }.unwrap(); + connect(credential); let mut response = CREDENTIAL_PROVIDER_GET_SERIALIZATION_RESPONSE::default(); let mut serialization = CREDENTIAL_PROVIDER_CREDENTIAL_SERIALIZATION::default(); @@ -867,6 +933,58 @@ mod tests { assert!(password.state().changes.is_empty()); } + /// The hint has to be the bare username: `COMPUTER\alice` is a Windows + /// account name, and authentik would not recognise the person by it. + #[test] + fn a_local_tile_hints_the_username_without_the_computer_name() { + let flow = FakeAuthFlow::completed("alice"); + let cred = credential_for( + r"COMPUTER\alice", + true, + &flow, + &FakePassword::default(), + &FakeStore::default(), + ); + + connect(&cred); + + assert_eq!(flow.hints(), vec![Some("alice".to_string())]); + } + + #[test] + fn a_domain_tile_hints_the_qualified_username() { + let flow = FakeAuthFlow::completed("alice@example.com"); + let cred = credential_for( + "alice@example.com", + false, + &flow, + &FakePassword::default(), + &FakeStore::default(), + ); + + connect(&cred); + + assert_eq!(flow.hints(), vec![Some("alice@example.com".to_string())]); + } + + /// LogonUI can hand over a user with no qualified name at all; hinting an + /// empty username would leave the sign-in page prefilled with nothing. + #[test] + fn a_tile_with_no_username_sends_no_hint() { + let flow = FakeAuthFlow::completed(""); + let cred = credential_for( + "", + false, + &flow, + &FakePassword::default(), + &FakeStore::default(), + ); + + connect(&cred); + + assert_eq!(flow.hints(), vec![None]); + } + #[test] fn local_user_matches_on_username_portion_only() { assert!(usernames_match("alice", r"COMPUTER\alice", true)); diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 0d475265..70352240 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -32,11 +32,17 @@ use windows::{ use crate::syscalls::{self, ForegroundControl, acquire_interactive_token}; use ak_ee_wcp_wire::AuthResult; -/// Spawns `ak_cef.exe` and waits for its result. `should_continue` is polled -/// while waiting, so LogonUI cancelling (the user backing out of the tile) -/// tears the browser process down instead of orphaning it. +/// Spawns `ak_cef.exe` and waits for its result. `login_hint` is the username +/// of the selected tile, passed on so the sign-in page can skip asking who is +/// signing in. `should_continue` is polled while waiting, so LogonUI +/// cancelling (the user backing out of the tile) tears the browser process +/// down instead of orphaning it. pub trait AuthFlow { - fn run(&self, should_continue: &mut dyn FnMut() -> bool) -> AuthResult; + fn run( + &self, + login_hint: Option<&str>, + should_continue: &mut dyn FnMut() -> bool, + ) -> AuthResult; } pub struct CefAuthFlow { @@ -45,8 +51,12 @@ pub struct CefAuthFlow { } impl AuthFlow for CefAuthFlow { - fn run(&self, should_continue: &mut dyn FnMut() -> bool) -> AuthResult { - run_cef_host(&self.cef_exe, self.cpus, should_continue) + fn run( + &self, + login_hint: Option<&str>, + should_continue: &mut dyn FnMut() -> bool, + ) -> AuthResult { + run_cef_host(&self.cef_exe, self.cpus, login_hint, should_continue) } } @@ -124,6 +134,7 @@ fn create_duplex_pipes() -> windows::core::Result { fn run_cef_host( cef_exe: &Path, cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO, + login_hint: Option<&str>, should_continue: &mut dyn FnMut() -> bool, ) -> AuthResult { let pipes = match create_duplex_pipes() { @@ -136,7 +147,7 @@ fn run_cef_host( } }; - let spawn = spawn_cef_host(cef_exe, &pipes, cpus); + let spawn = spawn_cef_host(cef_exe, &pipes, cpus, login_hint); unsafe { let _ = CloseHandle(pipes.result_write_inheritable); let _ = CloseHandle(pipes.cancel_read_inheritable); @@ -352,17 +363,57 @@ fn signal_cancel(cancel_write: HANDLE) { std::mem::forget(f); } +/// Quotes one command-line argument so `ak_cef.exe` reads back exactly what +/// was passed. `CreateProcess*` takes a single string, and the child splits it +/// again with the `CommandLineToArgvW` rules `std::env::args` follows: a +/// backslash run is only special before a quote, where it has to be doubled, +/// and a value ending in one would otherwise escape its own closing quote. +/// Account names reach here straight from LogonUI, so they are quoted rather +/// than trusted to be free of spaces. +fn quote_arg(value: &str) -> String { + let mut quoted = String::with_capacity(value.len() + 2); + quoted.push('"'); + let mut backslashes = 0usize; + for c in value.chars() { + match c { + '\\' => { + backslashes += 1; + quoted.push(c); + } + // Double the run this quote follows, then escape the quote itself. + '"' => { + quoted.extend(std::iter::repeat_n('\\', backslashes + 1)); + backslashes = 0; + quoted.push(c); + } + _ => { + backslashes = 0; + quoted.push(c); + } + } + } + // Trailing run: doubled so it stays literal against the closing quote. + quoted.extend(std::iter::repeat_n('\\', backslashes)); + quoted.push('"'); + quoted +} + fn spawn_cef_host( cef_exe: &Path, pipes: &DuplexPipes, cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO, + login_hint: Option<&str>, ) -> windows::core::Result { - let cmdline = format!( - "\"{}\" --result-pipe {} --cancel-pipe {}", - cef_exe.display(), + let mut cmdline = format!( + "{} --result-pipe {} --cancel-pipe {}", + quote_arg(&cef_exe.display().to_string()), pipes.result_write_inheritable.0 as usize, pipes.cancel_read_inheritable.0 as usize ); + if let Some(login_hint) = login_hint { + cmdline.push_str(" --login-hint "); + cmdline.push_str("e_arg(login_hint)); + } let mut cmdline_wide: Vec = cmdline.encode_utf16().chain(std::iter::once(0)).collect(); let mut attr_size = 0usize; @@ -701,7 +752,7 @@ mod tests { std::env::var("COMSPEC").unwrap_or_else(|_| r"C:\Windows\System32\cmd.exe".to_string()), ); - let spawned = spawn_cef_host(&exe, &pipes, CPUS_CREDUI); + let spawned = spawn_cef_host(&exe, &pipes, CPUS_CREDUI, Some("alice")); unsafe { let _ = CloseHandle(pipes.result_read); @@ -720,6 +771,48 @@ mod tests { } } + /// Round-trips through the same parser `ak_cef.exe`'s `std::env::args` + /// uses, since the point of the quoting is what the child reads back — + /// asserting the escaped string only restates the implementation. + fn argv(cmdline: &str) -> Vec { + use windows::Win32::Foundation::{HLOCAL, LocalFree}; + use windows::Win32::UI::Shell::CommandLineToArgvW; + + let wide: Vec = cmdline.encode_utf16().chain(std::iter::once(0)).collect(); + let mut count = 0i32; + let argv = unsafe { CommandLineToArgvW(PCWSTR(wide.as_ptr()), &mut count) }; + assert!(!argv.is_null(), "CommandLineToArgvW rejected {cmdline:?}"); + + let args = (0..count as usize) + .map(|i| unsafe { (*argv.add(i)).to_string() }.expect("an argument in valid UTF-16")) + .collect(); + unsafe { LocalFree(Some(HLOCAL(argv as *mut c_void))) }; + args + } + + /// A username reaches this straight from LogonUI. A space would otherwise + /// split it into two arguments, and the trailing backslash of a domain + /// name would escape its own closing quote and swallow the rest. + #[test] + fn quoted_arguments_survive_the_childs_own_parser() { + for value in [ + "alice", + "alice smith", + r"CORP\alice", + r"trailing\\", + "quote\"inside", + r#"both\"mixed"#, + "", + ] { + let quoted = quote_arg(value); + assert_eq!( + argv(&format!("prog {quoted}")), + vec!["prog".to_string(), value.to_string()], + "{value:?} quoted as {quoted:?}" + ); + } + } + #[test] fn only_credui_may_fall_back_to_the_current_session() { assert!(may_launch_in_current_session(CPUS_CREDUI)); diff --git a/ee/wcp/e2e/src/mock_sysd.rs b/ee/wcp/e2e/src/mock_sysd.rs index fc98f89b..d8426e60 100644 --- a/ee/wcp/e2e/src/mock_sysd.rs +++ b/ee/wcp/e2e/src/mock_sysd.rs @@ -10,9 +10,12 @@ use ak_platform::generated::ping::{ capabilities_response::Capability, ping_server::{Ping, PingServer}, }; +use std::sync::{Arc, Mutex}; + use ak_platform::generated::sys_auth::{ - InteractiveAuthAsyncResponse, InteractiveAuthRequest, InteractiveChallenge, SshCertAuthRequest, - SshCertAuthResponse, TokenAuthRequest, TokenAuthResponse, + InteractiveAuthAsyncRequest, InteractiveAuthAsyncResponse, InteractiveAuthRequest, + InteractiveChallenge, SshCertAuthRequest, SshCertAuthResponse, TokenAuthRequest, + TokenAuthResponse, system_auth_interactive_server::{SystemAuthInteractive, SystemAuthInteractiveServer}, system_auth_token_server::{SystemAuthToken, SystemAuthTokenServer}, }; @@ -53,8 +56,25 @@ impl Ping for MockPing { } } +/// Every login hint `interactive_auth_async` was called with, so a test can +/// assert what the selected tile carried all the way through `ak_cef.exe`'s +/// command line into the gRPC call. +#[derive(Clone, Default)] +pub struct ObservedLoginHints(Arc>>>); + +impl ObservedLoginHints { + pub fn get(&self) -> Vec> { + self.0.lock().unwrap_or_else(|e| e.into_inner()).clone() + } + + fn record(&self, hint: Option) { + self.0.lock().unwrap_or_else(|e| e.into_inner()).push(hint); + } +} + struct MockSystemAuthInteractive { config: MockConfig, + login_hints: ObservedLoginHints, } #[tonic::async_trait] @@ -68,8 +88,9 @@ impl SystemAuthInteractive for MockSystemAuthInteractive { async fn interactive_auth_async( &self, - _request: Request<()>, + request: Request, ) -> Result, Status> { + self.login_hints.record(request.into_inner().username); Ok(Response::new(InteractiveAuthAsyncResponse { url: self.config.interactive_auth_url.clone(), header_token: self.config.header_token.clone(), @@ -111,6 +132,7 @@ impl SystemAuthToken for MockSystemAuthToken { /// of a test. pub struct MockSysd { task: tokio::task::JoinHandle<()>, + pub login_hints: ObservedLoginHints, } impl Drop for MockSysd { @@ -126,9 +148,11 @@ pub async fn start(config: MockConfig) -> eyre::Result { ) .await?; + let login_hints = ObservedLoginHints::default(); let ping = PingServer::new(MockPing); let interactive = SystemAuthInteractiveServer::new(MockSystemAuthInteractive { config: config.clone(), + login_hints: login_hints.clone(), }); let token = SystemAuthTokenServer::new(MockSystemAuthToken { config }); @@ -144,5 +168,5 @@ pub async fn start(config: MockConfig) -> eyre::Result { } }); - Ok(MockSysd { task }) + Ok(MockSysd { task, login_hints }) } diff --git a/ee/wcp/e2e/tests/sign_in_flow.rs b/ee/wcp/e2e/tests/sign_in_flow.rs index 379f4bf2..e8fe4424 100644 --- a/ee/wcp/e2e/tests/sign_in_flow.rs +++ b/ee/wcp/e2e/tests/sign_in_flow.rs @@ -27,7 +27,7 @@ const VALID_TOKEN: &str = "valid-token-under-test"; const USERNAME: &str = "e2e-user"; struct Fixture { - _mock: mock_sysd::MockSysd, + mock: mock_sysd::MockSysd, _caps: harness::DebugCapabilities, server: RedirectServer, provider: LoadedProvider, @@ -77,7 +77,7 @@ async fn setup(user: TestUser, server: RedirectServer) -> Fixture { } Fixture { - _mock: mock, + mock, _caps: caps, server, provider, @@ -178,6 +178,15 @@ async fn completed_sign_in_serializes_a_credential() { )); } + // The tile's username has to reach `ak-sysd` as the login hint: it goes + // out over `ak_cef.exe`'s command line, so nothing short of the real + // spawn shows whether it survived the trip. + assert_eq!( + fixture.mock.login_hints.get(), + vec![Some(USERNAME.to_string())], + "interactive_auth_async should have been called once, hinting the tile's user" + ); + // `cef-host` must put the interactive-auth header on every request it // makes, which is what let the (mock) backend hand back a sign-in page. let headers = fixture.server.observed_auth_headers(); diff --git a/protobuf/sys_auth.proto b/protobuf/sys_auth.proto index 502a40bd..fbf3aec4 100644 --- a/protobuf/sys_auth.proto +++ b/protobuf/sys_auth.proto @@ -18,7 +18,7 @@ service SystemAuthInteractive { // Interactive auth without a browser (for example, CLI) rpc InteractiveAuth(InteractiveAuthRequest) returns (InteractiveChallenge); // Interactive auth which is handed of to a browser - rpc InteractiveAuthAsync(google.protobuf.Empty) returns (InteractiveAuthAsyncResponse); + rpc InteractiveAuthAsync(InteractiveAuthAsyncRequest) returns (InteractiveAuthAsyncResponse); } service SystemAuthAuthorize { @@ -62,11 +62,6 @@ message InteractiveAuthRequest { } } -message InteractiveAuthAsyncResponse { - string url = 1; - string header_token = 2; -} - enum InteractiveAuthResult { PAM_SUCCESS = 0; PAM_PERM_DENIED = 6; @@ -96,6 +91,15 @@ message InteractiveChallenge { string component = 8; } +message InteractiveAuthAsyncRequest { + optional string username = 1; +} + +message InteractiveAuthAsyncResponse { + string url = 1; + string header_token = 2; +} + message SystemAuthorizeRequest { string session_id = 1; agent_auth.AuthorizeRequest authz = 2; From 1ccf9de7e4b7414124bf58012668d359257bbcf2 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 22:43:02 +0100 Subject: [PATCH 2/2] cleanup --- ee/wcp/cef-host/src/app.rs | 12 ++++++-- ee/wcp/cef-host/src/main.rs | 3 +- ee/wcp/cef-host/src/sysd.rs | 5 ++-- ee/wcp/credprovider/src/credential.rs | 28 +++++++---------- ee/wcp/credprovider/src/ipc.rs | 43 +++++++++++---------------- ee/wcp/e2e/src/mock_sysd.rs | 21 ++++--------- ee/wcp/e2e/tests/sign_in_flow.rs | 7 ++--- 7 files changed, 50 insertions(+), 69 deletions(-) diff --git a/ee/wcp/cef-host/src/app.rs b/ee/wcp/cef-host/src/app.rs index 78200695..408573ed 100644 --- a/ee/wcp/cef-host/src/app.rs +++ b/ee/wcp/cef-host/src/app.rs @@ -15,7 +15,11 @@ wrap_app! { impl App { fn browser_process_handler(&self) -> Option { - Some(HostBrowserProcessHandler::new(self.result_pipe, self.cancel_pipe, self.login_hint.clone())) + Some(HostBrowserProcessHandler::new( + self.result_pipe, + self.cancel_pipe, + self.login_hint.clone(), + )) } } } @@ -36,7 +40,11 @@ wrap_browser_process_handler! { // re-entrantly, which also keeps a blocking gRPC call out of // `CefInitialize`. fn on_context_initialized(&self) { - let mut task = OpenSignInWindow::new(self.result_pipe, self.cancel_pipe, self.login_hint.clone()); + let mut task = OpenSignInWindow::new( + self.result_pipe, + self.cancel_pipe, + self.login_hint.clone(), + ); post_task(ThreadId::UI, Some(&mut task)); } } diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index 3cdc30b4..821cb02d 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -143,8 +143,7 @@ fn main() { return; }; let cancel_pipe = arg_value("--cancel-pipe").and_then(|s| s.parse::().ok()); - // Absent whenever the credential provider had no username to offer; the - // sign-in page then asks for one as it always did. + // Absent when the credential provider had no username to offer. let login_hint = arg_value("--login-hint").filter(|hint| !hint.is_empty()); wipe_browser_state(Path::new(ROOT_CACHE_PATH)); diff --git a/ee/wcp/cef-host/src/sysd.rs b/ee/wcp/cef-host/src/sysd.rs index fd3d5db5..569b5dea 100644 --- a/ee/wcp/cef-host/src/sysd.rs +++ b/ee/wcp/cef-host/src/sysd.rs @@ -21,9 +21,8 @@ pub struct TokenResponse { pub username: String, } -/// `login_hint` is the username of the tile that was selected, so the sign-in -/// page can skip the identification stage. It is only ever a hint: what the -/// flow authenticates as is whatever `sys_auth_url` reports back. +/// `login_hint` is the selected tile's username, prefilled into the sign-in +/// page. Only a hint: who actually authenticated comes back via `sys_auth_url`. pub fn sys_auth_start_async(login_hint: Option) -> Result { let response = grpc_request(async |ch| { Ok(SystemAuthInteractiveClient::new(ch) diff --git a/ee/wcp/credprovider/src/credential.rs b/ee/wcp/credprovider/src/credential.rs index 644913a1..c2e455f4 100644 --- a/ee/wcp/credprovider/src/credential.rs +++ b/ee/wcp/credprovider/src/credential.rs @@ -442,10 +442,8 @@ impl IConnectableCredentialProviderCredential_Impl for Credential_Impl { self.qualified_username, self.is_local_user ); - // The tile names who is signing in, so the sign-in page is told up - // front rather than asking again. Only a hint: the flow still reports - // whoever actually authenticated, and `usernames_match` below is what - // decides whether that is this tile's user. + // Only a hint: `usernames_match` below is what decides whether whoever + // authenticated is this tile's user. let login_hint = expected_username(&self.qualified_username, self.is_local_user); let login_hint = (!login_hint.is_empty()).then(|| login_hint.to_string()); if let Some(q) = pqcws.as_ref() { @@ -502,13 +500,12 @@ impl IConnectableCredentialProviderCredential_Impl for Credential_Impl { } } -/// The username the browser flow is expected to authenticate as. The tile's -/// qualified name is `domain\username` for a local account, of which only the -/// username portion is what authentik knows the person by; a domain account -/// is already qualified the way it signs in. +/// The username the browser flow is expected to authenticate as: a local +/// account's tile is qualified `domain\username`, and only the username +/// portion is what authentik knows the person by. /// /// Both the hint sent into the flow and the check on the way back out come -/// from here, so a tile can never suggest one username and accept another. +/// from here, so a tile cannot suggest one username and accept another. fn expected_username(qualified: &str, is_local_user: bool) -> &str { if is_local_user { qualified.rsplit('\\').next().unwrap_or(qualified) @@ -517,9 +514,6 @@ fn expected_username(qualified: &str, is_local_user: bool) -> &str { } } -/// The browser flow authenticates against the qualified username shown on -/// the tile; for local accounts that's `domain\username`, so compare only -/// the username portion. fn usernames_match(authenticated: &str, qualified: &str, is_local_user: bool) -> bool { expected_username(qualified, is_local_user).eq_ignore_ascii_case(authenticated) } @@ -545,8 +539,7 @@ mod tests { #[derive(Clone)] struct FakeAuthFlow { result: AuthResult, - /// The hint each `run` was handed, so tests can assert what the tile - /// offered the sign-in page. + /// The hint each `run` was handed. hints: Arc>>>, } @@ -933,8 +926,8 @@ mod tests { assert!(password.state().changes.is_empty()); } - /// The hint has to be the bare username: `COMPUTER\alice` is a Windows - /// account name, and authentik would not recognise the person by it. + /// `COMPUTER\alice` is a Windows account name; authentik would not + /// recognise the person by it. #[test] fn a_local_tile_hints_the_username_without_the_computer_name() { let flow = FakeAuthFlow::completed("alice"); @@ -967,8 +960,7 @@ mod tests { assert_eq!(flow.hints(), vec![Some("alice@example.com".to_string())]); } - /// LogonUI can hand over a user with no qualified name at all; hinting an - /// empty username would leave the sign-in page prefilled with nothing. + /// LogonUI can hand over a user with no qualified name at all. #[test] fn a_tile_with_no_username_sends_no_hint() { let flow = FakeAuthFlow::completed(""); diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 70352240..107e7297 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -32,11 +32,10 @@ use windows::{ use crate::syscalls::{self, ForegroundControl, acquire_interactive_token}; use ak_ee_wcp_wire::AuthResult; -/// Spawns `ak_cef.exe` and waits for its result. `login_hint` is the username -/// of the selected tile, passed on so the sign-in page can skip asking who is -/// signing in. `should_continue` is polled while waiting, so LogonUI -/// cancelling (the user backing out of the tile) tears the browser process -/// down instead of orphaning it. +/// Spawns `ak_cef.exe` and waits for its result. `login_hint` is the selected +/// tile's username, prefilled into the sign-in page. `should_continue` is +/// polled while waiting, so LogonUI cancelling (the user backing out of the +/// tile) tears the browser process down instead of orphaning it. pub trait AuthFlow { fn run( &self, @@ -363,13 +362,10 @@ fn signal_cancel(cancel_write: HANDLE) { std::mem::forget(f); } -/// Quotes one command-line argument so `ak_cef.exe` reads back exactly what -/// was passed. `CreateProcess*` takes a single string, and the child splits it -/// again with the `CommandLineToArgvW` rules `std::env::args` follows: a -/// backslash run is only special before a quote, where it has to be doubled, -/// and a value ending in one would otherwise escape its own closing quote. -/// Account names reach here straight from LogonUI, so they are quoted rather -/// than trusted to be free of spaces. +/// Quotes one argument the way `CommandLineToArgvW` — and so the child's own +/// `std::env::args` — splits it back out. Account names come straight from +/// LogonUI, and a backslash run before a quote has to be doubled or it escapes +/// the quote instead. fn quote_arg(value: &str) -> String { let mut quoted = String::with_capacity(value.len() + 2); quoted.push('"'); @@ -380,7 +376,7 @@ fn quote_arg(value: &str) -> String { backslashes += 1; quoted.push(c); } - // Double the run this quote follows, then escape the quote itself. + // Double the run this quote follows, then escape the quote. '"' => { quoted.extend(std::iter::repeat_n('\\', backslashes + 1)); backslashes = 0; @@ -392,7 +388,7 @@ fn quote_arg(value: &str) -> String { } } } - // Trailing run: doubled so it stays literal against the closing quote. + // Trailing run: doubled, or it escapes the closing quote. quoted.extend(std::iter::repeat_n('\\', backslashes)); quoted.push('"'); quoted @@ -406,7 +402,7 @@ fn spawn_cef_host( ) -> windows::core::Result { let mut cmdline = format!( "{} --result-pipe {} --cancel-pipe {}", - quote_arg(&cef_exe.display().to_string()), + quote_arg(&cef_exe.to_string_lossy()), pipes.result_write_inheritable.0 as usize, pipes.cancel_read_inheritable.0 as usize ); @@ -771,28 +767,25 @@ mod tests { } } - /// Round-trips through the same parser `ak_cef.exe`'s `std::env::args` - /// uses, since the point of the quoting is what the child reads back — - /// asserting the escaped string only restates the implementation. + /// Splits a command line the way `ak_cef.exe`'s own `std::env::args` does. fn argv(cmdline: &str) -> Vec { use windows::Win32::Foundation::{HLOCAL, LocalFree}; use windows::Win32::UI::Shell::CommandLineToArgvW; let wide: Vec = cmdline.encode_utf16().chain(std::iter::once(0)).collect(); let mut count = 0i32; - let argv = unsafe { CommandLineToArgvW(PCWSTR(wide.as_ptr()), &mut count) }; - assert!(!argv.is_null(), "CommandLineToArgvW rejected {cmdline:?}"); + let parsed = unsafe { CommandLineToArgvW(PCWSTR(wide.as_ptr()), &mut count) }; + assert!(!parsed.is_null(), "CommandLineToArgvW rejected {cmdline:?}"); let args = (0..count as usize) - .map(|i| unsafe { (*argv.add(i)).to_string() }.expect("an argument in valid UTF-16")) + .map(|i| unsafe { (*parsed.add(i)).to_string() }.expect("an argument in valid UTF-16")) .collect(); - unsafe { LocalFree(Some(HLOCAL(argv as *mut c_void))) }; + let _ = unsafe { LocalFree(Some(HLOCAL(parsed as *mut c_void))) }; args } - /// A username reaches this straight from LogonUI. A space would otherwise - /// split it into two arguments, and the trailing backslash of a domain - /// name would escape its own closing quote and swallow the rest. + /// Asserted through the real parser rather than against the escaped + /// string: what matters is what the child reads back out. #[test] fn quoted_arguments_survive_the_childs_own_parser() { for value in [ diff --git a/ee/wcp/e2e/src/mock_sysd.rs b/ee/wcp/e2e/src/mock_sysd.rs index d8426e60..e0430740 100644 --- a/ee/wcp/e2e/src/mock_sysd.rs +++ b/ee/wcp/e2e/src/mock_sysd.rs @@ -57,20 +57,8 @@ impl Ping for MockPing { } /// Every login hint `interactive_auth_async` was called with, so a test can -/// assert what the selected tile carried all the way through `ak_cef.exe`'s -/// command line into the gRPC call. -#[derive(Clone, Default)] -pub struct ObservedLoginHints(Arc>>>); - -impl ObservedLoginHints { - pub fn get(&self) -> Vec> { - self.0.lock().unwrap_or_else(|e| e.into_inner()).clone() - } - - fn record(&self, hint: Option) { - self.0.lock().unwrap_or_else(|e| e.into_inner()).push(hint); - } -} +/// assert what the selected tile carried through `ak_cef.exe`'s command line. +pub type ObservedLoginHints = Arc>>>; struct MockSystemAuthInteractive { config: MockConfig, @@ -90,7 +78,10 @@ impl SystemAuthInteractive for MockSystemAuthInteractive { &self, request: Request, ) -> Result, Status> { - self.login_hints.record(request.into_inner().username); + self.login_hints + .lock() + .unwrap_or_else(|e| e.into_inner()) + .push(request.into_inner().username); Ok(Response::new(InteractiveAuthAsyncResponse { url: self.config.interactive_auth_url.clone(), header_token: self.config.header_token.clone(), diff --git a/ee/wcp/e2e/tests/sign_in_flow.rs b/ee/wcp/e2e/tests/sign_in_flow.rs index e8fe4424..95c81347 100644 --- a/ee/wcp/e2e/tests/sign_in_flow.rs +++ b/ee/wcp/e2e/tests/sign_in_flow.rs @@ -178,11 +178,10 @@ async fn completed_sign_in_serializes_a_credential() { )); } - // The tile's username has to reach `ak-sysd` as the login hint: it goes - // out over `ak_cef.exe`'s command line, so nothing short of the real - // spawn shows whether it survived the trip. + // The login hint travels over `ak_cef.exe`'s command line, so only the + // real spawn shows whether it survived the trip. assert_eq!( - fixture.mock.login_hints.get(), + *fixture.mock.login_hints.lock().unwrap(), vec![Some(USERNAME.to_string())], "interactive_auth_async should have been called once, hinting the tile's user" );