Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

server,cmd: Add flag for disabling registation #144

Merged
merged 2 commits into from
Oct 1, 2015
Merged

server,cmd: Add flag for disabling registation #144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf9517f
server,cmd: Add flag for disabling registation
Sep 30, 2015
d3d6a75
fixup - Code review changes.
Oct 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions cmd/dex-worker/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ func main() {
emailFrom := fs.String("email-from", "[email protected]", "emails sent from dex will come from this address")
emailConfig := fs.String("email-cfg", "./static/fixtures/emailer.json", "configures emailer.")

enableRegistration := fs.Bool("enable-registration", true, "Allows users to self-register")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this default to true? While we might break existing installs if it defaults to false, it seems safer to have open registration be the explicit choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought process. yeah, you're probably right.


noDB := fs.Bool("no-db", false, "manage entities in-process w/o any encryption, used only for single-node testing")

// UI-related:
Expand Down Expand Up @@ -113,13 +115,14 @@ func main() {
}

scfg := server.ServerConfig{
IssuerURL: *issuer,
TemplateDir: *templates,
EmailTemplateDirs: emailTemplateDirs,
EmailFromAddress: *emailFrom,
EmailerConfigFile: *emailConfig,
IssuerName: *issuerName,
IssuerLogoURL: *issuerLogoURL,
IssuerURL: *issuer,
TemplateDir: *templates,
EmailTemplateDirs: emailTemplateDirs,
EmailFromAddress: *emailFrom,
EmailerConfigFile: *emailConfig,
IssuerName: *issuerName,
IssuerLogoURL: *issuerLogoURL,
EnableRegistration: *enableRegistration,
}

if *noDB {
Expand Down
27 changes: 17 additions & 10 deletions server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@ import (
)

type ServerConfig struct {
IssuerURL string
IssuerName string
IssuerLogoURL string
TemplateDir string
EmailTemplateDirs []string
EmailFromAddress string
EmailerConfigFile string
StateConfig StateConfigurer
IssuerURL string
IssuerName string
IssuerLogoURL string
TemplateDir string
EmailTemplateDirs []string
EmailFromAddress string
EmailerConfigFile string
StateConfig StateConfigurer
EnableRegistration bool
}

type StateConfigurer interface {
Expand All @@ -56,7 +57,7 @@ func (cfg *ServerConfig) Server() (*Server, error) {
return nil, err
}

tpl, err := getTemplates(cfg.IssuerName, cfg.IssuerLogoURL, cfg.TemplateDir)
tpl, err := getTemplates(cfg.IssuerName, cfg.IssuerLogoURL, cfg.EnableRegistration, cfg.TemplateDir)
if err != nil {
return nil, err
}
Expand All @@ -69,6 +70,8 @@ func (cfg *ServerConfig) Server() (*Server, error) {

HealthChecks: []health.Checkable{km},
Connectors: []connector.Connector{},

EnableRegistration: cfg.EnableRegistration,
}

err = cfg.StateConfig.Configure(&srv)
Expand Down Expand Up @@ -183,14 +186,18 @@ func (cfg *MultiServerConfig) Configure(srv *Server) error {
return nil
}

func getTemplates(issuerName, issuerLogoURL string, dir string) (*template.Template, error) {
func getTemplates(issuerName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider moving the line break until after the type, like

func getTemplates(issuerName, issuerLogoURL string,
    enableRegister bool....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, nicer.

issuerLogoURL string, enableRegister bool, dir string) (*template.Template, error) {
tpl := template.New("").Funcs(map[string]interface{}{
"issuerName": func() string {
return issuerName
},
"issuerLogoURL": func() string {
return issuerLogoURL
},
"enableRegister": func() bool {
return enableRegister
},
})

return tpl.ParseGlob(dir + "/*.html")
Expand Down
4 changes: 2 additions & 2 deletions server/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func renderLoginPage(w http.ResponseWriter, r *http.Request, srv OIDCServer, idp
execTemplate(w, tpl, td)
}

func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.Template) http.HandlerFunc {
func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.Template, registrationEnabled bool) http.HandlerFunc {
idx := makeConnectorMap(idpcs)
return func(w http.ResponseWriter, r *http.Request) {
if r.Method != "GET" {
Expand All @@ -264,7 +264,7 @@ func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.T
}

q := r.URL.Query()
register := q.Get("register") == "1"
register := q.Get("register") == "1" && registrationEnabled
e := q.Get("error")
if e != "" {
sessionKey := q.Get("state")
Expand Down
6 changes: 3 additions & 3 deletions server/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (c *fakeConnector) TrustedEmailProvider() bool {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth a test case demonstrating a rejection when an inbound request attempts to submit with register=1 and handleAuthFunc has been called with registrationEnabled=false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't really get rejected in this case, it just changes the UI. Given that the real protection is below, I don't think it's crucial

func TestHandleAuthFuncMethodNotAllowed(t *testing.T) {
for _, m := range []string{"POST", "PUT", "DELETE"} {
hdlr := handleAuthFunc(nil, nil, nil)
hdlr := handleAuthFunc(nil, nil, nil, true)
req, err := http.NewRequest(m, "http://example.com", nil)
if err != nil {
t.Errorf("case %s: unable to create HTTP request: %v", m, err)
Expand Down Expand Up @@ -170,7 +170,7 @@ func TestHandleAuthFuncResponsesSingleRedirectURL(t *testing.T) {
}

for i, tt := range tests {
hdlr := handleAuthFunc(srv, idpcs, nil)
hdlr := handleAuthFunc(srv, idpcs, nil, true)
w := httptest.NewRecorder()
u := fmt.Sprintf("http://server.example.com?%s", tt.query.Encode())
req, err := http.NewRequest("GET", u, nil)
Expand Down Expand Up @@ -271,7 +271,7 @@ func TestHandleAuthFuncResponsesMultipleRedirectURLs(t *testing.T) {
}

for i, tt := range tests {
hdlr := handleAuthFunc(srv, idpcs, nil)
hdlr := handleAuthFunc(srv, idpcs, nil, true)
w := httptest.NewRecorder()
u := fmt.Sprintf("http://server.example.com?%s", tt.query.Encode())
req, err := http.NewRequest("GET", u, nil)
Expand Down
9 changes: 7 additions & 2 deletions server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type Server struct {
PasswordInfoRepo user.PasswordInfoRepo
RefreshTokenRepo refresh.RefreshTokenRepo
UserEmailer *useremail.UserEmailer
EnableRegistration bool

localConnectorID string
}
Expand Down Expand Up @@ -198,11 +199,15 @@ func (s *Server) HTTPHandler() http.Handler {
clock := clockwork.NewRealClock()
mux := http.NewServeMux()
mux.HandleFunc(httpPathDiscovery, handleDiscoveryFunc(s.ProviderConfig()))
mux.HandleFunc(httpPathAuth, handleAuthFunc(s, s.Connectors, s.LoginTemplate))
mux.HandleFunc(httpPathAuth, handleAuthFunc(s, s.Connectors, s.LoginTemplate, s.EnableRegistration))
mux.HandleFunc(httpPathToken, handleTokenFunc(s))
mux.HandleFunc(httpPathKeys, handleKeysFunc(s.KeyManager, clock))
mux.Handle(httpPathHealth, makeHealthHandler(checks))
mux.HandleFunc(httpPathRegister, handleRegisterFunc(s))

if s.EnableRegistration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SWEET! This is a nice way to ensure that registration can't happen - it isn't even ever wired up!

mux.HandleFunc(httpPathRegister, handleRegisterFunc(s))
}

mux.HandleFunc(httpPathEmailVerify, handleEmailVerifyFunc(s.VerifyEmailTemplate,
s.IssuerURL, s.KeyManager.PublicKeys, s.UserManager))

Expand Down
3 changes: 2 additions & 1 deletion server/testutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ func makeTestFixtures() (*testFixtures, error) {
return nil, err
}

tpl, err := getTemplates("dex", "https://coreos.com/assets/images/brand/coreos-mark-30px.png", templatesLocation)
tpl, err := getTemplates("dex", "https://coreos.com/assets/images/brand/coreos-mark-30px.png",
true, templatesLocation)
if err != nil {
return nil, err
}
Expand Down
6 changes: 4 additions & 2 deletions static/html/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ <h2 class="heading">Log in to {{ issuerName }} </h2>
{{ if not .Error }}
<div class="footer subtle-text">
{{ if .Register }}
Already have an account? <a href="{{ .RegisterOrLoginURL }}">Log in</a>
Already have an account? <a href="{{ .RegisterOrLoginURL }}">Log in</a>
{{ else }}
Don't have an account yet? <a href="{{ .RegisterOrLoginURL }}">Register</a>
{{ if enableRegister }}
Don't have an account yet? <a href="{{ .RegisterOrLoginURL }}">Register</a>
{{ end }}
{{ end }}
</div>
{{ end }}
Expand Down