Minor broker refactoring and cleanup #349
Conversation
There was a problem hiding this comment.
Makes sense on moving the provider specific to non provider for now.
One question about firstSelectedMode. The idea was IIRC:
- you have an authentication, you select one auth method
- then, broker decides it’s MFA (which is not the current QR code method), and either ask for another auth method or is password definition/change (which on the local password definition is supported)
-> The goal was that next time you select the same user, the first auth method (if available) is auto-selected. Does this still work or does it work because by chance, we don’t set in the last local password reset the second time you log in?
AFAICT there is no functionality implemented for that. The PAM module currently autoselects the first element in the list of authentication modes returned by the broker in the |
adombeck
force-pushed
the
broker-refactorings
branch
from
3897095 to
ac0a46a
Compare
internal/broker/broker.go
Outdated
| @@ -277,6 +273,33 @@ func (b *Broker) GetAuthenticationModes(sessionID string, supportedUILayouts []m | |||
| return authModes, nil | |||
| } | |||
|
|
|||
| func (b *Broker) availableAuthModes(session session, tokenExists bool, endpoints map[string]struct{}) (availableModes []string, err error) { | |||
There was a problem hiding this comment.
IIRC, the purpose of this being considered provider-specific was to allow them more control over what is or isn't supported (i.e. EntraID can have modes that Google doesn't and so on...). It also makes sense to have it work as:
"general broker API: ok, this is what I have and what I can do, how will we do this?
provider API: Ok, so let's go like this..."
I know this is not something we interact with currently (and it results in a horrific function signature), but is it worth refactoring this now to maybe have to redo this later? We know that authd can handle TOTPs, codes and so on, so it could be something that we allow in the future...
There was a problem hiding this comment.
IMO yes, it is worth simplifying the interface until we actually need it to be more complex. If I understand correcelty, Didier also agreed with that above.
There was a problem hiding this comment.
I had handled this a bit differently some weeks ago in 789c0b0
We can consider still that so that we don't have code duplication but providers can easily re-implement it if required.
There was a problem hiding this comment.
I would still prefer the simpler interface, but using struct embedding to have implementations for the general case, which can be overridden by the provider implementations makes sense to me once we do have implementations which override those - and I could live with already using that now, even though it's unused. WDYT, @didrocks?
There was a problem hiding this comment.
using struct embedding to have implementations for the general case, which can be overridden by the provider implementations makes sense to me
I propose we rename NoProvider to GenericProvider or something similar if we do that.
There was a problem hiding this comment.
This is the only part which makes sense to be provider-specific IMO: Which authentication modes the provider supports for login, i.e. device_auth, device_auth_qr, or any other OIDC modes we might support later.
So if we want to keep this part provider-specific, then I think we should rename the method to something like SupportedOIDCAuthModes and only return the list of OIDC auth modes which are supported by that provider:
func (p GenericProvider) SupportedOIDCAuthModes() ([]string) {
return []string{authmodes.Device, authmodes.DeviceQr}
}and move all the logic to the provider-agnostic broker package. What do you think?
There was a problem hiding this comment.
As discussed during HO, we agree that the local password capabilities (defining and changing it) should be moved to the general implementation (i.e. the broker package) whilst the providers should control the available OIDC modes. Any thoughts, @didrocks @3v1n0?
So if we want to keep this part provider-specific, then I think we should rename the method to something like SupportedOIDCAuthModes and only return the list of OIDC auth modes which are supported by that provider
This proposal seems good to me, but I'd still like us to evaluate on the provider side whether or not an OIDC auth mode is feasible. Our current way of saying that a user can not authenticate is by having no available authentication modes, so the providers should still have some form of control over this, IMO (and it also takes some weight off the broker package).
There was a problem hiding this comment.
As discussed during HO, we agree that the local password capabilities (defining and changing it) should be moved to the general implementation (i.e. the broker package) whilst the providers should control the available OIDC modes. Any thoughts, @didrocks @3v1n0?
Yes, on that one, I think it makes sense to have that defined in a non per-provider way as it’s a single repo and it’s a common feature.
There was a problem hiding this comment.
As discussed during HO, we agree that the local password capabilities (defining and changing it) should be moved to the general implementation (i.e. the broker package) whilst the providers should control the available OIDC modes.
I pushed a commit which implements that.
This proposal seems good to me, but I'd still like us to evaluate on the provider side whether or not an OIDC auth mode is feasible. Our current way of saying that a user can not authenticate is by having no available authentication modes, so the providers should still have some form of control over this, IMO (and it also takes some weight off the broker package).
On which basis should the provider decide whether a mode is available or not? In which case would a provider say that the user can't authenticate?
There was a problem hiding this comment.
On which basis should the provider decide whether a mode is available or not?
Like we used to handle it already, no? If a given endpoint is configured, but isn't reachable and so on...
adombeck
force-pushed
the
broker-refactorings
branch
from
ac0a46a to
63e93ee
Compare
The firstSelectedMode was set but never used.
…CAuthModes All the decisions related to the local password do not belong in provider-specific code, so they were moved to the provider-agnostic brokers package. The provider implementation now only defines which OIDC authentication modes it supports.
adombeck
force-pushed
the
broker-refactorings
branch
from
63e93ee to
5cf4830
Compare
We used the auth modes supported by the UI to define the list of auth modes supported by provider endpoints, which was confusing. Also, print a log message when the UI doesn't support an authentication mode which is supported by the provider, instead of returning an error, because that's not an error if there are other modes available.
adombeck
force-pushed
the
broker-refactorings
branch
from
5cf4830 to
353b666
Compare
Remove an unused field and move a function that's not provider-specific from the provider interface to the broker. See commit messages for details.