git.iko.soy/typeable/oauth2-proxy
on master help sign in
Use `ErrNotImplemented` in default refresh implementation
commitff914d7e175566c59de93f88e4c05320f5367d07 authorNick Meves <nick.meves@greenhouse.io> dateSat 12 Jun 2021, 11:41 (5 years ago) parentbaf6cf3
5 files changed+54−29
compare with master
unifiedsplit
CHANGELOG.md+6 pkg/middleware/stored_session.go+18−11 pkg/middleware/stored_session_test.go+25−2 providers/provider_default.go+2−13 providers/provider_default_test.go+3−3
CHANGELOG.md+6
@@ -4,10 +4,16 @@
44
55## Important Notes
66
7+- [#1086](https://github.com/oauth2-proxy/oauth2-proxy/pull/1086) The extra validation to protect invalid session
8+ deserialization from v6.0.0 (only) has been removed to improve performance. If you are on v6.0.0, either upgrade
9+ to a version before this first and allow legacy sessions to expire gracefully or change your `cookie-secret`
10+ value and force all sessions to reauthenticate.
11+
712## Breaking Changes
813
914## Changes since v7.1.3
1015
16+- [#1086](https://github.com/oauth2-proxy/oauth2-proxy/pull/1086) Refresh sessions before token expiration if configured (@NickMeves)
1117- [#1226](https://github.com/oauth2-proxy/oauth2-proxy/pull/1226) Move app redirection logic to its own package (@JoelSpeed)
1218- [#1128](https://github.com/oauth2-proxy/oauth2-proxy/pull/1128) Use gorilla mux for OAuth Proxy routing (@JoelSpeed)
1319- [#1238](https://github.com/oauth2-proxy/oauth2-proxy/pull/1238) Added ADFS provider (@samirachoadi)
pkg/middleware/stored_session.go+18−11
@@ -11,19 +11,20 @@ import (
1111 middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
1212 sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
1313 "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
14+ "github.com/oauth2-proxy/oauth2-proxy/v7/providers"
1415)
1516
16−// StoredSessionLoaderOptions cotnains all of the requirements to construct
17+// StoredSessionLoaderOptions contains all of the requirements to construct
1718// a stored session loader.
1819// All options must be provided.
1920type StoredSessionLoaderOptions struct {
20− // Session storage basckend
21+ // Session storage backend
2122 SessionStore sessionsapi.SessionStore
2223
2324 // How often should sessions be refreshed
2425 RefreshPeriod time.Duration
2526
26− // Provider based sesssion refreshing
27+ // Provider based session refreshing
2728 RefreshSession func(context.Context, *sessionsapi.SessionState) (bool, error)
2829
2930 // Provider based session validation.
@@ -115,7 +116,7 @@ func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req
115116 return nil
116117 }
117118
118− logger.Printf("Refreshing %s old session cookie for %s (refresh after %s)", session.Age(), session, s.refreshPeriod)
119+ logger.Printf("Refreshing session - User: %s; SessionAge: %s", session.User, session.Age())
119120 err := s.refreshSession(rw, req, session)
120121 if err != nil {
121122 // If a preemptive refresh fails, we still keep the session
@@ -131,21 +132,27 @@ func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req
131132// and will save the session if it was updated.
132133func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) error {
133134 refreshed, err := s.sessionRefresher(req.Context(), session)
134− if err != nil {
135+ if err != nil && !errors.Is(err, providers.ErrNotImplemented) {
135136 return fmt.Errorf("error refreshing tokens: %v", err)
136137 }
137138
139+ // HACK:
140+ // Providers that don't implement `RefreshSession` use the default
141+ // implementation which returns `ErrNotImplemented`.
142+ // Pretend it refreshed to reset the refresh timer so that `ValidateSession`
143+ // isn't triggered every subsequent request and is only called once during
144+ // this request.
145+ if errors.Is(err, providers.ErrNotImplemented) {
146+ refreshed = true
147+ }
148+
149+ // Session not refreshed, nothing to persist.
138150 if !refreshed {
139151 return nil
140152 }
141153
142154 // If we refreshed, update the `CreatedAt` time to reset the refresh timer
143− //
144− // HACK:
145− // Providers that don't implement `RefreshSession` use the default
146− // implementation. It always returns `refreshed == true`, so the
147− // `session.CreatedAt` is updated and doesn't trigger `ValidateSession`
148− // every subsequent request.
155+ // (In case underlying provider implementations forget)
149156 session.CreatedAtNow()
150157
151158 // Because the session was refreshed, make sure to save it
pkg/middleware/stored_session_test.go+25−2
@@ -11,6 +11,7 @@ import (
1111 middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
1212 sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
1313 "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/clock"
14+ "github.com/oauth2-proxy/oauth2-proxy/v7/providers"
1415 . "github.com/onsi/ginkgo"
1516 . "github.com/onsi/ginkgo/extensions/table"
1617 . "github.com/onsi/gomega"
@@ -18,8 +19,9 @@ import (
1819
1920var _ = Describe("Stored Session Suite", func() {
2021 const (
21− refresh = "Refresh"
22− noRefresh = "NoRefresh"
22+ refresh = "Refresh"
23+ noRefresh = "NoRefresh"
24+ notImplemented = "NotImplemented"
2325 )
2426
2527 var ctx = context.Background()
@@ -293,6 +295,8 @@ var _ = Describe("Stored Session Suite", func() {
293295 return true, nil
294296 case noRefresh:
295297 return false, nil
298+ case notImplemented:
299+ return false, providers.ErrNotImplemented
296300 default:
297301 return false, errors.New("error refreshing session")
298302 }
@@ -364,6 +368,16 @@ var _ = Describe("Stored Session Suite", func() {
364368 expectRefreshed: true,
365369 expectValidated: true,
366370 }),
371+ Entry("when the provider doesn't implement refresh but validation succeeds", refreshSessionIfNeededTableInput{
372+ refreshPeriod: 1 * time.Minute,
373+ session: &sessionsapi.SessionState{
374+ RefreshToken: notImplemented,
375+ CreatedAt: &createdPast,
376+ },
377+ expectedErr: nil,
378+ expectRefreshed: true,
379+ expectValidated: true,
380+ }),
367381 Entry("when the provider refresh fails but validation succeeds", refreshSessionIfNeededTableInput{
368382 refreshPeriod: 1 * time.Minute,
369383 session: &sessionsapi.SessionState{
@@ -418,6 +432,8 @@ var _ = Describe("Stored Session Suite", func() {
418432 return true, nil
419433 case noRefresh:
420434 return false, nil
435+ case notImplemented:
436+ return false, providers.ErrNotImplemented
421437 default:
422438 return false, errors.New("error refreshing session")
423439 }
@@ -448,6 +464,13 @@ var _ = Describe("Stored Session Suite", func() {
448464 expectedErr: nil,
449465 expectSaved: true,
450466 }),
467+ Entry("when the provider doesn't implement refresh", refreshSessionWithProviderTableInput{
468+ session: &sessionsapi.SessionState{
469+ RefreshToken: notImplemented,
470+ },
471+ expectedErr: nil,
472+ expectSaved: true,
473+ }),
451474 Entry("when the provider returns an error", refreshSessionWithProviderTableInput{
452475 session: &sessionsapi.SessionState{
453476 RefreshToken: "RefreshError",
providers/provider_default.go+2−13
@@ -130,19 +130,8 @@ func (p *ProviderData) ValidateSession(ctx context.Context, s *sessions.SessionS
130130}
131131
132132// RefreshSession refreshes the user's session
133−func (p *ProviderData) RefreshSession(_ context.Context, s *sessions.SessionState) (bool, error) {
134− if s == nil {
135− return false, nil
136− }
137−
138− // HACK:
139− // Pretend `RefreshSession` occurred so `ValidateSession` isn't called
140− // on every request after any potential set refresh period elapses.
141− // See `middleware.refreshSession` for detailed logic & explanation.
142− //
143− // Intentionally doesn't use `ErrNotImplemented` since all providers will
144− // call this and we don't want to force them to implement this dummy logic.
145− return true, nil
133+func (p *ProviderData) RefreshSession(_ context.Context, _ *sessions.SessionState) (bool, error) {
134+ return false, ErrNotImplemented
146135}
147136
148137// CreateSessionFromToken converts Bearer IDTokens into sessions
providers/provider_default_test.go+3−3
@@ -22,12 +22,12 @@ func TestRefresh(t *testing.T) {
2222 ss.SetExpiresOn(expires)
2323
2424 refreshed, err := p.RefreshSession(context.Background(), ss)
25− assert.True(t, refreshed)
26− assert.NoError(t, err)
25+ assert.False(t, refreshed)
26+ assert.Equal(t, ErrNotImplemented, err)
2727
2828 refreshed, err = p.RefreshSession(context.Background(), nil)
2929 assert.False(t, refreshed)
30− assert.NoError(t, err)
30+ assert.Equal(t, ErrNotImplemented, err)
3131}
3232
3333func TestAcrValuesNotConfigured(t *testing.T) {