5931251 · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

Standarize provider refresh implemention & logging

commit593125152daa9e48ca34a8a8b021a3fcb234a13c authorNick Meves <nick.meves@greenhouse.io> dateSat 6 Mar 2021, 15:48 (6 years ago) parent7fa6d2d

10 files changed+123−70

compare with master

unifiedsplit

oauthproxy.go+3−3 pkg/apis/sessions/session_state_test.go+24 pkg/middleware/stored_session.go+12−8 pkg/middleware/stored_session_test.go+44−20 providers/azure.go+7−5 providers/azure_test.go+1−11 providers/google.go+12−15 providers/oidc.go−1 providers/provider_default.go+6−1 providers/provider_default_test.go+14−6

oauthproxy.go+3−3

@@ -870,9 +870,9 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) {

870870

871871// See https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching?hl=en

872872var noCacheHeaders = map[string]string{

873− "Expires": time.Unix(0, 0).Format(time.RFC1123),

874− "Cache-Control": "no-cache, no-store, must-revalidate, max-age=0",

875− "X-Accel-Expire": "0", // https://www.nginx.com/resources/wiki/start/topics/examples/x-accel/

873+ "Expires": time.Unix(0, 0).Format(time.RFC1123),

874+ "Cache-Control": "no-cache, no-store, must-revalidate, max-age=0",

875+ "X-Accel-Expires": "0", // https://www.nginx.com/resources/wiki/start/topics/examples/x-accel/

876876}

877877

878878// prepareNoCache prepares headers for preventing browser caching.

pkg/apis/sessions/session_state_test.go+24

@@ -16,6 +16,30 @@ func timePtr(t time.Time) *time.Time {

1616 return &t

1717}

1818

19+func TestCreatedAtNow(t *testing.T) {

20+ g := NewWithT(t)

21+ ss := &SessionState{}

22+

23+ now := time.Unix(1234567890, 0)

24+ ss.Clock.Set(now)

25+

26+ ss.CreatedAtNow()

27+ g.Expect(*ss.CreatedAt).To(Equal(now))

28+}

29+

30+func TestExpiresIn(t *testing.T) {

31+ g := NewWithT(t)

32+ ss := &SessionState{}

33+

34+ now := time.Unix(1234567890, 0)

35+ ss.Clock.Set(now)

36+

37+ ttl := time.Duration(743) * time.Second

38+ ss.ExpiresIn(ttl)

39+

40+ g.Expect(*ss.ExpiresOn).To(Equal(ss.CreatedAt.Add(ttl)))

41+}

42+

1943func TestString(t *testing.T) {

2044 g := NewWithT(t)

2145 created, err := time.Parse(time.RFC3339, "2000-01-01T00:00:00Z")

pkg/middleware/stored_session.go+12−8

@@ -108,11 +108,7 @@ func (s *storedSessionLoader) getValidatedSession(rw http.ResponseWriter, req *h

108108

109109// refreshSessionIfNeeded will attempt to refresh a session if the session

110110// is older than the refresh period.

111−// It is assumed that if the provider refreshes the session, the session is now

112−// valid.

113−// If the session requires refreshing but the provider does not refresh it,

114−// we must validate the session to ensure that the returned session is still

115−// valid.

111+// Success or fail, we will then validate the session.

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

117113 if s.refreshPeriod <= time.Duration(0) || session.Age() < s.refreshPeriod {

118114 // Refresh is disabled or the session is not old enough, do nothing

@@ -122,10 +118,12 @@ func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req

122118 logger.Printf("Refreshing %s old session cookie for %s (refresh after %s)", session.Age(), session, s.refreshPeriod)

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

124120 if err != nil {

125− return err

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

122+ // if validateSession succeeds.

123+ logger.Errorf("Unable to refresh session: %v", err)

126124 }

127125

128− // Validate all sessions after any Redeem/Refresh operation

126+ // Validate all sessions after any Redeem/Refresh operation (fail or success)

129127 return s.validateSession(req.Context(), session)

130128}

131129

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

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

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

136134 if err != nil {

137− return fmt.Errorf("error refreshing access token: %v", err)

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

138136 }

139137

140138 if !refreshed {

@@ -142,6 +140,12 @@ func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.R

142140 }

143141

144142 // 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.

145149 session.CreatedAtNow()

146150

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

pkg/middleware/stored_session_test.go+44−20

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

1010

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

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

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

1314 . "github.com/onsi/ginkgo"

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

1516 . "github.com/onsi/gomega"

@@ -24,8 +25,9 @@ var _ = Describe("Stored Session Suite", func() {

2425 var ctx = context.Background()

2526

2627 Context("StoredSessionLoader", func() {

27− createdPast := time.Now().Add(-5 * time.Minute)

28− createdFuture := time.Now().Add(5 * time.Minute)

28+ now := time.Now()

29+ createdPast := now.Add(-5 * time.Minute)

30+ createdFuture := now.Add(5 * time.Minute)

2931

3032 var defaultRefreshFunc = func(_ context.Context, ss *sessionsapi.SessionState) (bool, error) {

3133 switch ss.RefreshToken {

@@ -85,6 +87,14 @@ var _ = Describe("Stored Session Suite", func() {

8587 },

8688 }

8789

90+ BeforeEach(func() {

91+ clock.Set(now)

92+ })

93+

94+ AfterEach(func() {

95+ clock.Reset()

96+ })

97+

8898 type storedSessionLoaderTableInput struct {

8999 requestHeaders http.Header

90100 existingSession *sessionsapi.SessionState

@@ -208,6 +218,21 @@ var _ = Describe("Stored Session Suite", func() {

208218 existingSession: nil,

209219 expectedSession: &sessionsapi.SessionState{

210220 RefreshToken: "Refreshed",

221+ CreatedAt: &now,

222+ ExpiresOn: &createdFuture,

223+ },

224+ store: defaultSessionStore,

225+ refreshPeriod: 1 * time.Minute,

226+ refreshSession: defaultRefreshFunc,

227+ validateSession: defaultValidateFunc,

228+ }),

229+ Entry("when the provider refresh fails but validation succeeds", storedSessionLoaderTableInput{

230+ requestHeaders: http.Header{

231+ "Cookie": []string{"_oauth2_proxy=RefreshError"},

232+ },

233+ existingSession: nil,

234+ expectedSession: &sessionsapi.SessionState{

235+ RefreshToken: "RefreshError",

211236 CreatedAt: &createdPast,

212237 ExpiresOn: &createdFuture,

213238 },

@@ -216,7 +241,7 @@ var _ = Describe("Stored Session Suite", func() {

216241 refreshSession: defaultRefreshFunc,

217242 validateSession: defaultValidateFunc,

218243 }),

219− Entry("when the provider refresh fails", storedSessionLoaderTableInput{

244+ Entry("when the provider refresh fails and validation fails", storedSessionLoaderTableInput{

220245 requestHeaders: http.Header{

221246 "Cookie": []string{"_oauth2_proxy=RefreshError"},

222247 },

@@ -225,7 +250,7 @@ var _ = Describe("Stored Session Suite", func() {

225250 store: defaultSessionStore,

226251 refreshPeriod: 1 * time.Minute,

227252 refreshSession: defaultRefreshFunc,

228− validateSession: defaultValidateFunc,

253+ validateSession: func(context.Context, *sessionsapi.SessionState) bool { return false },

229254 }),

230255 Entry("when the session is not refreshed and is no longer valid", storedSessionLoaderTableInput{

231256 requestHeaders: http.Header{

@@ -326,7 +351,7 @@ var _ = Describe("Stored Session Suite", func() {

326351 },

327352 expectedErr: nil,

328353 expectRefreshed: true,

329− expectValidated: false,

354+ expectValidated: true,

330355 }),

331356 Entry("when the session is not refreshed by the provider", refreshSessionIfNeededTableInput{

332357 refreshPeriod: 1 * time.Minute,

@@ -339,15 +364,15 @@ var _ = Describe("Stored Session Suite", func() {

339364 expectRefreshed: true,

340365 expectValidated: true,

341366 }),

342− Entry("when the provider refresh fails", refreshSessionIfNeededTableInput{

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

343368 refreshPeriod: 1 * time.Minute,

344369 session: &sessionsapi.SessionState{

345370 RefreshToken: "RefreshError",

346371 CreatedAt: &createdPast,

347372 },

348− expectedErr: errors.New("error refreshing access token: error refreshing session"),

373+ expectedErr: nil,

349374 expectRefreshed: true,

350− expectValidated: false,

375+ expectValidated: true,

351376 }),

352377 Entry("when the session is not refreshed by the provider and validation fails", refreshSessionIfNeededTableInput{

353378 refreshPeriod: 1 * time.Minute,

@@ -366,10 +391,9 @@ var _ = Describe("Stored Session Suite", func() {

366391

367392 Context("refreshSession", func() {

368393 type refreshSessionWithProviderTableInput struct {

369− session *sessionsapi.SessionState

370− expectedErr error

371− expectRefreshed bool

372− expectSaved bool

394+ session *sessionsapi.SessionState

395+ expectedErr error

396+ expectSaved bool

373397 }

374398

375399 now := time.Now()

@@ -414,15 +438,15 @@ var _ = Describe("Stored Session Suite", func() {

414438 session: &sessionsapi.SessionState{

415439 RefreshToken: noRefresh,

416440 },

417− expectedErr: nil,

418− expectSaved: false,

441+ expectedErr: nil,

442+ expectSaved: false,

419443 }),

420444 Entry("when the provider refreshes the session", refreshSessionWithProviderTableInput{

421445 session: &sessionsapi.SessionState{

422446 RefreshToken: refresh,

423447 },

424− expectedErr: nil,

425− expectSaved: true,

448+ expectedErr: nil,

449+ expectSaved: true,

426450 }),

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

428452 session: &sessionsapi.SessionState{

@@ -430,16 +454,16 @@ var _ = Describe("Stored Session Suite", func() {

430454 CreatedAt: &now,

431455 ExpiresOn: &now,

432456 },

433− expectedErr: errors.New("error refreshing access token: error refreshing session"),

434− expectSaved: false,

457+ expectedErr: errors.New("error refreshing tokens: error refreshing session"),

458+ expectSaved: false,

435459 }),

436460 Entry("when the saving the session returns an error", refreshSessionWithProviderTableInput{

437461 session: &sessionsapi.SessionState{

438462 RefreshToken: refresh,

439463 AccessToken: "NoSave",

440464 },

441− expectedErr: errors.New("error saving session: unable to save session"),

442− expectSaved: true,

465+ expectedErr: errors.New("error saving session: unable to save session"),

466+ expectSaved: true,

443467 }),

444468 )

445469 })

providers/azure.go+7−5

@@ -242,21 +242,23 @@ func (p *AzureProvider) RefreshSession(ctx context.Context, s *sessions.SessionS

242242 return false, nil

243243 }

244244

245− origExpiration := s.ExpiresOn

246−

247245 err := p.redeemRefreshToken(ctx, s)

248246 if err != nil {

249247 return false, fmt.Errorf("unable to redeem refresh token: %v", err)

250248 }

251249

252− logger.Printf("refreshed id token %s (expired on %s)\n", s, origExpiration)

253250 return true, nil

254251}

255252

256253func (p *AzureProvider) redeemRefreshToken(ctx context.Context, s *sessions.SessionState) error {

254+ clientSecret, err := p.GetClientSecret()

255+ if err != nil {

256+ return err

257+ }

258+

257259 params := url.Values{}

258260 params.Add("client_id", p.ClientID)

259− params.Add("client_secret", p.ClientSecret)

261+ params.Add("client_secret", clientSecret)

260262 params.Add("refresh_token", s.RefreshToken)

261263 params.Add("grant_type", "refresh_token")

262264

@@ -267,7 +269,7 @@ func (p *AzureProvider) redeemRefreshToken(ctx context.Context, s *sessions.Sess

267269 IDToken string `json:"id_token"`

268270 }

269271

270− err := requests.New(p.RedeemURL.String()).

272+ err = requests.New(p.RedeemURL.String()).

271273 WithContext(ctx).

272274 WithMethod("POST").

273275 WithBody(bytes.NewBufferString(params.Encode())).

providers/azure_test.go+1−11

@@ -340,17 +340,7 @@ func TestAzureProviderProtectedResourceConfigured(t *testing.T) {

340340 assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test"))

341341}

342342

343−func TestAzureProviderNotRefreshWhenNotExpired(t *testing.T) {

344− p := testAzureProvider("")

345−

346− expires := time.Now().Add(time.Duration(1) * time.Hour)

347− session := &sessions.SessionState{AccessToken: "some_access_token", RefreshToken: "some_refresh_token", IDToken: "some_id_token", ExpiresOn: &expires}

348− refreshNeeded, err := p.RefreshSession(context.Background(), session)

349− assert.Equal(t, nil, err)

350− assert.False(t, refreshNeeded)

351−}

352−

353−func TestAzureProviderRefreshWhenExpired(t *testing.T) {

343+func TestAzureProviderRefresh(t *testing.T) {

354344 email := "foo@example.com"

355345 idToken := idTokenClaims{Email: email}

356346 idTokenString, err := newSignedTestIDToken(idToken)

providers/google.go+12−15

@@ -271,7 +271,7 @@ func (p *GoogleProvider) RefreshSession(ctx context.Context, s *sessions.Session

271271 return false, nil

272272 }

273273

274− newToken, newIDToken, ttl, err := p.redeemRefreshToken(ctx, s.RefreshToken)

274+ err := p.redeemRefreshToken(ctx, s)

275275 if err != nil {

276276 return false, err

277277 }

@@ -284,26 +284,20 @@ func (p *GoogleProvider) RefreshSession(ctx context.Context, s *sessions.Session

284284 return false, fmt.Errorf("%s is no longer in the group(s)", s.Email)

285285 }

286286

287− s.AccessToken = newToken

288− s.IDToken = newIDToken

289−

290− s.CreatedAtNow()

291− s.ExpiresIn(ttl)

292−

293287 return true, nil

294288}

295289

296−func (p *GoogleProvider) redeemRefreshToken(ctx context.Context, refreshToken string) (token string, idToken string, expires time.Duration, err error) {

290+func (p *GoogleProvider) redeemRefreshToken(ctx context.Context, s *sessions.SessionState) error {

297291 // https://developers.google.com/identity/protocols/OAuth2WebServer#refresh

298292 clientSecret, err := p.GetClientSecret()

299293 if err != nil {

300− return

294+ return err

301295 }

302296

303297 params := url.Values{}

304298 params.Add("client_id", p.ClientID)

305299 params.Add("client_secret", clientSecret)

306− params.Add("refresh_token", refreshToken)

300+ params.Add("refresh_token", s.RefreshToken)

307301 params.Add("grant_type", "refresh_token")

308302

309303 var data struct {

@@ -320,11 +314,14 @@ func (p *GoogleProvider) redeemRefreshToken(ctx context.Context, refreshToken st

320314 Do().

321315 UnmarshalInto(&data)

322316 if err != nil {

323− return "", "", 0, err

317+ return err

324318 }

325319

326− token = data.AccessToken

327− idToken = data.IDToken

328− expires = time.Duration(data.ExpiresIn) * time.Second

329− return

320+ s.AccessToken = data.AccessToken

321+ s.IDToken = data.IDToken

322+

323+ s.CreatedAtNow()

324+ s.ExpiresIn(time.Duration(data.ExpiresIn) * time.Second)

325+

326+ return nil

330327}

providers/oidc.go−1

@@ -154,7 +154,6 @@ func (p *OIDCProvider) RefreshSession(ctx context.Context, s *sessions.SessionSt

154154 return false, fmt.Errorf("unable to redeem refresh token: %v", err)

155155 }

156156

157− logger.Printf("refreshed session: %s", s)

158157 return true, nil

159158}

160159

providers/provider_default.go+6−1

@@ -135,8 +135,13 @@ func (p *ProviderData) RefreshSession(_ context.Context, s *sessions.SessionStat

135135 return false, nil

136136 }

137137

138− // Pretend `RefreshSession` occured so `ValidateSession` isn't called

138+ // HACK:

139+ // Pretend `RefreshSession` occurred so `ValidateSession` isn't called

139140 // 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.

140145 return true, nil

141146}

142147

providers/provider_default_test.go+14−6

@@ -14,12 +14,20 @@ import (

1414func TestRefresh(t *testing.T) {

1515 p := &ProviderData{}

1616

17− expires := time.Now().Add(time.Duration(-11) * time.Minute)

18− refreshed, err := p.RefreshSession(context.Background(), &sessions.SessionState{

19− ExpiresOn: &expires,

20− })

21− assert.Equal(t, false, refreshed)

22− assert.Equal(t, nil, err)

17+ now := time.Unix(1234567890, 10)

18+ expires := time.Unix(1234567890, 0)

19+

20+ ss := &sessions.SessionState{}

21+ ss.Clock.Set(now)

22+ ss.SetExpiresOn(expires)

23+

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

25+ assert.True(t, refreshed)

26+ assert.NoError(t, err)

27+

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

29+ assert.False(t, refreshed)

30+ assert.NoError(t, err)

2331}

2432

2533func TestAcrValuesNotConfigured(t *testing.T) {