Skip to content

Commit

Permalink
fix: resolve broken OIDC tests and disallow API flows
Browse files Browse the repository at this point in the history
  • Loading branch information
aeneasr committed Aug 27, 2020
1 parent 045ecab commit 9986d8f
Show file tree
Hide file tree
Showing 12 changed files with 201 additions and 178 deletions.
3 changes: 3 additions & 0 deletions selfservice/strategy/oidc/error.go
Expand Up @@ -10,4 +10,7 @@ var (
ErrIDTokenMissing = herodot.ErrBadRequest.
WithError("authentication failed because id_token is missing").
WithReasonf(`Authentication failed because no id_token was returned. Please accept the "openid" permission and try again.`)

ErrAPIFlowNotSupported = herodot.ErrBadRequest.WithError("API-based flows are not supported for this method").
WithReasonf("Social Sign In and OpenID Connect are only supported for flows initiated using the Browser endpoint.")
)
2 changes: 1 addition & 1 deletion selfservice/strategy/oidc/provider.go
Expand Up @@ -10,7 +10,7 @@ type Provider interface {
Config() *Configuration
OAuth2(ctx context.Context) (*oauth2.Config, error)
Claims(ctx context.Context, exchange *oauth2.Token) (*Claims, error)
AuthCodeURLOptions(r request) []oauth2.AuthCodeOption
AuthCodeURLOptions(r ider) []oauth2.AuthCodeOption
}

