Skip to content

Commit

Permalink
fix: omit irrelevant OIDC providers in forced refresh login flows (#3608
Browse files Browse the repository at this point in the history
)

Whenever an user is asked to reauthenticate (e.g. because they wish to execute settings flow touching their credentials and their session is no longer privileged) they are asked to provide their credentials again. The forced-refresh login flow generated for such cases already excludes some strategies that are enabled in Kratos but cannot be used to authenticate as current identity, and for example the form presented to the user will not have a password field if the identity does not have a password credential.

This, however, does not currently apply to OIDC providers; the user will always see the full set even if some of them can't be used to sign in as current identity. This change causes forced refresh login flows to also omit irrelevant OIDC providers in generated form in order to avoid confunding the user about which strategies/providers are valid and can actually be used to reauthenticate.
  • Loading branch information
Saancreed authored Nov 10, 2023
1 parent 843a215 commit 912dccd
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 6 deletions.
34 changes: 32 additions & 2 deletions selfservice/strategy/oidc/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"golang.org/x/oauth2"

"github.com/ory/kratos/cipher"
"github.com/ory/kratos/selfservice/flowhelpers"
"github.com/ory/kratos/selfservice/sessiontokenexchange"
"github.com/ory/x/jsonnetsecure"
"github.com/ory/x/otelx"
Expand Down Expand Up @@ -484,15 +485,44 @@ func (s *Strategy) ExchangeCode(ctx context.Context, provider Provider, code str
return token, err
}

func (s *Strategy) populateMethod(r *http.Request, c *container.Container, message func(provider string) *text.Message) error {
func (s *Strategy) populateMethod(r *http.Request, f flow.Flow, message func(provider string) *text.Message) error {
conf, err := s.Config(r.Context())
if err != nil {
return err
}

providers := conf.Providers

if lf, ok := f.(*login.Flow); ok && lf.IsForced() {
if _, id, c := flowhelpers.GuessForcedLoginIdentifier(r, s.d, lf, s.ID()); id != nil {
if c == nil {
// no OIDC credentials, don't add any providers
providers = nil
} else {
var credentials identity.CredentialsOIDC
if err := json.Unmarshal(c.Config, &credentials); err != nil {
// failed to read OIDC credentials, don't add any providers
providers = nil
} else {
// add only providers that can actually be used to log in as this identity
providers = make([]Configuration, 0, len(conf.Providers))
for i := range conf.Providers {
for j := range credentials.Providers {
if conf.Providers[i].ID == credentials.Providers[j].Provider {
providers = append(providers, conf.Providers[i])
break
}
}
}
}
}
}
}

// does not need sorting because there is only one field
c := f.GetUI()
c.SetCSRF(s.d.GenerateCSRFToken(r))
AddProviders(c, conf.Providers, message)
AddProviders(c, providers, message)

return nil
}
Expand Down
2 changes: 1 addition & 1 deletion selfservice/strategy/oidc/strategy_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (s *Strategy) PopulateLoginMethod(r *http.Request, requestedAAL identity.Au
return nil
}

return s.populateMethod(r, l.UI, text.NewInfoLoginWith)
return s.populateMethod(r, l, text.NewInfoLoginWith)
}

// Update Login Flow with OpenID Connect Method
Expand Down
2 changes: 1 addition & 1 deletion selfservice/strategy/oidc/strategy_registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (s *Strategy) RegisterRegistrationRoutes(r *x.RouterPublic) {
}

func (s *Strategy) PopulateRegistrationMethod(r *http.Request, f *registration.Flow) error {
return s.populateMethod(r, f.UI, text.NewInfoRegistrationWith)
return s.populateMethod(r, f, text.NewInfoRegistrationWith)
}

// Update Registration Flow with OpenID Connect Method
Expand Down
24 changes: 22 additions & 2 deletions selfservice/strategy/oidc/strategy_settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,17 @@ func TestSettingsStrategy(t *testing.T) {
_, res, req := unlink(t, agent, provider)
assert.Contains(t, res.Request.URL.String(), uiTS.URL+"/login")

rs, _, err := testhelpers.NewSDKCustomClient(publicTS, agents[agent]).FrontendApi.GetSettingsFlow(context.Background()).Id(req.Id).Execute()
fa := testhelpers.NewSDKCustomClient(publicTS, agents[agent]).FrontendApi
lf, _, err := fa.GetLoginFlow(context.Background()).Id(res.Request.URL.Query()["flow"][0]).Execute()
require.NoError(t, err)

for _, node := range lf.Ui.Nodes {
if node.Group == "oidc" && node.Attributes.UiNodeInputAttributes.Name == "provider" {
assert.Contains(t, []string{"ory", "github"}, node.Attributes.UiNodeInputAttributes.Value)
}
}

rs, _, err := fa.GetSettingsFlow(context.Background()).Id(req.Id).Execute()
require.NoError(t, err)
require.EqualValues(t, flow.StateShowForm, rs.State)

Expand Down Expand Up @@ -554,7 +564,17 @@ func TestSettingsStrategy(t *testing.T) {
_, res, req := link(t, agent, provider)
assert.Contains(t, res.Request.URL.String(), uiTS.URL+"/login")

rs, _, err := testhelpers.NewSDKCustomClient(publicTS, agents[agent]).FrontendApi.GetSettingsFlow(context.Background()).Id(req.Id).Execute()
fa := testhelpers.NewSDKCustomClient(publicTS, agents[agent]).FrontendApi
lf, _, err := fa.GetLoginFlow(context.Background()).Id(res.Request.URL.Query()["flow"][0]).Execute()
require.NoError(t, err)

for _, node := range lf.Ui.Nodes {
if node.Group == "oidc" && node.Attributes.UiNodeInputAttributes.Name == "provider" {
assert.Contains(t, []string{"ory", "github"}, node.Attributes.UiNodeInputAttributes.Value)
}
}

rs, _, err := fa.GetSettingsFlow(context.Background()).Id(req.Id).Execute()
require.NoError(t, err)
require.EqualValues(t, flow.StateShowForm, rs.State)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,19 @@ context("Social Sign In Settings Success", () => {
hydraReauthFails()
})

it("should show only linked providers during reauth", () => {
cy.shortPrivilegedSessionTime()

cy.get('input[name="password"]').type(gen.password())
cy.get('[value="password"]').click()

cy.location("pathname").should("equal", "/login")

cy.get('[value="hydra"]').should("exist")
cy.get('[value="google"]').should("not.exist")
cy.get('[value="github"]').should("not.exist")
})

it("settings screen stays intact when the original sign up method gets removed", () => {
const expectSettingsOk = () => {
cy.get('[value="google"]', { timeout: 1000 })
Expand Down

0 comments on commit 912dccd

Please sign in to comment.