2aa04c9 · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

Merge pull request #796 from grnhse/refactor-redeem-code

Refactor redeemCode and support a Provider-wide EnrichSessionState method

commit2aa04c97208612507d300f2a4145d1aecd775c60 authorJoel Speed <Joel.speed@hotmail.co.uk> dateWed 21 Oct 2020, 11:09 (6 years ago) parent8b44ddd 4a54c94

11 files changed+248−206

compare with master

unifiedsplit

CHANGELOG.md+1 oauthproxy.go+18−14 oauthproxy_test.go+75−3 pkg/validation/options.go−1 providers/github.go+31−22 providers/github_test.go+71−71 providers/gitlab.go+9−38 providers/gitlab_test.go+15−44 providers/provider_default.go+20−12 providers/provider_default_test.go+6 providers/providers.go+2−1

CHANGELOG.md+1

@@ -26,6 +26,7 @@

2626## Changes since v6.1.1

2727

2828- [#825](https://github.com/oauth2-proxy/oauth2-proxy/pull/825) Fix code coverage reporting on GitHub actions(@JoelSpeed)

29+- [#796](https://github.com/oauth2-proxy/oauth2-proxy/pull/796) Deprecate GetUserName & GetEmailAdress for EnrichSessionState (@NickMeves)

2930- [#705](https://github.com/oauth2-proxy/oauth2-proxy/pull/705) Add generic Header injectors for upstream request and response headers (@JoelSpeed)

3031- [#753](https://github.com/oauth2-proxy/oauth2-proxy/pull/753) Pass resource parameter in login url (@codablock)

3132- [#789](https://github.com/oauth2-proxy/oauth2-proxy/pull/789) Add `--skip-auth-route` configuration option for `METHOD=pathRegex` based allowlists (@NickMeves)

oauthproxy.go+18−14

@@ -357,22 +357,19 @@ func (p *OAuthProxy) redeemCode(ctx context.Context, host, code string) (*sessio

357357 if err != nil {

358358 return nil, err

359359 }

360+ return s, nil

361+}

360362

363+func (p *OAuthProxy) enrichSessionState(ctx context.Context, s *sessionsapi.SessionState) error {

364+ var err error

361365 if s.Email == "" {

362366 s.Email, err = p.provider.GetEmailAddress(ctx, s)

363− if err != nil && err.Error() != "not implemented" {

364− return nil, err

365− }

366− }

367−

368− if s.User == "" {

369− s.User, err = p.provider.GetUserName(ctx, s)

370− if err != nil && err.Error() != "not implemented" {

371− return nil, err

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

368+ return err

372369 }

373370 }

374371

375− return s, nil

372+ return p.provider.EnrichSessionState(ctx, s)

376373}

377374

378375// MakeCSRFCookie creates a cookie for CSRF

@@ -829,14 +826,21 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {

829826 return

830827 }

831828

832− s := strings.SplitN(req.Form.Get("state"), ":", 2)

833− if len(s) != 2 {

829+ err = p.enrichSessionState(req.Context(), session)

830+ if err != nil {

831+ logger.Errorf("Error creating session during OAuth2 callback: %v", err)

832+ p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", "Internal Error")

833+ return

834+ }

835+

836+ state := strings.SplitN(req.Form.Get("state"), ":", 2)

837+ if len(state) != 2 {

834838 logger.Error("Error while parsing OAuth2 state: invalid length")

835839 p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", "Invalid State")

836840 return

837841 }

838− nonce := s[0]

839− redirect := s[1]

842+ nonce := state[0]

843+ redirect := state[1]

840844 c, err := req.Cookie(p.CSRFCookieName)

841845 if err != nil {

842846 logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie")

oauthproxy_test.go+75−3

@@ -396,14 +396,86 @@ func NewTestProvider(providerURL *url.URL, emailAddress string) *TestProvider {

396396 }

397397}

398398

399−func (tp *TestProvider) GetEmailAddress(ctx context.Context, session *sessions.SessionState) (string, error) {

399+func (tp *TestProvider) GetEmailAddress(_ context.Context, _ *sessions.SessionState) (string, error) {

400400 return tp.EmailAddress, nil

401401}

402402

403−func (tp *TestProvider) ValidateSessionState(ctx context.Context, session *sessions.SessionState) bool {

403+func (tp *TestProvider) ValidateSessionState(_ context.Context, _ *sessions.SessionState) bool {

404404 return tp.ValidToken

405405}

406406

407+func Test_redeemCode(t *testing.T) {

408+ opts := baseTestOptions()

409+ err := validation.Validate(opts)

410+ assert.NoError(t, err)

411+

412+ proxy, err := NewOAuthProxy(opts, func(string) bool { return true })

413+ if err != nil {

414+ t.Fatal(err)

415+ }

416+

417+ _, err = proxy.redeemCode(context.Background(), "www.example.com", "")

418+ assert.Error(t, err)

419+}

420+

421+func Test_enrichSession(t *testing.T) {

422+ const (

423+ sessionUser = "Mr Session"

424+ sessionEmail = "session@example.com"

425+ providerEmail = "provider@example.com"

426+ )

427+

428+ testCases := map[string]struct {

429+ session *sessions.SessionState

430+ expectedUser string

431+ expectedEmail string

432+ }{

433+ "Session already has enrichable fields": {

434+ session: &sessions.SessionState{

435+ User: sessionUser,

436+ Email: sessionEmail,

437+ },

438+ expectedUser: sessionUser,

439+ expectedEmail: sessionEmail,

440+ },

441+ "Session is missing Email and GetEmailAddress is implemented": {

442+ session: &sessions.SessionState{

443+ User: sessionUser,

444+ },

445+ expectedUser: sessionUser,

446+ expectedEmail: providerEmail,

447+ },

448+ "Session is missing User and GetUserName is not implemented": {

449+ session: &sessions.SessionState{

450+ Email: sessionEmail,

451+ },

452+ expectedUser: "",

453+ expectedEmail: sessionEmail,

454+ },

455+ }

456+

457+ for name, tc := range testCases {

458+ t.Run(name, func(t *testing.T) {

459+ opts := baseTestOptions()

460+ err := validation.Validate(opts)

461+ assert.NoError(t, err)

462+

463+ // intentionally set after validation.Validate(opts) since it will clobber

464+ // our TestProvider and call `providers.New` defaulting to `providers.GoogleProvider`

465+ opts.SetProvider(NewTestProvider(&url.URL{Host: "www.example.com"}, providerEmail))

466+ proxy, err := NewOAuthProxy(opts, func(string) bool { return true })

467+ if err != nil {

468+ t.Fatal(err)

469+ }

470+

471+ err = proxy.enrichSessionState(context.Background(), tc.session)

472+ assert.NoError(t, err)

473+ assert.Equal(t, tc.expectedUser, tc.session.User)

474+ assert.Equal(t, tc.expectedEmail, tc.session.Email)

475+ })

476+ }

477+}

478+

407479func TestBasicAuthPassword(t *testing.T) {

408480 providerServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

409481 logger.Printf("%#v", r)

@@ -1883,7 +1955,7 @@ func TestClearSingleCookie(t *testing.T) {

18831955type NoOpKeySet struct {

18841956}

18851957

1886−func (NoOpKeySet) VerifySignature(ctx context.Context, jwt string) (payload []byte, err error) {

1958+func (NoOpKeySet) VerifySignature(_ context.Context, jwt string) (payload []byte, err error) {

18871959 splitStrings := strings.Split(jwt, ".")

18881960 payloadString := splitStrings[1]

18891961 return base64.RawURLEncoding.DecodeString(payloadString)

pkg/validation/options.go−1

@@ -273,7 +273,6 @@ func parseProviderInfo(o *options.Options, msgs []string) []string {

273273 case *providers.GitLabProvider:

274274 p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail

275275 p.Groups = o.GitLabGroup

276− p.EmailDomains = o.EmailDomains

277276

278277 if o.GetOIDCVerifier() != nil {

279278 p.Verifier = o.GetOIDCVerifier()

providers/github.go+31−22

@@ -102,6 +102,20 @@ func (p *GitHubProvider) SetUsers(users []string) {

102102 p.Users = users

103103}

104104

105+// EnrichSessionState updates the User & Email after the initial Redeem

106+func (p *GitHubProvider) EnrichSessionState(ctx context.Context, s *sessions.SessionState) error {

107+ err := p.getEmail(ctx, s)

108+ if err != nil {

109+ return err

110+ }

111+ return p.getUser(ctx, s)

112+}

113+

114+// ValidateSessionState validates the AccessToken

115+func (p *GitHubProvider) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {

116+ return validateToken(ctx, p, s.AccessToken, makeGitHubHeader(s.AccessToken))

117+}

118+

105119func (p *GitHubProvider) hasOrg(ctx context.Context, accessToken string) (bool, error) {

106120 // https://developer.github.com/v3/orgs/#list-your-organizations

107121

@@ -364,8 +378,8 @@ func (p *GitHubProvider) isCollaborator(ctx context.Context, username, accessTok

364378 return true, nil

365379}

366380

367−// GetEmailAddress returns the Account email address

368−func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {

381+// getEmail updates the SessionState Email

382+func (p *GitHubProvider) getEmail(ctx context.Context, s *sessions.SessionState) error {

369383

370384 var emails []struct {

371385 Email string `json:"email"`

@@ -379,11 +393,11 @@ func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.Sessio

379393 var err error

380394 verifiedUser, err = p.hasUser(ctx, s.AccessToken)

381395 if err != nil {

382− return "", err

396+ return err

383397 }

384398 // org and repository options are not configured

385399 if !verifiedUser && p.Org == "" && p.Repo == "" {

386− return "", errors.New("missing github user")

400+ return errors.New("missing github user")

387401 }

388402 }

389403 // If a user is verified by username options, skip the following restrictions

@@ -391,16 +405,16 @@ func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.Sessio

391405 if p.Org != "" {

392406 if p.Team != "" {

393407 if ok, err := p.hasOrgAndTeam(ctx, s.AccessToken); err != nil || !ok {

394− return "", err

408+ return err

395409 }

396410 } else {

397411 if ok, err := p.hasOrg(ctx, s.AccessToken); err != nil || !ok {

398− return "", err

412+ return err

399413 }

400414 }

401415 } else if p.Repo != "" && p.Token == "" { // If we have a token we'll do the collaborator check in GetUserName

402416 if ok, err := p.hasRepo(ctx, s.AccessToken); err != nil || !ok {

403− return "", err

417+ return err

404418 }

405419 }

406420 }

@@ -416,24 +430,23 @@ func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.Sessio

416430 Do().

417431 UnmarshalInto(&emails)

418432 if err != nil {

419− return "", err

433+ return err

420434 }

421435

422− returnEmail := ""

423436 for _, email := range emails {

424437 if email.Verified {

425− returnEmail = email.Email

426438 if email.Primary {

427− return returnEmail, nil

439+ s.Email = email.Email

440+ return nil

428441 }

429442 }

430443 }

431444

432− return returnEmail, nil

445+ return nil

433446}

434447

435−// GetUserName returns the Account user name

436−func (p *GitHubProvider) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {

448+// getUser updates the SessionState User

449+func (p *GitHubProvider) getUser(ctx context.Context, s *sessions.SessionState) error {

437450 var user struct {

438451 Login string `json:"login"`

439452 Email string `json:"email"`

@@ -451,22 +464,18 @@ func (p *GitHubProvider) GetUserName(ctx context.Context, s *sessions.SessionSta

451464 Do().

452465 UnmarshalInto(&user)

453466 if err != nil {

454− return "", err

467+ return err

455468 }

456469

457470 // Now that we have the username we can check collaborator status

458471 if !p.isVerifiedUser(user.Login) && p.Org == "" && p.Repo != "" && p.Token != "" {

459472 if ok, err := p.isCollaborator(ctx, user.Login, p.Token); err != nil || !ok {

460− return "", err

473+ return err

461474 }

462475 }

463476

464− return user.Login, nil

465−}

466−

467−// ValidateSessionState validates the AccessToken

468−func (p *GitHubProvider) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {

469− return validateToken(ctx, p, s.AccessToken, makeGitHubHeader(s.AccessToken))

477+ s.User = user.Login

478+ return nil

470479}

471480

472481// isVerifiedUser

providers/github_test.go+71−71

@@ -107,7 +107,7 @@ func TestGitHubProviderOverrides(t *testing.T) {

107107 assert.Equal(t, "profile", p.Data().Scope)

108108}

109109

110−func TestGitHubProviderGetEmailAddress(t *testing.T) {

110+func TestGitHubProvider_getEmail(t *testing.T) {

111111 b := testGitHubBackend(map[string][]string{

112112 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

113113 })

@@ -117,14 +117,14 @@ func TestGitHubProviderGetEmailAddress(t *testing.T) {

117117 p := testGitHubProvider(bURL.Host)

118118

119119 session := CreateAuthorizedSession()

120− email, err := p.GetEmailAddress(context.Background(), session)

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

122− assert.Equal(t, "michael.bland@gsa.gov", email)

120+ err := p.getEmail(context.Background(), session)

121+ assert.NoError(t, err)

122+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

123123}

124124

125−func TestGitHubProviderGetEmailAddressNotVerified(t *testing.T) {

125+func TestGitHubProvider_getEmailNotVerified(t *testing.T) {

126126 b := testGitHubBackend(map[string][]string{

127− "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

127+ "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": false, "primary": true} ]`},

128128 })

129129 defer b.Close()

130130

@@ -132,12 +132,12 @@ func TestGitHubProviderGetEmailAddressNotVerified(t *testing.T) {

132132 p := testGitHubProvider(bURL.Host)

133133

134134 session := CreateAuthorizedSession()

135− email, err := p.GetEmailAddress(context.Background(), session)

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

137− assert.Empty(t, "", email)

135+ err := p.getEmail(context.Background(), session)

136+ assert.NoError(t, err)

137+ assert.Empty(t, session.Email)

138138}

139139

140−func TestGitHubProviderGetEmailAddressWithOrg(t *testing.T) {

140+func TestGitHubProvider_getEmailWithOrg(t *testing.T) {

141141 b := testGitHubBackend(map[string][]string{

142142 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

143143 "/user/orgs": {

@@ -153,12 +153,12 @@ func TestGitHubProviderGetEmailAddressWithOrg(t *testing.T) {

153153 p.Org = "testorg1"

154154

155155 session := CreateAuthorizedSession()

156− email, err := p.GetEmailAddress(context.Background(), session)

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

158− assert.Equal(t, "michael.bland@gsa.gov", email)

156+ err := p.getEmail(context.Background(), session)

157+ assert.NoError(t, err)

158+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

159159}

160160

161−func TestGitHubProviderGetEmailAddressWithWriteAccessToPublicRepo(t *testing.T) {

161+func TestGitHubProvider_getEmailWithWriteAccessToPublicRepo(t *testing.T) {

162162 b := testGitHubBackend(map[string][]string{

163163 "/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": true}, "private": false}`},

164164 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

@@ -170,12 +170,12 @@ func TestGitHubProviderGetEmailAddressWithWriteAccessToPublicRepo(t *testing.T)

170170 p.SetRepo("oauth2-proxy/oauth2-proxy", "")

171171

172172 session := CreateAuthorizedSession()

173− email, err := p.GetEmailAddress(context.Background(), session)

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

175− assert.Equal(t, "michael.bland@gsa.gov", email)

173+ err := p.getEmail(context.Background(), session)

174+ assert.NoError(t, err)

175+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

176176}

177177

178−func TestGitHubProviderGetEmailAddressWithReadOnlyAccessToPrivateRepo(t *testing.T) {

178+func TestGitHubProvider_getEmailWithReadOnlyAccessToPrivateRepo(t *testing.T) {

179179 b := testGitHubBackend(map[string][]string{

180180 "/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": false}, "private": true}`},

181181 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

@@ -187,12 +187,12 @@ func TestGitHubProviderGetEmailAddressWithReadOnlyAccessToPrivateRepo(t *testing

187187 p.SetRepo("oauth2-proxy/oauth2-proxy", "")

188188

189189 session := CreateAuthorizedSession()

190− email, err := p.GetEmailAddress(context.Background(), session)

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

192− assert.Equal(t, "michael.bland@gsa.gov", email)

190+ err := p.getEmail(context.Background(), session)

191+ assert.NoError(t, err)

192+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

193193}

194194

195−func TestGitHubProviderGetEmailAddressWithWriteAccessToPrivateRepo(t *testing.T) {

195+func TestGitHubProvider_getEmailWithWriteAccessToPrivateRepo(t *testing.T) {

196196 b := testGitHubBackend(map[string][]string{

197197 "/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": true}, "private": true}`},

198198 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

@@ -204,14 +204,14 @@ func TestGitHubProviderGetEmailAddressWithWriteAccessToPrivateRepo(t *testing.T)

204204 p.SetRepo("oauth2-proxy/oauth2-proxy", "")

205205

206206 session := CreateAuthorizedSession()

207− email, err := p.GetEmailAddress(context.Background(), session)

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

209− assert.Equal(t, "michael.bland@gsa.gov", email)

207+ err := p.getEmail(context.Background(), session)

208+ assert.NoError(t, err)

209+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

210210}

211211

212−func TestGitHubProviderGetEmailAddressWithNoAccessToPrivateRepo(t *testing.T) {

212+func TestGitHubProvider_getEmailWithNoAccessToPrivateRepo(t *testing.T) {

213213 b := testGitHubBackend(map[string][]string{

214− "/repo/oauth2-proxy/oauth2-proxy": {},

214+ "/repo/oauth2-proxy/oauth2-proxy": {`{}`},

215215 })

216216 defer b.Close()

217217

@@ -220,12 +220,12 @@ func TestGitHubProviderGetEmailAddressWithNoAccessToPrivateRepo(t *testing.T) {

220220 p.SetRepo("oauth2-proxy/oauth2-proxy", "")

221221

222222 session := CreateAuthorizedSession()

223− email, err := p.GetEmailAddress(context.Background(), session)

224− assert.NotEqual(t, nil, err)

225− assert.Equal(t, "", email)

223+ err := p.getEmail(context.Background(), session)

224+ assert.NoError(t, err)

225+ assert.Empty(t, session.Email)

226226}

227227

228−func TestGitHubProviderGetEmailAddressWithToken(t *testing.T) {

228+func TestGitHubProvider_getEmailWithToken(t *testing.T) {

229229 b := testGitHubBackend(map[string][]string{

230230 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

231231 })

@@ -236,14 +236,14 @@ func TestGitHubProviderGetEmailAddressWithToken(t *testing.T) {

236236 p.SetRepo("oauth2-proxy/oauth2-proxy", "token")

237237

238238 session := CreateAuthorizedSession()

239− email, err := p.GetEmailAddress(context.Background(), session)

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

241− assert.Equal(t, "michael.bland@gsa.gov", email)

239+ err := p.getEmail(context.Background(), session)

240+ assert.NoError(t, err)

241+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

242242}

243243

244244// Note that trying to trigger the "failed building request" case is not

245245// practical, since the only way it can fail is if the URL fails to parse.

246−func TestGitHubProviderGetEmailAddressFailedRequest(t *testing.T) {

246+func TestGitHubProvider_getEmailFailedRequest(t *testing.T) {

247247 b := testGitHubBackend(map[string][]string{})

248248 defer b.Close()

249249

@@ -254,12 +254,12 @@ func TestGitHubProviderGetEmailAddressFailedRequest(t *testing.T) {

254254 // token. Alternatively, we could allow the parsing of the payload as

255255 // JSON to fail.

256256 session := &sessions.SessionState{AccessToken: "unexpected_access_token"}

257− email, err := p.GetEmailAddress(context.Background(), session)

258− assert.NotEqual(t, nil, err)

259− assert.Equal(t, "", email)

257+ err := p.getEmail(context.Background(), session)

258+ assert.Error(t, err)

259+ assert.Empty(t, session.Email)

260260}

261261

262−func TestGitHubProviderGetEmailAddressEmailNotPresentInPayload(t *testing.T) {

262+func TestGitHubProvider_getEmailNotPresentInPayload(t *testing.T) {

263263 b := testGitHubBackend(map[string][]string{

264264 "/user/emails": {`{"foo": "bar"}`},

265265 })

@@ -269,12 +269,12 @@ func TestGitHubProviderGetEmailAddressEmailNotPresentInPayload(t *testing.T) {

269269 p := testGitHubProvider(bURL.Host)

270270

271271 session := CreateAuthorizedSession()

272− email, err := p.GetEmailAddress(context.Background(), session)

273− assert.NotEqual(t, nil, err)

274− assert.Equal(t, "", email)

272+ err := p.getEmail(context.Background(), session)

273+ assert.Error(t, err)

274+ assert.Empty(t, session.Email)

275275}

276276

277−func TestGitHubProviderGetUserName(t *testing.T) {

277+func TestGitHubProvider_getUser(t *testing.T) {

278278 b := testGitHubBackend(map[string][]string{

279279 "/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},

280280 })

@@ -284,12 +284,12 @@ func TestGitHubProviderGetUserName(t *testing.T) {

284284 p := testGitHubProvider(bURL.Host)

285285

286286 session := CreateAuthorizedSession()

287− email, err := p.GetUserName(context.Background(), session)

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

289− assert.Equal(t, "mbland", email)

287+ err := p.getUser(context.Background(), session)

288+ assert.NoError(t, err)

289+ assert.Equal(t, "mbland", session.User)

290290}

291291

292−func TestGitHubProviderGetUserNameWithRepoAndToken(t *testing.T) {

292+func TestGitHubProvider_getUserWithRepoAndToken(t *testing.T) {

293293 b := testGitHubBackend(map[string][]string{

294294 "/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},

295295 "/repos/oauth2-proxy/oauth2-proxy/collaborators/mbland": {""},

@@ -301,12 +301,12 @@ func TestGitHubProviderGetUserNameWithRepoAndToken(t *testing.T) {

301301 p.SetRepo("oauth2-proxy/oauth2-proxy", "token")

302302

303303 session := CreateAuthorizedSession()

304− email, err := p.GetUserName(context.Background(), session)

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

306− assert.Equal(t, "mbland", email)

304+ err := p.getUser(context.Background(), session)

305+ assert.NoError(t, err)

306+ assert.Equal(t, "mbland", session.User)

307307}

308308

309−func TestGitHubProviderGetUserNameWithRepoAndTokenWithoutPushAccess(t *testing.T) {

309+func TestGitHubProvider_getUserWithRepoAndTokenWithoutPushAccess(t *testing.T) {

310310 b := testGitHubBackend(map[string][]string{})

311311 defer b.Close()

312312

@@ -315,12 +315,12 @@ func TestGitHubProviderGetUserNameWithRepoAndTokenWithoutPushAccess(t *testing.T

315315 p.SetRepo("oauth2-proxy/oauth2-proxy", "token")

316316

317317 session := CreateAuthorizedSession()

318− email, err := p.GetUserName(context.Background(), session)

319− assert.NotEqual(t, nil, err)

320− assert.Equal(t, "", email)

318+ err := p.getUser(context.Background(), session)

319+ assert.Error(t, err)

320+ assert.Empty(t, session.User)

321321}

322322

323−func TestGitHubProviderGetEmailAddressWithUsername(t *testing.T) {

323+func TestGitHubProvider_getEmailWithUsername(t *testing.T) {

324324 b := testGitHubBackend(map[string][]string{

325325 "/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},

326326 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

@@ -332,12 +332,12 @@ func TestGitHubProviderGetEmailAddressWithUsername(t *testing.T) {

332332 p.SetUsers([]string{"mbland", "octocat"})

333333

334334 session := CreateAuthorizedSession()

335− email, err := p.GetEmailAddress(context.Background(), session)

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

337− assert.Equal(t, "michael.bland@gsa.gov", email)

335+ err := p.getEmail(context.Background(), session)

336+ assert.NoError(t, err)

337+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

338338}

339339

340−func TestGitHubProviderGetEmailAddressWithNotAllowedUsername(t *testing.T) {

340+func TestGitHubProvider_getEmailWithNotAllowedUsername(t *testing.T) {

341341 b := testGitHubBackend(map[string][]string{

342342 "/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},

343343 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

@@ -349,12 +349,12 @@ func TestGitHubProviderGetEmailAddressWithNotAllowedUsername(t *testing.T) {

349349 p.SetUsers([]string{"octocat"})

350350

351351 session := CreateAuthorizedSession()

352− email, err := p.GetEmailAddress(context.Background(), session)

353− assert.NotEqual(t, nil, err)

354− assert.Equal(t, "", email)

352+ err := p.getEmail(context.Background(), session)

353+ assert.Error(t, err)

354+ assert.Empty(t, session.Email)

355355}

356356

357−func TestGitHubProviderGetEmailAddressWithUsernameAndNotBelongToOrg(t *testing.T) {

357+func TestGitHubProvider_getEmailWithUsernameAndNotBelongToOrg(t *testing.T) {

358358 b := testGitHubBackend(map[string][]string{

359359 "/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},

360360 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

@@ -371,16 +371,16 @@ func TestGitHubProviderGetEmailAddressWithUsernameAndNotBelongToOrg(t *testing.T

371371 p.SetUsers([]string{"mbland"})

372372

373373 session := CreateAuthorizedSession()

374− email, err := p.GetEmailAddress(context.Background(), session)

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

376− assert.Equal(t, "michael.bland@gsa.gov", email)

374+ err := p.getEmail(context.Background(), session)

375+ assert.NoError(t, err)

376+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

377377}

378378

379−func TestGitHubProviderGetEmailAddressWithUsernameAndNoAccessToPrivateRepo(t *testing.T) {

379+func TestGitHubProvider_getEmailWithUsernameAndNoAccessToPrivateRepo(t *testing.T) {

380380 b := testGitHubBackend(map[string][]string{

381381 "/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},

382382 "/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},

383− "/repo/oauth2-proxy/oauth2-proxy": {},

383+ "/repo/oauth2-proxy/oauth2-proxy": {`{}`},

384384 })

385385 defer b.Close()

386386

@@ -390,7 +390,7 @@ func TestGitHubProviderGetEmailAddressWithUsernameAndNoAccessToPrivateRepo(t *te

390390 p.SetUsers([]string{"mbland"})

391391

392392 session := CreateAuthorizedSession()

393− email, err := p.GetEmailAddress(context.Background(), session)

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

395− assert.Equal(t, "michael.bland@gsa.gov", email)

393+ err := p.getEmail(context.Background(), session)

394+ assert.NoError(t, err)

395+ assert.Equal(t, "michael.bland@gsa.gov", session.Email)

396396}

providers/gitlab.go+9−38

@@ -3,7 +3,6 @@ package providers

33import (

44 "context"

55 "fmt"

6− "strings"

76 "time"

98 oidc "github.com/coreos/go-oidc"

@@ -16,9 +15,7 @@ import (

1615type GitLabProvider struct {

1716 *ProviderData

1817

19− Groups []string

20− EmailDomains []string

21−

18+ Groups []string

2219 Verifier *oidc.IDTokenVerifier

2320 AllowUnverifiedEmail bool

2421}

@@ -168,20 +165,6 @@ func (p *GitLabProvider) verifyGroupMembership(userInfo *gitlabUserInfo) error {

168165 return fmt.Errorf("user is not a member of '%s'", p.Groups)

169166}

170167

171−func (p *GitLabProvider) verifyEmailDomain(userInfo *gitlabUserInfo) error {

172− if len(p.EmailDomains) == 0 || p.EmailDomains[0] == "*" {

173− return nil

174− }

175−

176− for _, domain := range p.EmailDomains {

177− if strings.HasSuffix(userInfo.Email, domain) {

178− return nil

179− }

180− }

181−

182− return fmt.Errorf("user email is not one of the valid domains '%v'", p.EmailDomains)

183−}

184−

185168func (p *GitLabProvider) createSessionState(ctx context.Context, token *oauth2.Token) (*sessions.SessionState, error) {

186169 rawIDToken, ok := token.Extra("id_token").(string)

187170 if !ok {

@@ -211,39 +194,27 @@ func (p *GitLabProvider) ValidateSessionState(ctx context.Context, s *sessions.S

211194}

212195

213196// GetEmailAddress returns the Account email address

214−func (p *GitLabProvider) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {

197+func (p *GitLabProvider) EnrichSessionState(ctx context.Context, s *sessions.SessionState) error {

215198 // Retrieve user info

216199 userInfo, err := p.getUserInfo(ctx, s)

217200 if err != nil {

218− return "", fmt.Errorf("failed to retrieve user info: %v", err)

201+ return fmt.Errorf("failed to retrieve user info: %v", err)

219202 }

220203

221204 // Check if email is verified

222205 if !p.AllowUnverifiedEmail && !userInfo.EmailVerified {

223− return "", fmt.Errorf("user email is not verified")

224− }

225−

226− // Check if email has valid domain

227− err = p.verifyEmailDomain(userInfo)

228− if err != nil {

229− return "", fmt.Errorf("email domain check failed: %v", err)

206+ return fmt.Errorf("user email is not verified")

230207 }

231208

232209 // Check group membership

210+ // TODO (@NickMeves) - Refactor to Authorize

233211 err = p.verifyGroupMembership(userInfo)

234212 if err != nil {

235− return "", fmt.Errorf("group membership check failed: %v", err)

213+ return fmt.Errorf("group membership check failed: %v", err)

236214 }

237215

238− return userInfo.Email, nil

239−}

240−

241−// GetUserName returns the Account user name

242−func (p *GitLabProvider) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {

243− userInfo, err := p.getUserInfo(ctx, s)

244− if err != nil {

245− return "", fmt.Errorf("failed to retrieve user info: %v", err)

246− }

216+ s.User = userInfo.Username

217+ s.Email = userInfo.Email

247218

248− return userInfo.Username, nil

219+ return nil

249220}

providers/gitlab_test.go+15−44

@@ -64,8 +64,8 @@ func TestGitLabProviderBadToken(t *testing.T) {

6464 p := testGitLabProvider(bURL.Host)

6565

6666 session := &sessions.SessionState{AccessToken: "unexpected_gitlab_access_token"}

67− _, err := p.GetEmailAddress(context.Background(), session)

68− assert.NotEqual(t, nil, err)

67+ err := p.EnrichSessionState(context.Background(), session)

68+ assert.Error(t, err)

6969}

7070

7171func TestGitLabProviderUnverifiedEmailDenied(t *testing.T) {

@@ -76,8 +76,8 @@ func TestGitLabProviderUnverifiedEmailDenied(t *testing.T) {

7676 p := testGitLabProvider(bURL.Host)

7777

7878 session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

79− _, err := p.GetEmailAddress(context.Background(), session)

80− assert.NotEqual(t, nil, err)

79+ err := p.EnrichSessionState(context.Background(), session)

80+ assert.Error(t, err)

8181}

8282

8383func TestGitLabProviderUnverifiedEmailAllowed(t *testing.T) {

@@ -89,9 +89,9 @@ func TestGitLabProviderUnverifiedEmailAllowed(t *testing.T) {

8989 p.AllowUnverifiedEmail = true

9090

9191 session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

92− email, err := p.GetEmailAddress(context.Background(), session)

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

94− assert.Equal(t, "foo@bar.com", email)

92+ err := p.EnrichSessionState(context.Background(), session)

93+ assert.NoError(t, err)

94+ assert.Equal(t, "foo@bar.com", session.Email)

9595}

9696

9797func TestGitLabProviderUsername(t *testing.T) {

@@ -103,9 +103,9 @@ func TestGitLabProviderUsername(t *testing.T) {

103103 p.AllowUnverifiedEmail = true

104104

105105 session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

106− username, err := p.GetUserName(context.Background(), session)

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

108− assert.Equal(t, "FooBar", username)

106+ err := p.EnrichSessionState(context.Background(), session)

107+ assert.NoError(t, err)

108+ assert.Equal(t, "FooBar", session.User)

109109}

110110

111111func TestGitLabProviderGroupMembershipValid(t *testing.T) {

@@ -118,9 +118,9 @@ func TestGitLabProviderGroupMembershipValid(t *testing.T) {

118118 p.Groups = []string{"foo"}

119119

120120 session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

121− email, err := p.GetEmailAddress(context.Background(), session)

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

123− assert.Equal(t, "foo@bar.com", email)

121+ err := p.EnrichSessionState(context.Background(), session)

122+ assert.NoError(t, err)

123+ assert.Equal(t, "FooBar", session.User)

124124}

125125

126126func TestGitLabProviderGroupMembershipMissing(t *testing.T) {

@@ -133,35 +133,6 @@ func TestGitLabProviderGroupMembershipMissing(t *testing.T) {

133133 p.Groups = []string{"baz"}

134134

135135 session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

136− _, err := p.GetEmailAddress(context.Background(), session)

137− assert.NotEqual(t, nil, err)

138−}

139−

140−func TestGitLabProviderEmailDomainValid(t *testing.T) {

141− b := testGitLabBackend()

142− defer b.Close()

143−

144− bURL, _ := url.Parse(b.URL)

145− p := testGitLabProvider(bURL.Host)

146− p.AllowUnverifiedEmail = true

147− p.EmailDomains = []string{"bar.com"}

148−

149− session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

150− email, err := p.GetEmailAddress(context.Background(), session)

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

152− assert.Equal(t, "foo@bar.com", email)

153−}

154−

155−func TestGitLabProviderEmailDomainInvalid(t *testing.T) {

156− b := testGitLabBackend()

157− defer b.Close()

158−

159− bURL, _ := url.Parse(b.URL)

160− p := testGitLabProvider(bURL.Host)

161− p.AllowUnverifiedEmail = true

162− p.EmailDomains = []string{"baz.com"}

163−

164− session := &sessions.SessionState{AccessToken: "gitlab_access_token"}

165− _, err := p.GetEmailAddress(context.Background(), session)

166− assert.NotEqual(t, nil, err)

136+ err := p.EnrichSessionState(context.Background(), session)

137+ assert.Error(t, err)

167138}

providers/provider_default.go+20−12

@@ -14,7 +14,13 @@ import (

1414 "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests"

1515)

1616

17−var _ Provider = (*ProviderData)(nil)

17+var (

18+ // ErrNotImplemented is returned when a provider did not override a default

19+ // implementation method that doesn't have sensible defaults

20+ ErrNotImplemented = errors.New("not implemented")

21+

22+ _ Provider = (*ProviderData)(nil)

23+)

1824

1925// Redeem provides a default implementation of the OAuth2 token redemption process

2026func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (s *sessions.SessionState, err error) {

@@ -81,21 +87,23 @@ func (p *ProviderData) GetLoginURL(redirectURI, state string) string {

8187}

8288

8389// GetEmailAddress returns the Account email address

84−func (p *ProviderData) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {

85− return "", errors.New("not implemented")

86−}

87−

88−// GetUserName returns the Account username

89−func (p *ProviderData) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {

90− return "", errors.New("not implemented")

90+// DEPRECATED: Migrate to EnrichSessionState

91+func (p *ProviderData) GetEmailAddress(_ context.Context, _ *sessions.SessionState) (string, error) {

92+ return "", ErrNotImplemented

9193}

9294

9395// ValidateGroup validates that the provided email exists in the configured provider

9496// email group(s).

95−func (p *ProviderData) ValidateGroup(email string) bool {

97+func (p *ProviderData) ValidateGroup(_ string) bool {

9698 return true

9799}

98100

101+// EnrichSessionState is called after Redeem to allow providers to enrich session fields

102+// such as User, Email, Groups with provider specific API calls.

103+func (p *ProviderData) EnrichSessionState(_ context.Context, _ *sessions.SessionState) error {

104+ return nil

105+}

106+

99107// ValidateSessionState validates the AccessToken

100108func (p *ProviderData) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {

101109 return validateToken(ctx, p, s.AccessToken, nil)

@@ -103,12 +111,12 @@ func (p *ProviderData) ValidateSessionState(ctx context.Context, s *sessions.Ses

103111

104112// RefreshSessionIfNeeded should refresh the user's session if required and

105113// do nothing if a refresh is not required

106−func (p *ProviderData) RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error) {

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

107115 return false, nil

108116}

109117

110118// CreateSessionStateFromBearerToken should be implemented to allow providers

111119// to convert ID tokens into sessions

112−func (p *ProviderData) CreateSessionStateFromBearerToken(ctx context.Context, rawIDToken string, idToken *oidc.IDToken) (*sessions.SessionState, error) {

113− return nil, errors.New("not implemented")

120+func (p *ProviderData) CreateSessionStateFromBearerToken(_ context.Context, _ string, _ *oidc.IDToken) (*sessions.SessionState, error) {

121+ return nil, ErrNotImplemented

114122}

providers/provider_default_test.go+6

@@ -47,3 +47,9 @@ func TestAcrValuesConfigured(t *testing.T) {

4747 result := p.GetLoginURL("https://my.test.app/oauth", "")

4848 assert.Contains(t, result, "acr_values=testValue")

4949}

50+

51+func TestEnrichSessionState(t *testing.T) {

52+ p := &ProviderData{}

53+ s := &sessions.SessionState{}

54+ assert.NoError(t, p.EnrichSessionState(context.Background(), s))

55+}

providers/providers.go+2−1

@@ -10,10 +10,11 @@ import (

1010// Provider represents an upstream identity provider implementation

1111type Provider interface {

1212 Data() *ProviderData

13+ // DEPRECATED: Migrate to EnrichSessionState

1314 GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error)

14− GetUserName(ctx context.Context, s *sessions.SessionState) (string, error)

1515 Redeem(ctx context.Context, redirectURI, code string) (*sessions.SessionState, error)

1616 ValidateGroup(string) bool

17+ EnrichSessionState(ctx context.Context, s *sessions.SessionState) error

1718 ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool

1819 GetLoginURL(redirectURI, finalRedirect string) string

1920 RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error)