Merge pull request #102 from stlaz/repeat_fetch_3

providers: don't cache login and redeem URLs

Author: openshift-merge-robot

oauthproxy.go

@@ -319,9 +319,9 @@ func (p *OAuthProxy) redeemCode(host, code string) (s *providers.SessionState, e
 	}
 	redirectURI := p.GetRedirectURI(host)
 
-	redeemURL := p.provider.GetRedeemURL()
-	if redeemURL == nil {
-		return s, fmt.Errorf("unable to discover the redeeem endpoint")
+	redeemURL, err := p.provider.GetRedeemURL()
+	if err != nil {
+		return s, err
 	}
 	s, err = p.provider.Redeem(redeemURL, redirectURI, code)
 	if err != nil {
@@ -612,13 +612,10 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	// clear cached endpoints, get fresh ones in case /.well-known/oauth-authorization-server changed its values
-	p.provider.ClearEndpointsCache()
-
 	redirectURI := p.GetRedirectURI(req.Host)
-	loginURL := p.provider.GetLoginURL()
-	if loginURL == nil {
-		p.ErrorPage(rw, 500, "Internal Error", fmt.Sprintf("could not get login endpoint"))
+	loginURL, err := p.provider.GetLoginURL()
+	if err != nil {
+		p.ErrorPage(rw, 500, "Internal Error", err.Error())
 		return
 	}
 	http.Redirect(rw, req, p.provider.GetLoginRedirectURL(*loginURL, redirectURI, fmt.Sprintf("%v:%v", nonce, redirect)), 302)

oauthproxy_test.go

@@ -185,12 +185,12 @@ func NewTestProvider(provider_url *url.URL, email_address string) *TestProvider
 	return &TestProvider{
 		ProviderData: &providers.ProviderData{
 			ProviderName: "Test Provider",
-			LoginURL: &url.URL{
+			ConfigLoginURL: &url.URL{
 				Scheme: "http",
 				Host:   provider_url.Host,
 				Path:   "/oauth/authorize",
 			},
-			RedeemURL: &url.URL{
+			ConfigRedeemURL: &url.URL{
 				Scheme: "http",
 				Host:   provider_url.Host,
 				Path:   "/oauth/token",

options.go

@@ -336,8 +336,8 @@ func (o *Options) validateProvider(provider providers.Provider) []string {
 		p.ClientID = data.ClientID
 		p.ClientSecret = data.ClientSecret
 		p.ApprovalPrompt = data.ApprovalPrompt
-		p.LoginURL = data.LoginURL
-		p.RedeemURL = data.RedeemURL
+		p.ConfigLoginURL = data.ConfigLoginURL
+		p.ConfigRedeemURL = data.ConfigRedeemURL
 		p.ProfileURL = data.ProfileURL
 		p.ValidateURL = data.ValidateURL
 	}

providers/openshift/provider.go

@@ -108,11 +108,6 @@ func (p *OpenShiftProvider) LoadDefaults(serviceAccount string, caPaths []string
 		}
 	}
 
-	// attempt to discover endpoints
-	if err := discoverOpenShiftOAuth(defaults, p.Client); err != nil {
-		log.Printf("Unable to discover default cluster OAuth info: %v", err)
-		return defaults, nil
-	}
 	// provide default URLs
 	defaults.ValidateURL = getKubeAPIURLWithPath("/apis/user.openshift.io/v1/users/~")
 
@@ -510,67 +505,54 @@ func (p *OpenShiftProvider) Redeem(redeemURL *url.URL, redirectURL, code string)
 	return
 }
 
-func (p *OpenShiftProvider) GetLoginURL() *url.URL {
+func (p *OpenShiftProvider) GetLoginURL() (*url.URL, error) {
 	if !emptyURL(p.ConfigLoginURL) {
-		return p.ConfigLoginURL
+		return p.ConfigLoginURL, nil
 	}
 
-	if emptyURL(p.LoginURL) {
-		// clear the endpoints so that we get all the newly discovered endpoints for all
-		p.ClearEndpointsCache()
-		discoverOpenShiftOAuth(p.ProviderData, p.Client)
-	}
-	return p.LoginURL
+	loginURL, _, err := discoverOpenShiftOAuth(p.Client)
+	return loginURL, err
 }
 
-func (p *OpenShiftProvider) GetRedeemURL() *url.URL {
+func (p *OpenShiftProvider) GetRedeemURL() (*url.URL, error) {
 	if !emptyURL(p.ConfigRedeemURL) {
-		return p.ConfigRedeemURL
+		return p.ConfigRedeemURL, nil
 	}
 
-	if emptyURL(p.RedeemURL) {
-		// clear the endpoints so that we get all the newly discovered endpoints
-		p.ClearEndpointsCache()
-		discoverOpenShiftOAuth(p.ProviderData, p.Client)
-	}
-	return p.RedeemURL
+	_, redeemURL, err := discoverOpenShiftOAuth(p.Client)
+	return redeemURL, err
 }
 
-// discoverOpenshiftOAuth sets the LoginURL and RedeemURL of the supplied ProviderData if they are unset or empty
-func discoverOpenShiftOAuth(provider *providers.ProviderData, client *http.Client) error {
+// discoverOpenshiftOAuth returns the urls of the login and code redeem endpoitns
+// it receives from the /.well-known/oauth-authorization-server endpoint
+func discoverOpenShiftOAuth(client *http.Client) (*url.URL, *url.URL, error) {
 	wellKnownAuthorization := getKubeAPIURLWithPath("/.well-known/oauth-authorization-server")
 	log.Printf("Performing OAuth discovery against %s", wellKnownAuthorization)
 	req, err := http.NewRequest("GET", wellKnownAuthorization.String(), nil)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 	json, err := request(client, req)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
-	if emptyURL(provider.LoginURL) {
-		if value, err := json.Get("authorization_endpoint").String(); err == nil && len(value) > 0 {
-			if u, err := url.Parse(value); err == nil {
-				provider.LoginURL = u
-			} else {
-				log.Printf("Unable to parse 'authorization_endpoint' from %s: %v", wellKnownAuthorization, err)
-			}
-		} else {
-			log.Printf("No 'authorization_endpoint' provided by %s: %v", wellKnownAuthorization, err)
+
+	var loginURL, redeemURL *url.URL
+	if value, err := json.Get("authorization_endpoint").String(); err == nil && len(value) > 0 {
+		if loginURL, err = url.Parse(value); err != nil {
+			return nil, nil, fmt.Errorf("Unable to parse 'authorization_endpoint' from %s: %v", wellKnownAuthorization, err)
 		}
+	} else {
+		return nil, nil, fmt.Errorf("No 'authorization_endpoint' provided by %s: %v", wellKnownAuthorization, err)
 	}
-	if emptyURL(provider.RedeemURL) {
-		if value, err := json.Get("token_endpoint").String(); err == nil && len(value) > 0 {
-			if u, err := url.Parse(value); err == nil {
-				provider.RedeemURL = u
-			} else {
-				log.Printf("Unable to parse 'token_endpoint' from %s: %v", wellKnownAuthorization, err)
-			}
-		} else {
-			log.Printf("No 'token_endpoint' provided by %s: %v", wellKnownAuthorization, err)
+	if value, err := json.Get("token_endpoint").String(); err == nil && len(value) > 0 {
+		if redeemURL, err = url.Parse(value); err != nil {
+			return nil, nil, fmt.Errorf("Unable to parse 'token_endpoint' from %s: %v", wellKnownAuthorization, err)
 		}
+	} else {
+		return nil, nil, fmt.Errorf("No 'token_endpoint' provided by %s: %v", wellKnownAuthorization, err)
 	}
-	return nil
+	return loginURL, redeemURL, nil
 }
 
 // Copied to override http.Client so that CA can be set

providers/provider_data.go

@@ -8,12 +8,8 @@ type ProviderData struct {
 	ProviderName string
 	ClientID     string
 	ClientSecret string
-	// LoginURL, RedeemURL are cached in runtime, only set/unset
-	// them in cache-related methods
-	LoginURL  *url.URL
-	RedeemURL *url.URL
-	// Config* attributes are attributes that are set in options and override
-	// the cached attributes above
+	// Config* attributes are set in the options, if set, these endpoints won't
+	// be refetched
 	ConfigLoginURL    *url.URL
 	ConfigRedeemURL   *url.URL
 	ValidateURL       *url.URL

providers/provider_default.go

@@ -127,22 +127,18 @@ func (p *ProviderData) RefreshSessionIfNeeded(s *SessionState) (bool, error) {
 	return false, nil
 }
 
-func (p *ProviderData) GetLoginURL() *url.URL {
+func (p *ProviderData) GetLoginURL() (*url.URL, error) {
 	if !(p.ConfigLoginURL == nil || p.ConfigLoginURL.String() == "") {
-		return p.ConfigLoginURL
+		return p.ConfigLoginURL, nil
 	}
 
-	return p.LoginURL
+	return nil, fmt.Errorf("no login endpoint was configured")
 }
 
-func (p *ProviderData) GetRedeemURL() *url.URL {
+func (p *ProviderData) GetRedeemURL() (*url.URL, error) {
 	if !(p.ConfigRedeemURL == nil || p.ConfigRedeemURL.String() == "") {
-		return p.ConfigRedeemURL
+		return p.ConfigRedeemURL, nil
 	}
-	return p.RedeemURL
-}
 
-func (p *ProviderData) ClearEndpointsCache() {
-	p.LoginURL = nil
-	p.RedeemURL = nil
+	return nil, fmt.Errorf("no redeem endpoint was configured")
 }

providers/providers.go

@@ -21,9 +21,8 @@ type Provider interface {
 	SessionFromCookie(string, *cookie.Cipher) (*SessionState, error)
 	CookieForSession(*SessionState, *cookie.Cipher) (string, error)
 	ValidateRequest(*http.Request) (*SessionState, error)
-	GetLoginURL() *url.URL
-	GetRedeemURL() *url.URL
-	ClearEndpointsCache()
+	GetLoginURL() (*url.URL, error)
+	GetRedeemURL() (*url.URL, error)
 }
 
 // ErrPermissionDenied may be returned from Redeem() to indicate the user is not allowed to login.