Repository: coreos/dex
Pull request #596 (2016-10-10 18:05:35)

dev branch: add tests for refreshing with explicit scopes


Patch:
diff --git a/cmd/example-app/main.go b/cmd/example-app/main.go
index 3e6c7006..56d1a2c8 100644
--- a/cmd/example-app/main.go
+++ b/cmd/example-app/main.go
@@ -30,10 +30,6 @@ type app struct {
 	verifier *oidc.IDTokenVerifier
 	provider *oidc.Provider
 
-	// Does the provider use "offline_access" scope to request a refresh token
-	// or does it use "access_type=offline" (e.g. Google)?
-	offlineAsScope bool
-
 	ctx    context.Context
 	cancel context.CancelFunc
 }
@@ -106,34 +102,6 @@ func cmd() *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("Failed to query provider %q: %v", issuerURL, err)
 			}
-
-			var s struct {
-				// What scopes does a provider support?
-				//
-				// See: https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata
-				ScopesSupported []string `json:"scopes_supported"`
-			}
-			if err := provider.Claims(&s); err != nil {
-				return fmt.Errorf("Failed to parse provider scopes_supported: %v", err)
-			}
-
-			if len(s.ScopesSupported) == 0 {
-				// scopes_supported is a "RECOMMENDED" discovery claim, not a required
-				// one. If missing, assume that the provider follows the spec and has
-				// an "offline_access" scope.
-				a.offlineAsScope = true
-			} else {
-				// See if scopes_supported has the "offline_access" scope.
-				a.offlineAsScope = func() bool {
-					for _, scope := range s.ScopesSupported {
-						if scope == oidc.ScopeOfflineAccess {
-							return true
-						}
-					}
-					return false
-				}()
-			}
-
 			a.provider = provider
 			a.verifier = provider.NewVerifier(a.ctx, oidc.VerifyAudience(a.clientID))
 
@@ -198,15 +166,10 @@ func (a *app) handleLogin(w http.ResponseWriter, r *http.Request) {
 		scopes = append(scopes, "audience:server:client_id:"+client)
 	}
 
-	authCodeURL := ""
-	scopes = append(scopes, "openid", "profile", "email")
-	if a.offlineAsScope {
-		scopes = append(scopes, "offline_access")
-		authCodeURL = a.oauth2Config(scopes).AuthCodeURL("")
-	} else {
-		authCodeURL = a.oauth2Config(scopes).AuthCodeURL("", oauth2.AccessTypeOffline)
-	}
-	http.Redirect(w, r, authCodeURL, http.StatusSeeOther)
+	// TODO(ericchiang): Determine if provider does not support "offline_access" or has
+	// some other mechanism for requesting refresh tokens.
+	scopes = append(scopes, "openid", "profile", "email", "offline_access")
+	http.Redirect(w, r, a.oauth2Config(scopes).AuthCodeURL(""), http.StatusSeeOther)
 }
 
 func (a *app) handleCallback(w http.ResponseWriter, r *http.Request) {
@@ -232,7 +195,7 @@ func (a *app) handleCallback(w http.ResponseWriter, r *http.Request) {
 		}
 		token, err = oauth2Config.TokenSource(a.ctx, t).Token()
 	default:
-		http.Error(w, fmt.Sprintf("no code in request: %q", r.Form), http.StatusBadRequest)
+		http.Error(w, "no code in request", http.StatusBadRequest)
 		return
 	}
 
