Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit db550f9

Browse files
authored
auth: Remove redirects to session package (#64260)
These functions are not required to be called outside of frontend, so there's no need to reexport them. Instead, we consolidate the signout cookie logic in the session package. Test plan: Just moved some code around, go compiler doesn't complain.
1 parent 384959a commit db550f9

File tree

13 files changed

+24
-44
lines changed

13 files changed

+24
-44
lines changed

cmd/frontend/auth/BUILD.bazel

-2
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ go_library(
88
"non_public.go",
99
"redirect.go",
1010
"reset_password.go",
11-
"sign_out_cookie.go",
1211
"user.go",
1312
],
1413
importpath = "github.com/sourcegraph/sourcegraph/cmd/frontend/auth",
@@ -17,7 +16,6 @@ go_library(
1716
"//cmd/frontend/backend",
1817
"//cmd/frontend/internal/app/router",
1918
"//cmd/frontend/internal/app/ui/router",
20-
"//cmd/frontend/internal/auth/session",
2119
"//cmd/frontend/internal/auth/userpasswd",
2220
"//internal/actor",
2321
"//internal/auth",

cmd/frontend/auth/sign_out_cookie.go

-24
This file was deleted.

cmd/frontend/internal/app/BUILD.bazel

-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ go_library(
2020
importpath = "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/app",
2121
visibility = ["//cmd/frontend:__subpackages__"],
2222
deps = [
23-
"//cmd/frontend/auth",
2423
"//cmd/frontend/backend",
2524
"//cmd/frontend/internal/app/assetsutil",
2625
"//cmd/frontend/internal/app/debugproxies",

cmd/frontend/internal/app/sign_out.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77

88
"github.com/sourcegraph/log"
99

10-
"github.com/sourcegraph/sourcegraph/cmd/frontend/auth"
1110
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/openidconnect"
1211
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/saml"
1312
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/session"
@@ -49,7 +48,7 @@ func serveSignOutHandler(logger log.Logger, db database.DB) http.HandlerFunc {
4948
logger.Error("serveSignOutHandler", log.Error(err))
5049
}
5150

52-
auth.SetSignOutCookie(w)
51+
session.SetSignOutCookie(w)
5352

5453
ssoSignOutHandler(w, r)
5554

cmd/frontend/internal/auth/bitbucketcloudoauth/middleware_test.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616

1717
"github.com/sourcegraph/log/logtest"
1818

19-
"github.com/sourcegraph/sourcegraph/cmd/frontend/auth"
2019
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/oauth"
2120
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/providers"
2221
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/session"
@@ -64,7 +63,7 @@ func TestMiddleware(t *testing.T) {
6463
}
6564

6665
t.Run("unauthenticated homepage visit, sign-out cookie present, access requests enabled -> sg sign-in", func(t *testing.T) {
67-
cookie := &http.Cookie{Name: auth.SignOutCookie, Value: "true"}
66+
cookie := &http.Cookie{Name: session.SignOutCookie, Value: "true"}
6867
resp := doRequest("GET", "http://example.com/", "", "", []*http.Cookie{cookie}, false)
6968
if want := http.StatusOK; resp.StatusCode != want {
7069
t.Errorf("got response code %v, want %v", resp.StatusCode, want)

cmd/frontend/internal/auth/githuboauth/middleware_test.go

+2-3
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414

1515
"github.com/sourcegraph/log/logtest"
1616

17-
"github.com/sourcegraph/sourcegraph/cmd/frontend/auth"
1817
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/oauth"
1918
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/providers"
2019
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/session"
@@ -65,7 +64,7 @@ func TestMiddleware(t *testing.T) {
6564
}
6665

6766
t.Run("unauthenticated homepage visit, sign-out cookie present, access requests enabled -> sg sign-in", func(t *testing.T) {
68-
cookie := &http.Cookie{Name: auth.SignOutCookie, Value: "true"}
67+
cookie := &http.Cookie{Name: session.SignOutCookie, Value: "true"}
6968
resp := doRequest("GET", "http://example.com/", "", "", []*http.Cookie{cookie}, false)
7069
if want := http.StatusOK; resp.StatusCode != want {
7170
t.Errorf("got response code %v, want %v", resp.StatusCode, want)
@@ -82,7 +81,7 @@ func TestMiddleware(t *testing.T) {
8281
})
8382
t.Cleanup(func() { conf.Mock(nil) })
8483

85-
cookie := &http.Cookie{Name: auth.SignOutCookie, Value: "true"}
84+
cookie := &http.Cookie{Name: session.SignOutCookie, Value: "true"}
8685

8786
resp := doRequest("GET", "http://example.com/", "", "", []*http.Cookie{cookie}, false)
8887
if want := http.StatusOK; resp.StatusCode != want {

cmd/frontend/internal/auth/gitlaboauth/middleware_test.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414

1515
"github.com/sourcegraph/log/logtest"
1616

17-
"github.com/sourcegraph/sourcegraph/cmd/frontend/auth"
1817
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/oauth"
1918
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/providers"
2019
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/session"
@@ -94,7 +93,7 @@ func TestMiddleware(t *testing.T) {
9493
}
9594
})
9695
t.Run("unauthenticated homepage visit, sign-out cookie present, access requests enabled -> sg sign-in", func(t *testing.T) {
97-
cookie := &http.Cookie{Name: auth.SignOutCookie, Value: "true"}
96+
cookie := &http.Cookie{Name: session.SignOutCookie, Value: "true"}
9897

9998
resp := doRequest("GET", "http://example.com/", "", "", []*http.Cookie{cookie}, false)
10099
if want := http.StatusOK; resp.StatusCode != want {

cmd/frontend/internal/auth/oauth/middleware.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818

1919
"github.com/sourcegraph/sourcegraph/cmd/frontend/auth"
2020
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/providers"
21+
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/session"
2122
"github.com/sourcegraph/sourcegraph/internal/actor"
2223
"github.com/sourcegraph/sourcegraph/internal/conf"
2324
"github.com/sourcegraph/sourcegraph/internal/database"
@@ -63,7 +64,7 @@ func NewMiddleware(db database.DB, serviceType, authPrefix string, isAPIHandler
6364
// Note: For instances that are conf.AuthPublic(), we don't redirect to sign-in automatically, as that would
6465
// lock out unauthenticated access.
6566
pc := getExactlyOneOAuthProvider(!r.URL.Query().Has("sourcegraph-operator"))
66-
if !conf.AuthPublic() && pc != nil && !isAPIHandler && pc.AuthPrefix == authPrefix && !auth.HasSignOutCookie(r) && isHuman(r) && !conf.IsAccessRequestEnabled() {
67+
if !conf.AuthPublic() && pc != nil && !isAPIHandler && pc.AuthPrefix == authPrefix && !session.HasSignOutCookie(r) && isHuman(r) && !conf.IsAccessRequestEnabled() {
6768
span.AddEvent("redirect to signin")
6869
v := make(url.Values)
6970
v.Set("redirect", auth.SafeRedirectURL(r.URL.String()))

cmd/frontend/internal/auth/openidconnect/middleware.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func handleOpenIDConnectAuth(logger log.Logger, db database.DB, w http.ResponseW
131131
// lock out unauthenticated access.
132132
ps := providers.SignInProviders(!r.URL.Query().Has("sourcegraph-operator"))
133133
openIDConnectEnabled := len(ps) == 1 && ps[0].Config().Openidconnect != nil
134-
if !conf.AuthPublic() && openIDConnectEnabled && !auth.HasSignOutCookie(r) && !isAPIRequest {
134+
if !conf.AuthPublic() && openIDConnectEnabled && !session.HasSignOutCookie(r) && !isAPIRequest {
135135
p, oidcClient, safeErrMsg, err := GetProviderAndClient(r.Context(), ps[0].ConfigID().ID, GetProvider)
136136
if err != nil {
137137
log15.Error("Failed to get provider", "error", err)

cmd/frontend/internal/auth/openidconnect/middleware_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ func TestMiddleware(t *testing.T) {
208208
}
209209

210210
t.Run("unauthenticated homepage visit, sign-out cookie present -> sg sign-in", func(t *testing.T) {
211-
cookie := &http.Cookie{Name: auth.SignOutCookie, Value: "true"}
211+
cookie := &http.Cookie{Name: session.SignOutCookie, Value: "true"}
212212

213213
resp := doRequest("GET", "http://example.com/", "", "", []*http.Cookie{cookie}, false)
214214
if want := http.StatusOK; resp.StatusCode != want {

cmd/frontend/internal/auth/saml/middleware.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func authHandler(db database.DB, w http.ResponseWriter, r *http.Request, next ht
6565
// Note: For instances that are conf.AuthPublic(), we don't redirect to sign-in automatically, as that would
6666
// lock out unauthenticated access.
6767
ps := providers.SignInProviders(!r.URL.Query().Has("sourcegraph-operator"))
68-
if !conf.AuthPublic() && len(ps) == 1 && ps[0].Config().Saml != nil && !auth.HasSignOutCookie(r) && !isAPIRequest {
68+
if !conf.AuthPublic() && len(ps) == 1 && ps[0].Config().Saml != nil && !session.HasSignOutCookie(r) && !isAPIRequest {
6969
p, handled := handleGetProvider(r.Context(), w, ps[0].ConfigID().ID)
7070
if handled {
7171
return

cmd/frontend/internal/auth/saml/middleware_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ func TestMiddleware(t *testing.T) {
294294
}
295295
})
296296
t.Run("unauthenticated homepage visit, sign-out cookie present -> sg login", func(t *testing.T) {
297-
cookie := &http.Cookie{Name: auth.SignOutCookie, Value: "true"}
297+
cookie := &http.Cookie{Name: session.SignOutCookie, Value: "true"}
298298

299299
resp := doRequest("GET", "http://example.com/", "", []*http.Cookie{cookie}, false, nil)
300300
if want := http.StatusOK; resp.StatusCode != want {

cmd/frontend/internal/auth/session/session.go

+13-3
Original file line numberDiff line numberDiff line change
@@ -217,15 +217,15 @@ func SetActor(w http.ResponseWriter, r *http.Request, actor *actor.Actor, expiry
217217
expiryPeriod = defaultExpiryPeriod
218218
}
219219
}
220-
RemoveSignOutCookieIfSet(r, w)
220+
removeSignOutCookieIfSet(r, w)
221221

222222
value = &sessionInfo{Actor: actor, ExpiryPeriod: expiryPeriod, LastActive: time.Now(), UserCreatedAt: userCreatedAt}
223223
}
224224
return SetData(w, r, "actor", value)
225225
}
226226

227-
// RemoveSignOutCookieIfSet removes the sign-out cookie if it is set.
228-
func RemoveSignOutCookieIfSet(r *http.Request, w http.ResponseWriter) {
227+
// removeSignOutCookieIfSet removes the sign-out cookie if it is set.
228+
func removeSignOutCookieIfSet(r *http.Request, w http.ResponseWriter) {
229229
if HasSignOutCookie(r) {
230230
http.SetCookie(w, &http.Cookie{Name: SignOutCookie, Value: "", MaxAge: -1})
231231
}
@@ -240,6 +240,16 @@ func HasSignOutCookie(r *http.Request) bool {
240240
return ck != nil
241241
}
242242

243+
// SetSignOutCookie sets a sign-out cookie on the given response.
244+
func SetSignOutCookie(w http.ResponseWriter) {
245+
http.SetCookie(w, &http.Cookie{
246+
Name: SignOutCookie,
247+
Value: "true",
248+
Secure: true,
249+
Path: "/",
250+
})
251+
}
252+
243253
// hasSessionCookie returns true if the given request has a session cookie.
244254
func hasSessionCookie(r *http.Request) bool {
245255
c, _ := r.Cookie(cookieName)

0 commit comments

Comments
 (0)