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
| @@ -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.
I propose we rename NoProvider to GenericProvider or something similar if we do that.
I’m more in favour of this yes, and that’s on my mind for quite a while!
There was a problem hiding this comment.
I’m more in favour of this yes
Does that refer only to renaming the NoProvider to GenericProvider or also to "use an embedded struct which implements the CurrentAuthenticationModesOffered now, instead of simplifying the interface"?
There was a problem hiding this comment.
Does that refer only to renaming the NoProvider to GenericProvider or also to "use an embedded struct which implements the CurrentAuthenticationModesOffered now, instead of simplifying the interface"?
To both IMHO, we rename + embed it. I’m not close enough to the code to know about the CurrentAuthenticationModesOffered, but I’m happy to jump on a call if you want to explain this to me.
The firstSelectedMode was set but never used.
The decision which authentication modes are offered is (currently) not provider-specific. Lets make the interface simpler until we actually have a need to make it provider-specific.
adombeck
force-pushed
the
broker-refactorings
branch
from
ac0a46a to
63e93ee
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.