diff --git a/server/handlers.go b/server/handlers.go
index 40621adc..02c1881b 100644
--- a/server/handlers.go
+++ b/server/handlers.go
@@ -538,20 +538,25 @@ func (s *Server) handleRefreshToken(w http.ResponseWriter, r *http.Request, clie
 	scopes := refresh.Scopes
 	if scope != "" {
 		requestedScopes := strings.Split(scope, " ")
-		contains := func() bool {
-		Loop:
-			for _, s := range requestedScopes {
+		var unauthorizedScopes []string
+
+		for _, s := range requestedScopes {
+			contains := func() bool {
 				for _, scope := range refresh.Scopes {
 					if s == scope {
-						continue Loop
+						return true
 					}
 				}
 				return false
+			}()
+			if !contains {
+				unauthorizedScopes = append(unauthorizedScopes, s)
 			}
-			return true
-		}()
-		if !contains {
-			tokenErr(w, errInvalidRequest, "Requested scopes did not contain authorized scopes.", http.StatusBadRequest)
+		}
+
+		if len(unauthorizedScopes) > 0 {
+			msg := fmt.Sprintf("Requested scopes contain unauthorized scope(s): %q.", unauthorizedScopes)
+			tokenErr(w, errInvalidRequest, msg, http.StatusBadRequest)
 			return
 		}
 		scopes = requestedScopes
diff --git a/server/oauth2.go b/server/oauth2.go
index 003d8947..61f6a923 100644
--- a/server/oauth2.go
+++ b/server/oauth2.go
@@ -52,6 +52,7 @@ func tokenErr(w http.ResponseWriter, typ, description string, statusCode int) {
 	}
 	w.Header().Set("Content-Type", "application/json")
 	w.Header().Set("Content-Length", strconv.Itoa(len(body)))
+	w.WriteHeader(statusCode)
 	w.Write(body)
 }
 


Test patch:
diff --git a/server/server_test.go b/server/server_test.go
index ea0793aa..13f46ac1 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -131,111 +131,191 @@ func TestDiscovery(t *testing.T) {
 }
 
 func TestOAuth2CodeFlow(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	httpServer, s := newTestServer(t, func(c *Config) {
-		c.Issuer = c.Issuer + "/non-root-path"
-	})
-	defer httpServer.Close()
-
-	p, err := oidc.NewProvider(ctx, httpServer.URL)
-	if err != nil {
-		t.Fatalf("failed to get provider: %v", err)
-	}
-
-	var (
-		reqDump, respDump []byte
-		gotCode           bool
-		state             = "a_state"
-	)
-	defer func() {
-		if !gotCode {
-			t.Errorf("never got a code in callback\n%s\n%s", reqDump, respDump)
-		}
-	}()
-
-	var oauth2Config *oauth2.Config
-	oauth2Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path == "/callback" {
-			q := r.URL.Query()
-			if errType := q.Get("error"); errType != "" {
-				if desc := q.Get("error_description"); desc != "" {
-					t.Errorf("got error from server %s: %s", errType, desc)
-				} else {
-					t.Errorf("got error from server %s", errType)
-				}
-				w.WriteHeader(http.StatusInternalServerError)
-				return
-			}
+	clientID := "testclient"
+	clientSecret := "testclientsecret"
+	requestedScopes := []string{oidc.ScopeOpenID, "email", "offline_access"}
 
-			if code := q.Get("code"); code != "" {
-				gotCode = true
-				token, err := oauth2Config.Exchange(ctx, code)
-				if err != nil {
-					t.Errorf("failed to exchange code for token: %v", err)
-					return
-				}
+	tests := []struct {
+		name        string
+		handleToken func(context.Context, *oidc.Provider, *oauth2.Config, *oauth2.Token) error
+	}{
+		{
+			name: "verify ID Token",
+			handleToken: func(ctx context.Context, p *oidc.Provider, config *oauth2.Config, token *oauth2.Token) error {
 				idToken, ok := token.Extra("id_token").(string)
 				if !ok {
-					t.Errorf("no id token found: %v", err)
-					return
+					return fmt.Errorf("no id token found")
 				}
-				// TODO(ericchiang): validate id token
-				_ = idToken
-
+				if _, err := p.NewVerifier(ctx).Verify(idToken); err != nil {
+					return fmt.Errorf("failed to verify id token: %v", err)
+				}
+				return nil
+			},
+		},
+		{
+			name: "refresh token",
+			handleToken: func(ctx context.Context, p *oidc.Provider, config *oauth2.Config, token *oauth2.Token) error {
+				// have to use time.Now because the OAuth2 package uses it.
 				token.Expiry = time.Now().Add(time.Second * -10)
 				if token.Valid() {
-					t.Errorf("token shouldn't be valid")
+					return errors.New("token shouldn't be valid")
 				}
 
-				newToken, err := oauth2Config.TokenSource(ctx, token).Token()
+				newToken, err := config.TokenSource(ctx, token).Token()
 				if err != nil {
-					t.Errorf("failed to refresh token: %v", err)
-					return
+					return fmt.Errorf("failed to refresh token: %v", err)
 				}
 				if token.RefreshToken == newToken.RefreshToken {
-					t.Errorf("old refresh token was the same as the new token %q", token.RefreshToken)
+					return fmt.Errorf("old refresh token was the same as the new token %q", token.RefreshToken)
 				}
+				return nil
+			},
+		},
+		{
+			name: "refresh with explicit scopes",
+			handleToken: func(ctx context.Context, p *oidc.Provider, config *oauth2.Config, token *oauth2.Token) error {
+				v := url.Values{}
+				v.Add("client_id", clientID)
+				v.Add("client_secret", clientSecret)
+				v.Add("grant_type", "refresh_token")
+				v.Add("refresh_token", token.RefreshToken)
+				v.Add("scope", strings.Join(requestedScopes, " "))
+				resp, err := http.PostForm(p.TokenURL, v)
+				if err != nil {
+					return err
+				}
+				defer resp.Body.Close()
+				if resp.StatusCode != http.StatusOK {
+					dump, err := httputil.DumpResponse(resp, true)
+					if err != nil {
+						panic(err)
+					}
+					return fmt.Errorf("unexpected response: %s", dump)
+				}
+				return nil
+			},
+		},
+		{
+			name: "refresh with unauthorized scopes",
+			handleToken: func(ctx context.Context, p *oidc.Provider, config *oauth2.Config, token *oauth2.Token) error {
+				v := url.Values{}
+				v.Add("client_id", clientID)
+				v.Add("client_secret", clientSecret)
+				v.Add("grant_type", "refresh_token")
+				v.Add("refresh_token", token.RefreshToken)
+				// Request a scope that wasn't requestd initially.
+				v.Add("scope", strings.Join(append(requestedScopes, "profile"), " "))
+				resp, err := http.PostForm(p.TokenURL, v)
+				if err != nil {
+					return err
+				}
+				defer resp.Body.Close()
+				if resp.StatusCode == http.StatusOK {
+					dump, err := httputil.DumpResponse(resp, true)
+					if err != nil {
+						panic(err)
+					}
+					return fmt.Errorf("unexpected response: %s", dump)
+				}
+				return nil
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		func() {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			httpServer, s := newTestServer(t, func(c *Config) {
+				c.Issuer = c.Issuer + "/non-root-path"
+			})
+			defer httpServer.Close()
+
+			p, err := oidc.NewProvider(ctx, httpServer.URL)
+			if err != nil {
+				t.Fatalf("failed to get provider: %v", err)
 			}
-			if gotState := q.Get("state"); gotState != state {
-				t.Errorf("state did not match, want=%q got=%q", state, gotState)
-			}
-			w.WriteHeader(http.StatusOK)
-			return
-		}
-		http.Redirect(w, r, oauth2Config.AuthCodeURL(state), http.StatusSeeOther)
-	}))
 
-	defer oauth2Server.Close()
+			var (
+				reqDump, respDump []byte
+				gotCode           bool
+				state             = "a_state"
+			)
+			defer func() {
+				if !gotCode {
+					t.Errorf("never got a code in callback\n%s\n%s", reqDump, respDump)
+				}
+			}()
+
+			var oauth2Config *oauth2.Config
+			oauth2Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if r.URL.Path == "/callback" {
+					q := r.URL.Query()
+					if errType := q.Get("error"); errType != "" {
+						if desc := q.Get("error_description"); desc != "" {
+							t.Errorf("got error from server %s: %s", errType, desc)
+						} else {
+							t.Errorf("got error from server %s", errType)
+						}
+						w.WriteHeader(http.StatusInternalServerError)
+						return
+					}
+
+					if code := q.Get("code"); code != "" {
+						gotCode = true
+						token, err := oauth2Config.Exchange(ctx, code)
+						if err != nil {
+							t.Errorf("failed to exchange code for token: %v", err)
+							return
+						}
+						err = tc.handleToken(ctx, p, oauth2Config, token)
+						if err != nil {
+							t.Errorf("%s: %v", tc.name, err)
+						}
+						return
+
+					}
+					if gotState := q.Get("state"); gotState != state {
+						t.Errorf("state did not match, want=%q got=%q", state, gotState)
+					}
+					w.WriteHeader(http.StatusOK)
+					return
+				}
+				http.Redirect(w, r, oauth2Config.AuthCodeURL(state), http.StatusSeeOther)
+			}))
 
-	redirectURL := oauth2Server.URL + "/callback"
-	client := storage.Client{
-		ID:           "testclient",
-		Secret:       "testclientsecret",
-		RedirectURIs: []string{redirectURL},
-	}
-	if err := s.storage.CreateClient(client); err != nil {
-		t.Fatalf("failed to create client: %v", err)
-	}
+			defer oauth2Server.Close()
 
-	oauth2Config = &oauth2.Config{
-		ClientID:     client.ID,
-		ClientSecret: client.Secret,
-		Endpoint:     p.Endpoint(),
-		Scopes:       []string{oidc.ScopeOpenID, "profile", "email", "offline_access"},
-		RedirectURL:  redirectURL,
-	}
+			redirectURL := oauth2Server.URL + "/callback"
+			client := storage.Client{
+				ID:           clientID,
+				Secret:       clientSecret,
+				RedirectURIs: []string{redirectURL},
+			}
+			if err := s.storage.CreateClient(client); err != nil {
+				t.Fatalf("failed to create client: %v", err)
+			}
 
-	resp, err := http.Get(oauth2Server.URL + "/login")
-	if err != nil {
-		t.Fatalf("get failed: %v", err)
-	}
-	if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil {
-		t.Fatal(err)
-	}
-	if respDump, err = httputil.DumpResponse(resp, true); err != nil {
-		t.Fatal(err)
+			oauth2Config = &oauth2.Config{
+				ClientID:     client.ID,
+				ClientSecret: client.Secret,
+				Endpoint:     p.Endpoint(),
+				Scopes:       requestedScopes,
+				RedirectURL:  redirectURL,
+			}
+
+			resp, err := http.Get(oauth2Server.URL + "/login")
+			if err != nil {
+				t.Fatalf("get failed: %v", err)
+			}
+			if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil {
+				t.Fatal(err)
+			}
+			if respDump, err = httputil.DumpResponse(resp, true); err != nil {
+				t.Fatal(err)
+			}
+		}()
 	}
 }