type Claims struct {
Expand Down
2 changes: 1 addition & 1 deletion selfservice/strategy/oidc/provider_generic_oidc.go
Expand Up @@ -72,7 +72,7 @@ func (g *ProviderGenericOIDC) OAuth2(ctx context.Context) (*oauth2.Config, error
return g.oauth2ConfigFromEndpoint(endpoint), nil
}

func (g *ProviderGenericOIDC) AuthCodeURLOptions(r request) []oauth2.AuthCodeOption {
func (g *ProviderGenericOIDC) AuthCodeURLOptions(r ider) []oauth2.AuthCodeOption {
if isForced(r) {
return []oauth2.AuthCodeOption{
oauth2.SetAuthURLParam("prompt", "login"),
Expand Down
2 changes: 1 addition & 1 deletion selfservice/strategy/oidc/provider_github.go
Expand Up @@ -50,7 +50,7 @@ func (g *ProviderGitHub) OAuth2(ctx context.Context) (*oauth2.Config, error) {
return g.oauth2(), nil
}

func (g *ProviderGitHub) AuthCodeURLOptions(r request) []oauth2.AuthCodeOption {
func (g *ProviderGitHub) AuthCodeURLOptions(r ider) []oauth2.AuthCodeOption {
return []oauth2.AuthCodeOption{}
}

Expand Down
53 changes: 33 additions & 20 deletions selfservice/strategy/oidc/strategy.go
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/ory/kratos/identity"
"github.com/ory/kratos/schema"
"github.com/ory/kratos/selfservice/errorx"
"github.com/ory/kratos/selfservice/flow"
"github.com/ory/kratos/selfservice/flow/login"
"github.com/ory/kratos/selfservice/flow/registration"
"github.com/ory/kratos/selfservice/flow/settings"
Expand All @@ -36,9 +37,9 @@ import (
)

const (
BasePath = "/self-service/browser/flows/strategies/oidc"
BasePath = "/self-service/methods/oidc"

AuthPath = BasePath + "/auth/:request"
AuthPath = BasePath + "/auth/:flow"
CallbackPath = BasePath + "/callback/:provider"
)

Expand Down Expand Up @@ -97,9 +98,9 @@ type Strategy struct {
}

type authCodeContainer struct {
RequestID string `json:"request_id"`
State string `json:"state"`
Form url.Values `json:"form"`
FlowID string `json:"flow_id"`
State string `json:"state"`
Form url.Values `json:"form"`
}

func (s *Strategy) CountActiveCredentials(cc map[identity.CredentialsType]identity.Credentials) (count int, err error) {
Expand Down Expand Up @@ -158,7 +159,7 @@ func (s *Strategy) ID() identity.CredentialsType {
}

func (s *Strategy) handleAuth(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
rid := x.ParseUUID(ps.ByName("request"))
rid := x.ParseUUID(ps.ByName("flow"))
if err := r.ParseForm(); err != nil {
s.handleError(w, r, rid, "", nil, errors.WithStack(herodot.ErrBadRequest.WithDebug(err.Error()).WithReasonf("Unable to parse HTTP form request: %s", err.Error())))
return
Expand All @@ -182,7 +183,7 @@ func (s *Strategy) handleAuth(w http.ResponseWriter, r *http.Request, ps httprou
return
}

req, err := s.validateRequest(r.Context(), r, rid)
req, err := s.validateFlow(r.Context(), r, rid)
if err != nil {
s.handleError(w, r, rid, pid, nil, err)
return
Expand All @@ -195,9 +196,9 @@ func (s *Strategy) handleAuth(w http.ResponseWriter, r *http.Request, ps httprou
state := x.NewUUID().String()
if err := s.d.ContinuityManager().Pause(r.Context(), w, r, sessionName,
continuity.WithPayload(&authCodeContainer{
State: state,
RequestID: rid.String(),
Form: r.PostForm,
State: state,
FlowID: rid.String(),
Form: r.PostForm,
}),
continuity.WithLifespan(time.Minute*30)); err != nil {
s.handleError(w, r, rid, pid, nil, err)
Expand All @@ -207,19 +208,27 @@ func (s *Strategy) handleAuth(w http.ResponseWriter, r *http.Request, ps httprou
http.Redirect(w, r, config.AuthCodeURL(state, provider.AuthCodeURLOptions(req)...), http.StatusFound)
}

func (s *Strategy) validateRequest(ctx context.Context, r *http.Request, rid uuid.UUID) (request, error) {
func (s *Strategy) validateFlow(ctx context.Context, r *http.Request, rid uuid.UUID) (ider, error) {
if x.IsZeroUUID(rid) {
return nil, errors.WithStack(herodot.ErrBadRequest.WithReason("The session cookie contains invalid values and the request could not be executed. Please try again."))
return nil, errors.WithStack(herodot.ErrBadRequest.WithReason("The session cookie contains invalid values and the flow could not be executed. Please try again."))
}

if ar, err := s.d.RegistrationFlowPersister().GetRegistrationFlow(ctx, rid); err == nil {
if ar.Type != flow.TypeBrowser {
return ar, ErrAPIFlowNotSupported
}

if err := ar.Valid(); err != nil {
return ar, err
}
return ar, nil
}

if ar, err := s.d.LoginFlowPersister().GetLoginFlow(ctx, rid); err == nil {
if ar.Type != flow.TypeBrowser {
return ar, ErrAPIFlowNotSupported
}

if err := ar.Valid(); err != nil {
return ar, err
}
Expand All @@ -228,6 +237,10 @@ func (s *Strategy) validateRequest(ctx context.Context, r *http.Request, rid uui

ar, err := s.d.SettingsFlowPersister().GetSettingsFlow(ctx, rid)
if err == nil {
if ar.Type != flow.TypeBrowser {
return ar, ErrAPIFlowNotSupported
}

sess, err := s.d.SessionManager().FetchFromRequest(ctx, r)
if err != nil {
return ar, err
Expand All @@ -242,7 +255,7 @@ func (s *Strategy) validateRequest(ctx context.Context, r *http.Request, rid uui
return ar, err // this must return the error
}

func (s *Strategy) validateCallback(w http.ResponseWriter, r *http.Request) (request, *authCodeContainer, error) {
func (s *Strategy) validateCallback(w http.ResponseWriter, r *http.Request) (ider, *authCodeContainer, error) {
var (
code = r.URL.Query().Get("code")
state = r.URL.Query().Get("state")
Expand All @@ -261,7 +274,7 @@ func (s *Strategy) validateCallback(w http.ResponseWriter, r *http.Request) (req
return nil, &container, errors.WithStack(herodot.ErrBadRequest.WithReasonf(`Unable to complete OpenID Connect flow because the query state parameter does not match the state parameter from the session cookie.`))
}

req, err := s.validateRequest(r.Context(), r, x.ParseUUID(container.RequestID))
req, err := s.validateFlow(r.Context(), r, x.ParseUUID(container.FlowID))
if err != nil {
return nil, &container, err
}
Expand All @@ -281,7 +294,7 @@ func (s *Strategy) alreadyAuthenticated(w http.ResponseWriter, r *http.Request,
// we assume an error means the user has no session
if _, err := s.d.SessionManager().FetchFromRequest(r.Context(), r); err == nil {
if _, ok := req.(*settings.Flow); ok {
// ignore this if it's a settings request
// ignore this if it's a settings flow
} else if !isForced(req) {
http.Redirect(w, r, s.c.SelfServiceBrowserDefaultReturnTo().String(), http.StatusFound)
return true
Expand Down Expand Up @@ -361,26 +374,26 @@ func uid(provider, subject string) string {
return fmt.Sprintf("%s:%s", provider, subject)
}

func (s *Strategy) authURL(request uuid.UUID) string {
func (s *Strategy) authURL(flowID uuid.UUID) string {
return urlx.AppendPaths(
urlx.Copy(s.c.SelfPublicURL()),
strings.Replace(
AuthPath, ":request", request.String(), 1,
AuthPath, ":flow", flowID.String(), 1,
),
).String()
}

func (s *Strategy) populateMethod(r *http.Request, request uuid.UUID) (*RequestMethod, error) {
func (s *Strategy) populateMethod(r *http.Request, flowID uuid.UUID) (*FlowMethod, error) {
conf, err := s.Config()
if err != nil {
return nil, err
}

f := form.NewHTMLForm(s.authURL(request))
f := form.NewHTMLForm(s.authURL(flowID))
f.SetCSRF(s.d.GenerateCSRFToken(r))
// does not need sorting because there is only one field

return NewRequestMethodConfig(f).AddProviders(conf.Providers), nil
return NewFlowMethod(f).AddProviders(conf.Providers), nil
}

func (s *Strategy) Config() (*ConfigurationCollection, error) {
Expand Down
6 changes: 3 additions & 3 deletions selfservice/strategy/oidc/strategy_helper_test.go
Expand Up @@ -168,11 +168,11 @@ func newUI(t *testing.T, reg driver.Registry) *httptest.Server {
var e interface{}
var err error
if r.URL.Path == "/login" {
e, err = reg.LoginFlowPersister().GetLoginFlow(r.Context(), x.ParseUUID(r.URL.Query().Get("request")))
e, err = reg.LoginFlowPersister().GetLoginFlow(r.Context(), x.ParseUUID(r.URL.Query().Get("flow")))
} else if r.URL.Path == "/registration" {
e, err = reg.RegistrationFlowPersister().GetRegistrationFlow(r.Context(), x.ParseUUID(r.URL.Query().Get("request")))
e, err = reg.RegistrationFlowPersister().GetRegistrationFlow(r.Context(), x.ParseUUID(r.URL.Query().Get("flow")))
} else if r.URL.Path == "/settings" {
e, err = reg.SettingsFlowPersister().GetSettingsFlow(r.Context(), x.ParseUUID(r.URL.Query().Get("request")))
e, err = reg.SettingsFlowPersister().GetSettingsFlow(r.Context(), x.ParseUUID(r.URL.Query().Get("flow")))
}

require.NoError(t, err)
Expand Down
8 changes: 6 additions & 2 deletions selfservice/strategy/oidc/strategy_login.go
Expand Up @@ -22,6 +22,10 @@ func (s *Strategy) RegisterLoginRoutes(r *x.RouterPublic) {
}

func (s *Strategy) PopulateLoginMethod(r *http.Request, sr *login.Flow) error {
if sr.Type != flow.TypeBrowser {
return nil
}

config, err := s.populateMethod(r, sr.ID)
if err != nil {
return err
Expand All @@ -35,13 +39,13 @@ func (s *Strategy) processLogin(w http.ResponseWriter, r *http.Request, a *login
i, c, err := s.d.PrivilegedIdentityPool().FindByCredentialsIdentifier(r.Context(), identity.CredentialsTypeOIDC, uid(provider.Config().ID, claims.Subject))
if err != nil {
if errors.Is(err, herodot.ErrNotFound) {
// If no account was found we're "manually" creating a new registration request and redirecting the browser
// If no account was found we're "manually" creating a new registration flow and redirecting the browser
// to that endpoint.

// That will execute the "pre registration" hook which allows to e.g. disallow this request. The registration
// ui however will NOT be shown, instead the user is directly redirected to the auth path. That should then
// do a silent re-request. While this might be a bit excessive from a network perspective it should usually
// happen without any downsides to user experience as the request has already been authorized and should
// happen without any downsides to user experience as the flow has already been authorized and should
// not need additional consent/login.

// This is kinda hacky but the only way to ensure seamless login/registration flows when using OIDC.
Expand Down
8 changes: 6 additions & 2 deletions selfservice/strategy/oidc/strategy_registration.go
Expand Up @@ -33,6 +33,10 @@ func (s *Strategy) RegisterRegistrationRoutes(r *x.RouterPublic) {
}

func (s *Strategy) PopulateRegistrationMethod(r *http.Request, sr *registration.Flow) error {
if sr.Type != flow.TypeBrowser {
return nil
}

config, err := s.populateMethod(r, sr.ID)
if err != nil {
return err
Expand All @@ -48,7 +52,7 @@ func (s *Strategy) processRegistration(w http.ResponseWriter, r *http.Request, a
if _, _, err := s.d.PrivilegedIdentityPool().FindByCredentialsIdentifier(r.Context(), identity.CredentialsTypeOIDC, uid(provider.Config().ID, claims.Subject)); err == nil {
// If the identity already exists, we should perform the login flow instead.

// That will execute the "pre login" hook which allows to e.g. disallow this request. The login
// That will execute the "pre registration" hook which allows to e.g. disallow this flow. The registration
// ui however will NOT be shown, instead the user is directly redirected to the auth path. That should then
// do a silent re-request. While this might be a bit excessive from a network perspective it should usually
// happen without any downsides to user experience as the request has already been authorized and should
Expand All @@ -59,7 +63,7 @@ func (s *Strategy) processRegistration(w http.ResponseWriter, r *http.Request, a
WithField("subject", claims.Subject).
Debug("Received successful OpenID Connect callback but user is already registered. Re-initializing login flow now.")

// This endpoint only handles browser requests at the moment.
// This endpoint only handles browser flow at the moment.
ar, err := s.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser)
if err != nil {
s.handleError(w, r, a.GetID(), provider.Config().ID, nil, err)
Expand Down
35 changes: 22 additions & 13 deletions selfservice/strategy/oidc/strategy_settings.go
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/ory/x/urlx"

"github.com/ory/kratos/identity"
"github.com/ory/kratos/selfservice/flow"
"github.com/ory/kratos/selfservice/flow/settings"
"github.com/ory/kratos/selfservice/form"
"github.com/ory/kratos/x"
Expand Down Expand Up @@ -114,6 +115,10 @@ func (s *Strategy) linkableProviders(conf *ConfigurationCollection, confidential
}

func (s *Strategy) PopulateSettingsMethod(r *http.Request, id *identity.Identity, sr *settings.Flow) error {
if sr.Type != flow.TypeBrowser {
return nil
}

conf, err := s.Config()
if err != nil {
return err
Expand Down Expand Up @@ -156,7 +161,7 @@ func (s *Strategy) PopulateSettingsMethod(r *http.Request, id *identity.Identity

sr.Methods[s.SettingsStrategyID()] = &settings.FlowMethod{
Method: s.SettingsStrategyID(),
Config: &settings.FlowMethodConfig{FlowMethodConfigurator: NewRequestMethodConfig(f)},
Config: &settings.FlowMethodConfig{FlowMethodConfigurator: NewFlowMethod(f)},
}

return nil
Expand All @@ -180,18 +185,18 @@ type completeSelfServiceBrowserSettingsOIDCFlowPayload struct {
// in: body
Unlink string `json:"unlink"`

// RequestID is request ID.
// Flow ID is the flow's ID.
//
// in: query
RequestID string `json:"request_id"`
FlowID string `json:"flow"`
}

func (p *completeSelfServiceBrowserSettingsOIDCFlowPayload) GetRequestID() uuid.UUID {
return x.ParseUUID(p.RequestID)
func (p *completeSelfServiceBrowserSettingsOIDCFlowPayload) GetFlowID() uuid.UUID {
return x.ParseUUID(p.FlowID)
}

func (p *completeSelfServiceBrowserSettingsOIDCFlowPayload) SetRequestID(rid uuid.UUID) {
p.RequestID = rid.String()
func (p *completeSelfServiceBrowserSettingsOIDCFlowPayload) SetFlowID(rid uuid.UUID) {
p.FlowID = rid.String()
}

// swagger:route POST /self-service/browser/flows/registration/strategies/oidc/settings/connections public completeSelfServiceBrowserSettingsOIDCSettingsFlow
Expand Down Expand Up @@ -233,6 +238,10 @@ func (s *Strategy) completeSettingsFlow(w http.ResponseWriter, r *http.Request,
return
}

if err := r.ParseForm(); err != nil {
s.handleSettingsError(w, r, ctxUpdate, &p, err)
}

p.Link = r.Form.Get("link")
p.Unlink = r.Form.Get("unlink")
if l, u := len(p.Link), len(p.Unlink); l > 0 && u > 0 {
Expand Down Expand Up @@ -292,21 +301,21 @@ func (s *Strategy) initLinkProvider(w http.ResponseWriter, r *http.Request, ctxU
}

if ctxUpdate.Session.AuthenticatedAt.Add(s.c.SelfServiceFlowSettingsPrivilegedSessionMaxAge()).Before(time.Now()) {
s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(settings.ErrRequestNeedsReAuthentication))
s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(settings.NewFlowNeedsReAuth()))
return
}

http.Redirect(w, r, urlx.CopyWithQuery(urlx.AppendPaths(s.c.SelfPublicURL(),
strings.Replace(AuthPath, ":request", p.RequestID, 1)),
strings.Replace(AuthPath, ":flow", p.FlowID, 1)),
url.Values{"provider": {p.Link}}).String(), http.StatusFound)
}

func (s *Strategy) linkProvider(w http.ResponseWriter, r *http.Request,
ctxUpdate *settings.UpdateContext, claims *Claims, provider Provider) {
p := &completeSelfServiceBrowserSettingsOIDCFlowPayload{
Link: provider.Config().ID, RequestID: ctxUpdate.Flow.ID.String()}
Link: provider.Config().ID, FlowID: ctxUpdate.Flow.ID.String()}
if ctxUpdate.Session.AuthenticatedAt.Add(s.c.SelfServiceFlowSettingsPrivilegedSessionMaxAge()).Before(time.Now()) {
s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(settings.ErrRequestNeedsReAuthentication))
s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(settings.NewFlowNeedsReAuth()))
return
}

Expand Down Expand Up @@ -350,7 +359,7 @@ func (s *Strategy) linkProvider(w http.ResponseWriter, r *http.Request,
func (s *Strategy) unlinkProvider(w http.ResponseWriter, r *http.Request,
ctxUpdate *settings.UpdateContext, p *completeSelfServiceBrowserSettingsOIDCFlowPayload) {
if ctxUpdate.Session.AuthenticatedAt.Add(s.c.SelfServiceFlowSettingsPrivilegedSessionMaxAge()).Before(time.Now()) {
s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(settings.ErrRequestNeedsReAuthentication))
s.handleSettingsError(w, r, ctxUpdate, p, errors.WithStack(settings.NewFlowNeedsReAuth()))
return
}

Expand Down Expand Up @@ -417,7 +426,7 @@ func (s *Strategy) unlinkProvider(w http.ResponseWriter, r *http.Request,
}

func (s *Strategy) handleSettingsError(w http.ResponseWriter, r *http.Request, ctxUpdate *settings.UpdateContext, p *completeSelfServiceBrowserSettingsOIDCFlowPayload, err error) {
if errors.Is(err, settings.ErrRequestNeedsReAuthentication) {
if e := new(settings.FlowNeedsReAuth); errors.As(err, &e) {
if err := s.d.ContinuityManager().Pause(r.Context(), w, r,
settings.ContinuityKey(s.SettingsStrategyID()), settings.ContinuityOptions(p, ctxUpdate.Session.Identity)...); err != nil {
s.d.SettingsFlowErrorHandler().WriteFlowError(w, r, s.SettingsStrategyID(), ctxUpdate.Flow, ctxUpdate.Session.Identity, err)
Expand Down

0 comments on commit 9986d8f

Please sign in to comment.