From 1ee9278786491c46be6526a5100618ea44e40f0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 15 Feb 2021 20:59:57 +0100 Subject: [PATCH 1/6] gdm: Don't try to retry authenticating when the service is unavailable In the case a service is not available (as it can be in the fingereprint case when a supported reader is available but has not enrolled prints) we were trying indefinitely to restart it, however this can lead to troubles since commit 7a2e629b as when the service conversation was stopped we had no way to figure out this case and we'd end up to eventually fail the whole authentication. However, in such cases the PAM services are expected to return a PAM_AUTHINFO_UNAVAIL and gdm to handle it, emitting service-unavailable signal. So connect to ::service-unavailable and keep track of the unavailable services so that we can avoid retrying with them. In case such service is not the foreground one, we can just silently ignore the error as we did before commit 7a2e629b, without bothering failing the whole verification. In case we got a valid error message on service-unavailable, we also show it, this is normally not happening unless GDM isn't redirecting here other kind of problems (such as MAXTRIES) which are supposed to stop the authentication stopping any further retry. Closes: https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/3734 Part-of: --- js/gdm/util.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/js/gdm/util.js b/js/gdm/util.js index a91badf4fe..ddd03c5f0d 100644 --- a/js/gdm/util.js +++ b/js/gdm/util.js @@ -175,6 +175,7 @@ var ShellUserVerifier = class { this.reauthenticating = false; this._failCounter = 0; + this._unavailableServices = new Set(); this._credentialManagers = {}; this._credentialManagers[OVirt.SERVICE_NAME] = OVirt.getOVirtCredentialsManager(); @@ -452,6 +453,8 @@ var ShellUserVerifier = class { this._signalIds.push(id); id = this._userVerifier.connect('conversation-stopped', this._onConversationStopped.bind(this)); this._signalIds.push(id); + id = this._userVerifier.connect('service-unavailable', this._onServiceUnavailable.bind(this)); + this._signalIds.push(id); id = this._userVerifier.connect('reset', this._onReset.bind(this)); this._signalIds.push(id); id = this._userVerifier.connect('verification-complete', this._onVerificationComplete.bind(this)); @@ -598,6 +601,7 @@ var ShellUserVerifier = class { _onReset() { // Clear previous attempts to authenticate this._failCounter = 0; + this._unavailableServices.clear(); this._updateDefaultService(); this.emit('reset'); @@ -661,6 +665,16 @@ var ShellUserVerifier = class { this.emit('verification-failed', serviceName, canRetry); } + _onServiceUnavailable(_client, serviceName, errorMessage) { + this._unavailableServices.add(serviceName); + + if (!errorMessage) + return; + + if (this.serviceIsForeground(serviceName) || this.serviceIsFingerprint(serviceName)) + this._queueMessage(serviceName, errorMessage, MessageType.ERROR); + } + _onConversationStopped(client, serviceName) { // If the login failed with the preauthenticated oVirt credentials // then discard the credentials and revert to default authentication @@ -674,6 +688,9 @@ var ShellUserVerifier = class { return; } + if (this._unavailableServices.has(serviceName)) + return; + // if the password service fails, then cancel everything. // But if, e.g., fingerprint fails, still give // password authentication a chance to succeed -- GitLab From 9ecc1a4cd7f15e84898f6649125080d24a2a5e11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 16 Feb 2021 04:18:18 +0100 Subject: [PATCH 2/6] gdm: Compress fingerprint failures events using a timeout When a fingerprint failure event happens we may also soon receive a conversation-stopped event with an error message (such as in the case we hit the MAXRETRIES value), but this is going to be ignored in case we are too quick in consider the first failure a verification-failed event because that implies disconnecting from all the events and then ignoring such signals. To prevent this, add a small timeout before failing the verification so that if we get a further event we will process it. Part-of: --- js/gdm/util.js | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/js/gdm/util.js b/js/gdm/util.js index ddd03c5f0d..369c0bb331 100644 --- a/js/gdm/util.js +++ b/js/gdm/util.js @@ -46,6 +46,7 @@ var DISABLE_USER_LIST_KEY = 'disable-user-list'; // Give user 48ms to read each character of a PAM message var USER_READ_TIME = 48; +const FINGERPRINT_ERROR_TIMEOUT_WAIT = 15; var MessageType = { NONE: 0, @@ -568,10 +569,30 @@ var ShellUserVerifier = class { this._queueMessage(serviceName, problem, MessageType.ERROR); if (isFingerprint) { + // pam_fprintd allows the user to retry multiple (maybe even infinite! + // times before failing the authentication conversation. + // We don't want this behavior to bypass the max-tries setting the user has set, + // so we count the problem messages to know how many times the user has failed. + // Once we hit the max number of failures we allow, it's time to failure the + // conversation from our side. We can't do that right away, however, because + // we may drop pending messages coming from pam_fprintd. In order to make sure + // the user sees everything, we queue the failure up to get handled in the + // near future, after we've finished up the current round of messages. this._failCounter++; - if (!this._canRetry()) - this._verificationFailed(serviceName, false); + if (!this._canRetry()) { + if (this._fingerprintFailedId) + GLib.source_remove(this._fingerprintFailedId); + + const cancellable = this._cancellable; + this._fingerprintFailedId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, + FINGERPRINT_ERROR_TIMEOUT_WAIT, () => { + this._fingerprintFailedId = 0; + if (!cancellable.is_cancelled()) + this._verificationFailed(serviceName, false); + return GLib.SOURCE_REMOVE; + }); + } } } -- GitLab From ef10bb6229ceb323822f27a436c5f3ce9ba12696 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 16 Feb 2021 02:00:41 +0100 Subject: [PATCH 3/6] gdm: Keep messages in queue until we've not fully processed them It can be convenient to get the currently showing message in order to replace or remove it in case it's not needed anymore. So simplify the message queue handling by only depending on a single local variable (_messageQueue) and redefining hasPendingMessages depending on its content. Now messages are kept in queue till they are not fully processed and the first message is always the one currently shown. Part-of: --- js/gdm/util.js | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/js/gdm/util.js b/js/gdm/util.js index 369c0bb331..16689f10e6 100644 --- a/js/gdm/util.js +++ b/js/gdm/util.js @@ -172,7 +172,6 @@ var ShellUserVerifier = class { this._messageQueue = []; this._messageQueueTimeoutId = 0; - this.hasPendingMessages = false; this.reauthenticating = false; this._failCounter = 0; @@ -194,10 +193,18 @@ var ShellUserVerifier = class { } } + get hasPendingMessages() { + return !!this._messageQueue.length; + } + get allowedFailures() { return this._settings.get_int(ALLOWED_FAILURES_KEY); } + get currentMessage() { + return this._messageQueue ? this._messageQueue[0] : null; + } + begin(userName, hold) { this._cancellable = new Gio.Cancellable(); this._hold = hold; @@ -283,7 +290,6 @@ var ShellUserVerifier = class { this._messageQueue = []; - this.hasPendingMessages = false; this.emit('no-more-messages'); } @@ -293,33 +299,33 @@ var ShellUserVerifier = class { } _queueMessageTimeout() { - if (this._messageQueue.length == 0) { - this.finishMessageQueue(); - return; - } - if (this._messageQueueTimeoutId != 0) return; - let message = this._messageQueue.shift(); + const message = this.currentMessage; delete this._currentMessageExtraInterval; this.emit('show-message', message.serviceName, message.text, message.type); this._messageQueueTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, - message.interval + (this._currentMessageExtraInterval | 0), - () => { - this._messageQueueTimeoutId = 0; - this._queueMessageTimeout(); - return GLib.SOURCE_REMOVE; - }); + message.interval + (this._currentMessageExtraInterval | 0), () => { + this._messageQueueTimeoutId = 0; + + if (this._messageQueue.length > 1) { + this._messageQueue.shift(); + this._queueMessageTimeout(); + } else { + this.finishMessageQueue(); + } + + return GLib.SOURCE_REMOVE; + }); GLib.Source.set_name_by_id(this._messageQueueTimeoutId, '[gnome-shell] this._queueMessageTimeout'); } _queueMessage(serviceName, message, messageType) { let interval = this._getIntervalForMessage(message); - this.hasPendingMessages = true; this._messageQueue.push({ serviceName, text: message, type: messageType, interval }); this._queueMessageTimeout(); } -- GitLab From 1cc20ca6b67d1b211d5c3ec78e6981a9ad7adee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 16 Feb 2021 03:08:38 +0100 Subject: [PATCH 4/6] gdm: Add ability to queue a message overriding ones with less priority There are cases in which a service may want to override a message with one coming with higher priority, for example an info or hint message isn't useful anymore if we've already got an error message. In the same way when a service has been stopped we don't care anymore showing its info or hint messages, while it still may be relevant to show errors. An example is the fingerprint service that may emit errors quickly while the hints messages should not be kept around when an error is already queued or when the service has been stopped. So, add function that allows to override queued messages based by their type that follows this policy: - Messages coming from different services are always preserved in their original order. - Messages (from the same service) with a priority equal or higher than the last queued one are preserved. - Messages matching completely the last queued are dropped in favor of this one. Part-of: --- js/gdm/util.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/js/gdm/util.js b/js/gdm/util.js index 16689f10e6..67255bc9e2 100644 --- a/js/gdm/util.js +++ b/js/gdm/util.js @@ -49,10 +49,11 @@ var USER_READ_TIME = 48; const FINGERPRINT_ERROR_TIMEOUT_WAIT = 15; var MessageType = { + // Keep messages in order by priority NONE: 0, - ERROR: 1, + HINT: 1, INFO: 2, - HINT: 3, + ERROR: 3, }; const FingerprintReaderType = { @@ -330,6 +331,20 @@ var ShellUserVerifier = class { this._queueMessageTimeout(); } + _queuePriorityMessage(serviceName, message, messageType) { + const newQueue = this._messageQueue.filter(m => { + if (m.serviceName !== serviceName || m.type >= messageType) + return m.text !== message; + return false; + }); + + if (!newQueue.includes(this.currentMessage)) + this._clearMessageQueue(); + + this._messageQueue = newQueue; + this._queueMessage(serviceName, message, messageType); + } + _clearMessageQueue() { this.finishMessageQueue(); -- GitLab From 45a5171a95c7337a541c765306b5e5fd7f790de3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 16 Feb 2021 03:45:14 +0100 Subject: [PATCH 5/6] gdm: Filter service non-error messages on verification stopped or failed Once the verification has been stopped or has failed all the messages that are not of error type are just not needed or wrong to show. For example, in the fingerprint case we may still show the hint to swipe or touch the device, while the fingerprint PAM service has already been stopped. So filter them by adding a new function that adds a null message to the queue, overriding all the messages that have a lower priority. Part-of: --- js/gdm/util.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/js/gdm/util.js b/js/gdm/util.js index 67255bc9e2..f1c3c91497 100644 --- a/js/gdm/util.js +++ b/js/gdm/util.js @@ -281,6 +281,9 @@ var ShellUserVerifier = class { } _getIntervalForMessage(message) { + if (!message) + return 0; + // We probably could be smarter here return message.length * USER_READ_TIME; } @@ -299,6 +302,18 @@ var ShellUserVerifier = class { this._currentMessageExtraInterval = interval; } + _serviceHasPendingMessages(serviceName) { + return this._messageQueue.some(m => m.serviceName === serviceName); + } + + _filterServiceMessages(serviceName, messageType) { + // This function allows to remove queued messages for the @serviceName + // whose type has lower priority than @messageType, replacing them + // with a null message that will lead to clearing the prompt once done. + if (this._serviceHasPendingMessages(serviceName)) + this._queuePriorityMessage(serviceName, null, messageType); + } + _queueMessageTimeout() { if (this._messageQueueTimeoutId != 0) return; @@ -678,6 +693,7 @@ var ShellUserVerifier = class { const canRetry = retry && this._canRetry(); this._disconnectSignals(); + this._filterServiceMessages(serviceName, MessageType.ERROR); if (canRetry) { if (!this.hasPendingMessages) { @@ -730,6 +746,8 @@ var ShellUserVerifier = class { return; } + this._filterServiceMessages(serviceName, MessageType.ERROR); + if (this._unavailableServices.has(serviceName)) return; -- GitLab From b74900b3a3dd7cd6484255e2ef209e3a65507294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 16 Feb 2021 03:56:09 +0100 Subject: [PATCH 6/6] gdm: Override any other lower-priority service message on error When we got an error, all the other HINT or INFO messages are not useful anymore and delaying to show them is just a waste of time and may be even wrong in scenarios with fast authentication devices. An example are the fingerprint devices, where the user may touch the sensor repeatedly while we may still show the "touch the sensor" hint instead of notifying of possible errors. So, in case we got an error override all the other errors coming from the same service with lower priority. Part-of: --- js/gdm/util.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/js/gdm/util.js b/js/gdm/util.js index f1c3c91497..1ee84acde2 100644 --- a/js/gdm/util.js +++ b/js/gdm/util.js @@ -603,7 +603,8 @@ var ShellUserVerifier = class { if (!this.serviceIsForeground(serviceName) && !isFingerprint) return; - this._queueMessage(serviceName, problem, MessageType.ERROR); + this._queuePriorityMessage(serviceName, problem, MessageType.ERROR); + if (isFingerprint) { // pam_fprintd allows the user to retry multiple (maybe even infinite! // times before failing the authentication conversation. -- GitLab