ff914d7 · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

on master help sign in

Use `ErrNotImplemented` in default refresh implementation

ff914d7Nick Meves5 years ago

CHANGELOG.md+6

compare with master

unifiedsplit

@@ -4,10 +4,16 @@

5## Important Notes

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+

7## Breaking Changes

12## Breaking Changes

9## Changes since v7.1.3

14## Changes since v7.1.3

16+- [#1086](https://github.com/oauth2-proxy/oauth2-proxy/pull/1086) Refresh sessions before token expiration if configured (@NickMeves)

11- [#1226](https://github.com/oauth2-proxy/oauth2-proxy/pull/1226) Move app redirection logic to its own package (@JoelSpeed)

17- [#1226](https://github.com/oauth2-proxy/oauth2-proxy/pull/1226) Move app redirection logic to its own package (@JoelSpeed)

12- [#1128](https://github.com/oauth2-proxy/oauth2-proxy/pull/1128) Use gorilla mux for OAuth Proxy routing (@JoelSpeed)

18- [#1128](https://github.com/oauth2-proxy/oauth2-proxy/pull/1128) Use gorilla mux for OAuth Proxy routing (@JoelSpeed)

13- [#1238](https://github.com/oauth2-proxy/oauth2-proxy/pull/1238) Added ADFS provider (@samirachoadi)

19- [#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 (

11 middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"

12 sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"

13 "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"

14+ "github.com/oauth2-proxy/oauth2-proxy/v7/providers"

14)

15)

16−// StoredSessionLoaderOptions cotnains all of the requirements to construct

17+// StoredSessionLoaderOptions contains all of the requirements to construct

17// a stored session loader.

18// a stored session loader.

18// All options must be provided.

19// All options must be provided.

19type StoredSessionLoaderOptions struct {

20type StoredSessionLoaderOptions struct {

20− // Session storage basckend

21+ // Session storage backend

21 SessionStore sessionsapi.SessionStore

22 SessionStore sessionsapi.SessionStore

23 // How often should sessions be refreshed

24 // How often should sessions be refreshed

24 RefreshPeriod time.Duration

25 RefreshPeriod time.Duration

26− // Provider based sesssion refreshing

27+ // Provider based session refreshing

27 RefreshSession func(context.Context, *sessionsapi.SessionState) (bool, error)

28 RefreshSession func(context.Context, *sessionsapi.SessionState) (bool, error)

29 // Provider based session validation.

30 // Provider based session validation.

@@ -115,7 +116,7 @@ func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req

115 return nil

116 return nil

116 }

117 }

117

118

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())

119 err := s.refreshSession(rw, req, session)

120 err := s.refreshSession(rw, req, session)

120 if err != nil {

121 if err != nil {

121 // If a preemptive refresh fails, we still keep the session

122 // If a preemptive refresh fails, we still keep the session

@@ -131,21 +132,27 @@ func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req

131// and will save the session if it was updated.

132// and will save the session if it was updated.

132func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) error {

133func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) error {

133 refreshed, err := s.sessionRefresher(req.Context(), session)

134 refreshed, err := s.sessionRefresher(req.Context(), session)

134− if err != nil {

135+ if err != nil && !errors.Is(err, providers.ErrNotImplemented) {

135 return fmt.Errorf("error refreshing tokens: %v", err)

136 return fmt.Errorf("error refreshing tokens: %v", err)

136 }

137 }

137

138

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.

138 if !refreshed {

150 if !refreshed {

139 return nil

151 return nil

140 }

152 }

141

153

142 // If we refreshed, update the `CreatedAt` time to reset the refresh timer

154 // If we refreshed, update the `CreatedAt` time to reset the refresh timer

143− //

155+ // (In case underlying provider implementations forget)

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.

149 session.CreatedAtNow()

156 session.CreatedAtNow()

150

157

151 // Because the session was refreshed, make sure to save it

158 // Because the session was refreshed, make sure to save it

pkg/middleware/stored_session_test.go+25−2

@@ -11,6 +11,7 @@ import (

11 middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"

12 sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"

13 "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/clock"

14+ "github.com/oauth2-proxy/oauth2-proxy/v7/providers"

14 . "github.com/onsi/ginkgo"

15 . "github.com/onsi/ginkgo"

15 . "github.com/onsi/ginkgo/extensions/table"

16 . "github.com/onsi/ginkgo/extensions/table"

16 . "github.com/onsi/gomega"

17 . "github.com/onsi/gomega"

@@ -18,8 +19,9 @@ import (

19var _ = Describe("Stored Session Suite", func() {

20var _ = Describe("Stored Session Suite", func() {

20 const (

21 const (

21− refresh = "Refresh"

22+ refresh = "Refresh"

22− noRefresh = "NoRefresh"

23+ noRefresh = "NoRefresh"

24+ notImplemented = "NotImplemented"

23 )

25 )

25 var ctx = context.Background()

27 var ctx = context.Background()

@@ -293,6 +295,8 @@ var _ = Describe("Stored Session Suite", func() {

293 return true, nil

295 return true, nil

294 case noRefresh:

296 case noRefresh:

295 return false, nil

297 return false, nil

298+ case notImplemented:

299+ return false, providers.ErrNotImplemented

296 default:

300 default:

297 return false, errors.New("error refreshing session")

301 return false, errors.New("error refreshing session")

298 }

302 }

@@ -364,6 +368,16 @@ var _ = Describe("Stored Session Suite", func() {

364 expectRefreshed: true,

368 expectRefreshed: true,

365 expectValidated: true,

369 expectValidated: true,

366 }),

370 }),

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+ }),

367 Entry("when the provider refresh fails but validation succeeds", refreshSessionIfNeededTableInput{

381 Entry("when the provider refresh fails but validation succeeds", refreshSessionIfNeededTableInput{

368 refreshPeriod: 1 * time.Minute,

382 refreshPeriod: 1 * time.Minute,

369 session: &sessionsapi.SessionState{

383 session: &sessionsapi.SessionState{

@@ -418,6 +432,8 @@ var _ = Describe("Stored Session Suite", func() {

418 return true, nil

432 return true, nil

419 case noRefresh:

433 case noRefresh:

420 return false, nil

434 return false, nil

435+ case notImplemented:

436+ return false, providers.ErrNotImplemented

421 default:

437 default:

422 return false, errors.New("error refreshing session")

438 return false, errors.New("error refreshing session")

423 }

439 }

@@ -448,6 +464,13 @@ var _ = Describe("Stored Session Suite", func() {

448 expectedErr: nil,

464 expectedErr: nil,

449 expectSaved: true,

465 expectSaved: true,

450 }),

466 }),

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+ }),

451 Entry("when the provider returns an error", refreshSessionWithProviderTableInput{

474 Entry("when the provider returns an error", refreshSessionWithProviderTableInput{

452 session: &sessionsapi.SessionState{

475 session: &sessionsapi.SessionState{

453 RefreshToken: "RefreshError",

476 RefreshToken: "RefreshError",

providers/provider_default.go+2−13

@@ -130,19 +130,8 @@ func (p *ProviderData) ValidateSession(ctx context.Context, s *sessions.SessionS

130}

131

132// RefreshSession refreshes the user's session

133−func (p *ProviderData) RefreshSession(_ context.Context, s *sessions.SessionState) (bool, error) {

133+func (p *ProviderData) RefreshSession(_ context.Context, _ *sessions.SessionState) (bool, error) {

134− if s == nil {

135− return false, nil

134+ return false, ErrNotImplemented

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

146}

135}

147

136

148// CreateSessionFromToken converts Bearer IDTokens into sessions

137// CreateSessionFromToken converts Bearer IDTokens into sessions

providers/provider_default_test.go+3−3

@@ -22,12 +22,12 @@ func TestRefresh(t *testing.T) {

22 ss.SetExpiresOn(expires)

24 refreshed, err := p.RefreshSession(context.Background(), ss)

25− assert.True(t, refreshed)

25+ assert.False(t, refreshed)

26− assert.NoError(t, err)

26+ assert.Equal(t, ErrNotImplemented, err)

28 refreshed, err = p.RefreshSession(context.Background(), nil)

29 assert.False(t, refreshed)

30− assert.NoError(t, err)

30+ assert.Equal(t, ErrNotImplemented, err)

31}

33func TestAcrValuesNotConfigured(t *testing.T) {