[Web] Add "set as default" feature to Auth Connectors in the Web UI #50751
Conversation
| } | ||
|
|
||
| // CheckAndSetDefaults checks if the provided values are valid. | ||
| func (r *SetDefaultAuthConnectorRequest) CheckAndSetDefaults() error { |
There was a problem hiding this comment.
this seems a bit unwieldy. could. you do something like having a map or slice with all the valid types and then this method can just check if r.Type exists in the valid type struct. something like this
// ValidConnectorTypes defines the allowed authentication connector types
var ValidConnectorTypes = []string{
constants.SAML,
constants.OIDC,
constants.Github,
constants.LocalConnector,
}
// and then this in the method
if !slices.Contains(ValidConnectorTypes, r.Type) {
return trace.BadParameter("unsupported connector type: %q", r.Type)
}
| @@ -954,6 +954,9 @@ func (h *Handler) bindDefaultEndpoints() { | |||
| h.PUT("/webapi/github/:name", h.WithAuth(h.updateGithubConnectorHandle)) | |||
| h.DELETE("/webapi/github/:name", h.WithAuth(h.deleteGithubConnector)) | |||
|
|
|||
| // Sets the default connector in the auth preference. | |||
| h.PUT("/webapi/defaultconnector", h.WithAuth(h.setDefaultConnectorHandle)) | |||
There was a problem hiding this comment.
what do you think of adding connector to a dedicated path now that we have a chance
h.PUT("/webapi/authconnector/default", h.WithAuth(h.setDefaultConnectorHandle)).
Such separation will save us from issues that you have in your another PR https://github.com/gravitational/teleport/pull/50749/files#diff-5a12dd7747d28af8fe5043638cc11f31a3314b2a23f3e23fe0bcdc952a805fa6R951-R953
| @@ -1766,6 +1769,19 @@ func (h *Handler) getWebConfig(w http.ResponseWriter, r *http.Request, p httprou | |||
|
|
|||
| if authType == constants.Local { | |||
| localConnectorName = cap.GetConnectorName() | |||
| } else { | |||
| // Move the default connector to the top of the list so that it shows up first in the UI | |||
There was a problem hiding this comment.
This looks risky. Can't we add a new "defaultConnector" field and send it in response?
Also this will break if UI wants to apply custom sorting logic?
| if err := httplib.ReadJSON(r, &req); err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| if err := req.CheckAndSetDefaults(); err != nil { |
There was a problem hiding this comment.
UpsertAuthPreference does its own connector type validation https://github.com/gravitational/teleport/blob/master/api/types/authentication.go#L701C2-L708C3 among others. Since we aren't doing anything extra op on the data received in ui.SetDefaultAuthConnectorRequest and only use it to construct authPref connecter name and type, my suggestion is to remove this CheckAndSetDefaults from here as well as its declaration.
Otherwise it gets confusing on validation responsibilities or you will end up duplicating validation logic between AuthPreferenceV2.CheckAndSetDefaults and ui.SetDefaultAuthConnectorRequest.CheckAndSetDefaults. Or worse, update one CheckAndSetDefaults and forget to update another.
| <MenuItem onClick={onClickDelete}>Delete</MenuItem> | ||
| </MenuIcon> | ||
| )} | ||
| {!isPlaceholder && |
There was a problem hiding this comment.
Add test for this whole block to ensure the items/buttons appear as expected? Such logic that deals with multiple states are prone to bug/regression.
| authPref.SetConnectorName(defaultConnectorName) | ||
| authPref.SetType(defaultConnectorType) | ||
|
|
||
| _, err = clt.UpsertAuthPreference(r.Context(), authPref) |
There was a problem hiding this comment.
Add unit test for this logic to deduce and update auth pref with default connector?
| defaultConnectorName = connectors[len(connectors)-1].Name | ||
| defaultConnectorType = connectors[len(connectors)-1].Kind |
There was a problem hiding this comment.
for the oss, won't there always be one Github auth connector?
Purpose
Part of https://github.com/gravitational/teleport.e/issues/4989
ecounterpart: https://github.com/gravitational/teleport.e/pull/5791This PR is based on changes from:
This PR adds the ability to set auth connectors as the default auth preference in the Web UI. If the default connector configured in the auth preferences references a connector that doesn't exist, it will fall back to whichever item is last in the list. When deleting a default auth connector, the confirm dialog will warn the user and inform them of this.
Previously, if there were multiple auth connectors, it would make no difference in the Web UI login page which one was set as default since they weren't ordered in any particular way. However in this PR, I decided to improve the UX for this and ensure that the default connector will always show up at the top of the list. This addition wasn't discussed prior, so feel free to chime in.
Demo
newssoflow-compressed.mov
Changelog: Add the ability to an auth connector as the default authentication method in the Web UI