pam/native: Do not support device authentication mode in polkit #768
Conversation
Polkit UI is quite limited so, although the device authentication works as expected, it's far from being user friendly so disable the "qrcode" UI at all in such case. Ultimately it should be up to the broker to decide whether to support an UI or not, but since we can't provide it the service name, we have to handle it in the module.
…modes We did tested the cases in which many authentication modes were available but we never tested the case in which only one or two are, so here it is!
Polkit UI is something that is shown when the user is already logged in so we assume that's under the authd control, thus it's not needed to switch to it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #768 +/- ##
==========================================
- Coverage 83.43% 83.26% -0.18%
==========================================
Files 83 96 +13
Lines 8689 9651 +962
Branches 74 74
==========================================
+ Hits 7250 8036 +786
- Misses 1111 1236 +125
- Partials 328 379 +51 ☔ View full report in Codecov by Sentry. |
| @@ -90,6 +90,7 @@ func (m *nativeModel) Init() tea.Cmd { | |||
|
|
|||
| m.interactive = isSSHSession(m.pamMTx) || IsTerminalTTY(m.pamMTx) | |||
| rendersQrCode := m.isQrcodeRenderingSupported() | |||
| hasQrCode := m.serviceName != polkitServiceName | |||
There was a problem hiding this comment.
| hasQrCode := m.serviceName != polkitServiceName | |
| supportsQRCode := m.serviceName != polkitServiceName |
There was a problem hiding this comment.
Actually, I think this should be named supportsDeviceAuth (and rendersQrCode should then be named supportsQRCode for consistency). The Type: layouts.QrCode is confusing, because it doesn't actually specify a layout used for QR code authentication but for device authentication in general.
There was a problem hiding this comment.
Indeed, but again I think that's for another reafactor PR... I definitely think that we should change the qr-code story as something else (not even device, since it is just a web-based authentication that can be done via another device or in the very same machine once we've a web-UI rendering it).
I had commented it already internally, I'll ping you about it.
There was a problem hiding this comment.
Indeed, but again I think that's for another reafactor PR.
You're adding this new variable here, why should we rename it in a separate PR?
| if _, ok := exampleUsers[username]; !ok && strings.HasPrefix(username, "user-auth-modes-") { | ||
| r := regexp.MustCompile(`user-auth-modes-([\w-,]+)-integration`) | ||
| if matches := r.FindStringSubmatch(username); len(matches) > 1 { | ||
| exampleUsers[username] = userInfoBroker{Password: "goodpass"} | ||
| info.acceptedAuthModesIDs = strings.Split(matches[1], ",") | ||
| log.Debugf(context.Background(), "%q accepts authentication mode IDs: %v", | ||
| username, info.acceptedAuthModesIDs) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not a fan of encoding behavior of the broker via literal strings in the username, because it makes it hard to navigate from the test case to the code it affects. I would prefer to extract string constants which we use both in the test case and here. We can do that in a separate PR though.
There was a problem hiding this comment.
Yeah, I have a commit doing this already... It's part of #593 but it's waiting for review for a while, so if that lands before I can rebase it to it
Polkit UI is quite limited so, although the device authentication works
as expected, it's far from being user friendly so disable the "qrcode"
UI at all in such case.
Ultimately it should be up to the broker to decide whether to support an
UI or not, but since we can't provide it the service name, we have to
handle it in the module.
At the same time, ignore the local broker when using polkit service.
That said this avoids polkit to show the device authentication as it's something we already mentioned we don't want, but we can't by design do this kind of filtering in the PAM module, that should be thinnest layer between authd and the broker, but instead I think we should re-consider refactoring this so that:
UDENG-5154