Allow group authZ in AuthOnly endpoint via Querystring

didinahmadi · Dec 24, 2020 · 23b2355 · 23b2355
1 parent 8bd2409
commit 23b2355
Showing 3 changed files with 147 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) `--user-id-claim` option is deprecated and replaced by `--oidc-email-claim`
 - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Gitlab projects needs a Gitlab application with the extra `read_api` enabled
+- [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) `/oauth2/auth` `allowed_groups` querystring parameter can be paired with the `allowed-groups` configuration option.
+  - In this scenario, the user's group must be in both lists to not get a 401 response code.
+  - The `allowed_group` querystring parameter can be specified multiple times to support multiple groups.
+  - The `allowed_groups` querystring parameter can specify multiple comma delimited groups.
 - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Existing sessions from v6.0.0 or earlier are no longer valid. They will trigger a reauthentication.
 - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) `skip-auth-strip-headers` now applies to all requests, not just those where authentication would be skipped.
 - [#797](https://github.com/oauth2-proxy/oauth2-proxy/pull/797) The behavior of the Google provider Groups restriction changes with this
@@ -56,6 +60,7 @@
 - [#918](https://github.com/oauth2-proxy/oauth2-proxy/pull/918) Fix log header output (@JoelSpeed)
 - [#911](https://github.com/oauth2-proxy/oauth2-proxy/pull/911) Validate provider type on startup.
 - [#869](https://github.com/oauth2-proxy/oauth2-proxy/pull/869) Streamline provider interface method names and signatures (@NickMeves)
+- [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) Support group authorization on `oauth2/auth` endpoint via `allowed_group` & `allowed_groups` querystring parameters (@NickMeves)
 - [#906](https://github.com/oauth2-proxy/oauth2-proxy/pull/906) Set up v6.1.x versioned documentation as default documentation (@JoelSpeed)
 - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Remove v5 legacy sessions support (@NickMeves)
 - [#904](https://github.com/oauth2-proxy/oauth2-proxy/pull/904) Set `skip-auth-strip-headers` to `true` by default (@NickMeves)

diff --git a/oauthproxy.go b/oauthproxy.go
@@ -744,7 +744,7 @@ func (p *OAuthProxy) serveHTTP(rw http.ResponseWriter, req *http.Request) {
 	case path == p.OAuthCallbackPath:
 		p.OAuthCallback(rw, req)
 	case path == p.AuthOnlyPath:
-		p.AuthenticateOnly(rw, req)
+		p.AuthOnly(rw, req)
 	case path == p.UserInfoPath:
 		p.UserInfo(rw, req)
 	default:
@@ -925,14 +925,22 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 	}
 }
 
-// AuthenticateOnly checks whether the user is currently logged in
-func (p *OAuthProxy) AuthenticateOnly(rw http.ResponseWriter, req *http.Request) {
+// AuthOnly checks whether the user is currently logged in (both authentication
+// and optional authorization via `allowed_groups` querystring).
+func (p *OAuthProxy) AuthOnly(rw http.ResponseWriter, req *http.Request) {
 	session, err := p.getAuthenticatedSession(rw, req)
 	if err != nil {
 		http.Error(rw, "unauthorized request", http.StatusUnauthorized)
 		return
 	}
 
+	// Allow secondary group restrictions based on the `allowed_group` or
+	// `allowed_groups` querystring parameter
+	if !checkAllowedGroups(req, session) {
+		http.Error(rw, "unauthorized request", http.StatusUnauthorized)
+		return
+	}
+
 	// we are authenticated
 	p.addHeadersForProxying(rw, req, session)
 	p.headersChain.Then(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
@@ -1016,6 +1024,42 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R
 	return session, nil
 }
 
+func checkAllowedGroups(req *http.Request, session *sessionsapi.SessionState) bool {
+	allowedGroups := extractAllowedGroups(req)
+	if len(allowedGroups) == 0 {
+		return true
+	}
+
+	for _, group := range session.Groups {
+		if _, ok := allowedGroups[group]; ok {
+			return true
+		}
+	}
+
+	return false
+}
+
+func extractAllowedGroups(req *http.Request) map[string]struct{} {
+	groups := map[string]struct{}{}
+	query := req.URL.Query()
+
+	// multi-key singular support
+	if multiGroups, ok := query["allowed_group"]; ok {
+		for _, group := range multiGroups {
+			groups[group] = struct{}{}
+		}
+	}
+
+	// single key plural comma delimited support
+	for _, group := range strings.Split(query.Get("allowed_groups"), ",") {
+		if group != "" {
+			groups[group] = struct{}{}
+		}
+	}
+
+	return groups
+}
+
 // addHeadersForProxying adds the appropriate headers the request / response for proxying
 func (p *OAuthProxy) addHeadersForProxying(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) {
 	if session.Email == "" {

diff --git a/oauthproxy_test.go b/oauthproxy_test.go
@@ -1197,18 +1197,20 @@ func TestUserInfoEndpointUnauthorizedOnNoCookieSetError(t *testing.T) {
 	assert.Equal(t, http.StatusUnauthorized, test.rw.Code)
 }
 
-func NewAuthOnlyEndpointTest(modifiers ...OptionsModifier) (*ProcessCookieTest, error) {
+func NewAuthOnlyEndpointTest(querystring string, modifiers ...OptionsModifier) (*ProcessCookieTest, error) {
 	pcTest, err := NewProcessCookieTestWithOptionsModifiers(modifiers...)
 	if err != nil {
 		return nil, err
 	}
-	pcTest.req, _ = http.NewRequest("GET",
-		pcTest.opts.ProxyPrefix+"/auth", nil)
+	pcTest.req, _ = http.NewRequest(
+		"GET",
+		fmt.Sprintf("%s/auth%s", pcTest.opts.ProxyPrefix, querystring),
+		nil)
 	return pcTest, nil
 }
 
 func TestAuthOnlyEndpointAccepted(t *testing.T) {
-	test, err := NewAuthOnlyEndpointTest()
+	test, err := NewAuthOnlyEndpointTest("")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1226,7 +1228,7 @@ func TestAuthOnlyEndpointAccepted(t *testing.T) {
 }
 
 func TestAuthOnlyEndpointUnauthorizedOnNoCookieSetError(t *testing.T) {
-	test, err := NewAuthOnlyEndpointTest()
+	test, err := NewAuthOnlyEndpointTest("")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1238,7 +1240,7 @@ func TestAuthOnlyEndpointUnauthorizedOnNoCookieSetError(t *testing.T) {
 }
 
 func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) {
-	test, err := NewAuthOnlyEndpointTest(func(opts *options.Options) {
+	test, err := NewAuthOnlyEndpointTest("", func(opts *options.Options) {
 		opts.Cookie.Expire = time.Duration(24) * time.Hour
 	})
 	if err != nil {
@@ -1258,7 +1260,7 @@ func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) {
 }
 
 func TestAuthOnlyEndpointUnauthorizedOnEmailValidationFailure(t *testing.T) {
-	test, err := NewAuthOnlyEndpointTest()
+	test, err := NewAuthOnlyEndpointTest("")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1960,7 +1962,7 @@ func TestGetJwtSession(t *testing.T) {
 	verifier := oidc.NewVerifier("https://issuer.example.com", keyset,
 		&oidc.Config{ClientID: "https://test.myapp.com", SkipExpiryCheck: true})
 
-	test, err := NewAuthOnlyEndpointTest(func(opts *options.Options) {
+	test, err := NewAuthOnlyEndpointTest("", func(opts *options.Options) {
 		opts.InjectRequestHeaders = []options.Header{
 			{
 				Name: "Authorization",
@@ -2028,7 +2030,6 @@ func TestGetJwtSession(t *testing.T) {
 				},
 			},
 		}
-
 		opts.SkipJwtBearerTokens = true
 		opts.SetJWTBearerVerifiers(append(opts.GetJWTBearerVerifiers(), verifier))
 	})
@@ -2692,32 +2693,106 @@ func TestProxyAllowedGroups(t *testing.T) {
 }
 
 func TestAuthOnlyAllowedGroups(t *testing.T) {
-	tests := []struct {
+	testCases := []struct {
 		name               string
 		allowedGroups      []string
 		groups             []string
+		querystring        string
 		expectUnauthorized bool
 	}{
-		{"NoAllowedGroups", []string{}, []string{}, false},
-		{"NoAllowedGroupsUserHasGroups", []string{}, []string{"a", "b"}, false},
-		{"UserInAllowedGroup", []string{"a"}, []string{"a", "b"}, false},
-		{"UserNotInAllowedGroup", []string{"a"}, []string{"c"}, true},
+		{
+			name:               "NoAllowedGroups",
+			allowedGroups:      []string{},
+			groups:             []string{},
+			querystring:        "",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "NoAllowedGroupsUserHasGroups",
+			allowedGroups:      []string{},
+			groups:             []string{"a", "b"},
+			querystring:        "",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "UserInAllowedGroup",
+			allowedGroups:      []string{"a"},
+			groups:             []string{"a", "b"},
+			querystring:        "",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "UserNotInAllowedGroup",
+			allowedGroups:      []string{"a"},
+			groups:             []string{"c"},
+			querystring:        "",
+			expectUnauthorized: true,
+		},
+		{
+			name:               "UserInQuerystringGroup",
+			allowedGroups:      []string{"a", "b"},
+			groups:             []string{"a", "c"},
+			querystring:        "?allowed_group=a",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "UserInOnlyQuerystringGroup",
+			allowedGroups:      []string{},
+			groups:             []string{"a", "c"},
+			querystring:        "?allowed_groups=a,b",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "UserInMultiParamQuerystringGroup",
+			allowedGroups:      []string{"a", "b"},
+			groups:             []string{"b"},
+			querystring:        "?allowed_group=a&allowed_group=b",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "UserInDelimitedQuerystringGroup",
+			allowedGroups:      []string{"a", "b", "c"},
+			groups:             []string{"c"},
+			querystring:        "?allowed_groups=a,c",
+			expectUnauthorized: false,
+		},
+		{
+			name:               "UserNotInQuerystringGroup",
+			allowedGroups:      []string{},
+			groups:             []string{"c"},
+			querystring:        "?allowed_group=a&allowed_group=b",
+			expectUnauthorized: true,
+		},
+		{
+			name:               "UserInConfigGroupNotInQuerystringGroup",
+			allowedGroups:      []string{"a", "b", "c"},
+			groups:             []string{"c"},
+			querystring:        "?allowed_group=a&allowed_group=b",
+			expectUnauthorized: true,
+		},
+		{
+			name:               "UserInQuerystringGroupNotInConfigGroup",
+			allowedGroups:      []string{"a", "b"},
+			groups:             []string{"c"},
+			querystring:        "?allowed_groups=b,c",
+			expectUnauthorized: true,
+		},
 	}
 
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			emailAddress := "test"
 			created := time.Now()
 
 			session := &sessions.SessionState{
-				Groups:      tt.groups,
+				Groups:      tc.groups,
 				Email:       emailAddress,
 				AccessToken: "oauth_token",
 				CreatedAt:   &created,
 			}
 
-			test, err := NewAuthOnlyEndpointTest(func(opts *options.Options) {
-				opts.AllowedGroups = tt.allowedGroups
+			test, err := NewAuthOnlyEndpointTest(tc.querystring, func(opts *options.Options) {
+				opts.AllowedGroups = tc.allowedGroups
 			})
 			if err != nil {
 				t.Fatal(err)
@@ -2728,7 +2803,7 @@ func TestAuthOnlyAllowedGroups(t *testing.T) {
 
 			test.proxy.ServeHTTP(test.rw, test.req)
 
-			if tt.expectUnauthorized {
+			if tc.expectUnauthorized {
 				assert.Equal(t, http.StatusUnauthorized, test.rw.Code)
 			} else {
 				assert.Equal(t, http.StatusAccepted, test.rw.Code)