From 80686dec269ac66e8e100daf2500fc149a76d876 Mon Sep 17 00:00:00 2001 From: cblgh Date: Fri, 23 Apr 2021 13:25:13 +0200 Subject: [PATCH 1/8] cherrypick: ts.User is now pointer --- web/handlers/admin/setup_test.go | 6 +++++- web/members/testing.go | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/web/handlers/admin/setup_test.go b/web/handlers/admin/setup_test.go index 3f12498..60b7968 100644 --- a/web/handlers/admin/setup_test.go +++ b/web/handlers/admin/setup_test.go @@ -99,6 +99,10 @@ func newSession(t *testing.T) *testSession { ts.User = roomdb.Member{ ID: 1234, Role: roomdb.RoleModerator, + PubKey: refs.FeedRef{ + ID: bytes.Repeat([]byte("0"), 32), + Algo: "ed25519", + }, } testPath := filepath.Join("testrun", t.Name()) @@ -174,7 +178,7 @@ func newSession(t *testing.T) *testSession { }, ) - handler = members.MiddlewareForTests(ts.User)(handler) + handler = members.MiddlewareForTests(&ts.User)(handler) ts.Mux = http.NewServeMux() ts.Mux.Handle("/", handler) diff --git a/web/members/testing.go b/web/members/testing.go index 6f31f1b..2ef5ef9 100644 --- a/web/members/testing.go +++ b/web/members/testing.go @@ -13,10 +13,10 @@ import ( // This is part of testing.go because we need to use roomMemberContextKey, which shouldn't be exported either. // TODO: could be protected with an extra build tag. // (Sadly +build test does not exist https://github.com/golang/go/issues/21360 ) -func MiddlewareForTests(m roomdb.Member) func(http.Handler) http.Handler { +func MiddlewareForTests(m *roomdb.Member) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ctx := context.WithValue(req.Context(), roomMemberContextKey, &m) + ctx := context.WithValue(req.Context(), roomMemberContextKey, m) next.ServeHTTP(w, req.WithContext(ctx)) }) } From ab06233f287c859abe8c5cb775b15a419926c5d5 Mon Sep 17 00:00:00 2001 From: cblgh Date: Sun, 25 Apr 2021 15:59:17 +0200 Subject: [PATCH 2/8] test invite creation under community & restricted for admin, mod & member Update web/handlers/admin/setup_test.go Co-authored-by: Henry <111202+cryptix@users.noreply.github.com> --- web/handlers/admin/invites.go | 4 +-- web/handlers/admin/invites_test.go | 55 ++++++++++++++++++++++++++---- web/handlers/admin/setup_test.go | 19 +++++++---- 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/web/handlers/admin/invites.go b/web/handlers/admin/invites.go index d1e4ae3..ea92822 100644 --- a/web/handlers/admin/invites.go +++ b/web/handlers/admin/invites.go @@ -76,11 +76,11 @@ func (h invitesHandler) create(w http.ResponseWriter, req *http.Request) (interf case roomdb.ModeOpen: case roomdb.ModeCommunity: if member.Role == roomdb.RoleUnknown { - return nil, fmt.Errorf("warning: member with unknown role tried to create an invite") + return nil, weberrors.ErrNotAuthorized } case roomdb.ModeRestricted: if member.Role == roomdb.RoleMember || member.Role == roomdb.RoleUnknown { - return nil, fmt.Errorf("warning: non-admin/mod user tried to create an invite") + return nil, weberrors.ErrNotAuthorized } } diff --git a/web/handlers/admin/invites_test.go b/web/handlers/admin/invites_test.go index c074060..95a4327 100644 --- a/web/handlers/admin/invites_test.go +++ b/web/handlers/admin/invites_test.go @@ -2,6 +2,7 @@ package admin import ( "net/http" + "net/http/httptest" "net/url" "testing" @@ -143,17 +144,29 @@ func TestInvitesCreate(t *testing.T) { a := assert.New(t) r := require.New(t) - urlRemove := ts.URLTo(router.AdminInvitesCreate) + urlCreate := ts.URLTo(router.AdminInvitesCreate) testInvite := "your-fake-test-invite" ts.InvitesDB.CreateReturns(testInvite, nil) - rec := ts.Client.PostForm(urlRemove, url.Values{}) - a.Equal(http.StatusOK, rec.Code) + totalCreateCallCount := 0 + createInviteShouldWork := func(works bool) *httptest.ResponseRecorder { + rec := ts.Client.PostForm(urlCreate, url.Values{}) + if works { + totalCreateCallCount += 1 + a.Equal(http.StatusOK, rec.Code) + r.Equal(totalCreateCallCount, ts.InvitesDB.CreateCallCount()) + _, userID := ts.InvitesDB.CreateArgsForCall(totalCreateCallCount - 1) + a.EqualValues(ts.User.ID, userID) + } else { + // TODO: status should be http.StatusForbidden? see invites.go:79 + a.Equal(http.StatusInternalServerError, rec.Code) + r.Equal(totalCreateCallCount, ts.InvitesDB.CreateCallCount()) + } + return rec + } - r.Equal(1, ts.InvitesDB.CreateCallCount(), "expected one invites.Create call") - _, userID := ts.InvitesDB.CreateArgsForCall(0) - a.EqualValues(ts.User.ID, userID) + rec := createInviteShouldWork(true) doc, err := goquery.NewDocumentFromReader(rec.Body) require.NoError(t, err, "failed to parse response") @@ -167,4 +180,34 @@ func TestInvitesCreate(t *testing.T) { shownLink := doc.Find("#invite-facade-link").Text() a.Equal(wantURL.String(), shownLink) + + memberUser := roomdb.Member{ + ID: 7331, + Role: roomdb.RoleMember, + PubKey: generatePubKey(), + } + modUser := roomdb.Member{ + ID: 9001, + Role: roomdb.RoleModerator, + PubKey: generatePubKey(), + } + adminUser := roomdb.Member{ + ID: 1337, + Role: roomdb.RoleAdmin, + PubKey: generatePubKey(), + } + + /* test invite creation under various restricted mode with the roles member, mod, admin */ + modes := []roomdb.PrivacyMode{roomdb.ModeRestricted, roomdb.ModeCommunity} + for _, mode := range modes { + ts.ConfigDB.GetPrivacyModeReturns(mode, nil) + ts.User = memberUser + // members can only invite in community rooms + createInviteShouldWork(mode == roomdb.ModeCommunity) + // mods & admins can always invite + ts.User = modUser + createInviteShouldWork(true) + ts.User = adminUser + createInviteShouldWork(true) + } } diff --git a/web/handlers/admin/setup_test.go b/web/handlers/admin/setup_test.go index 60b7968..871e46b 100644 --- a/web/handlers/admin/setup_test.go +++ b/web/handlers/admin/setup_test.go @@ -56,6 +56,14 @@ type testSession struct { RoomState *roomstate.Manager } +var pubKeyCount byte + +func generatePubKey() refs.FeedRef { + pk := refs.FeedRef{Algo: "ed25519", ID: bytes.Repeat([]byte{pubKeyCount}, 32)} + pubKeyCount++ + return pk +} + func newSession(t *testing.T) *testSession { var ts testSession @@ -76,7 +84,7 @@ func newSession(t *testing.T) *testSession { ts.netInfo = network.ServerEndpointDetails{ Domain: randutil.String(10), - RoomID: refs.FeedRef{Algo: "ed25519", ID: bytes.Repeat([]byte{0}, 32)}, + RoomID: generatePubKey(), UseSubdomainForAliases: true, } @@ -97,12 +105,9 @@ func newSession(t *testing.T) *testSession { // fake user ts.User = roomdb.Member{ - ID: 1234, - Role: roomdb.RoleModerator, - PubKey: refs.FeedRef{ - ID: bytes.Repeat([]byte("0"), 32), - Algo: "ed25519", - }, + ID: 1234, + Role: roomdb.RoleModerator, + PubKey: generatePubKey(), } testPath := filepath.Join("testrun", t.Name()) From f0b4c7a5349bcfd34286a68f3fc59d5dffa04a1e Mon Sep 17 00:00:00 2001 From: cblgh Date: Sun, 25 Apr 2021 16:33:55 +0200 Subject: [PATCH 3/8] test that alias resolving is turned off for restricted rooms --- web/handlers/aliases_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/web/handlers/aliases_test.go b/web/handlers/aliases_test.go index 78b947f..c95b814 100644 --- a/web/handlers/aliases_test.go +++ b/web/handlers/aliases_test.go @@ -88,4 +88,11 @@ func TestAliasResolve(t *testing.T) { a.Equal(testAlias.Feed.Ref(), ar.UserID, "wrong user feed on response") a.Equal(ts.NetworkInfo.RoomID.Ref(), ar.RoomID, "wrong room feed on response") a.Equal(ts.NetworkInfo.MultiserverAddress(), ar.MultiserverAddress) + + /* alias resolving should not work for restricted rooms */ + ts.ConfigDB.GetPrivacyModeReturns(roomdb.ModeRestricted, nil) + htmlURL, err = routes.Get(router.CompleteAliasResolve).URL("alias", testAlias.Name) + r.NoError(err) + html, resp = ts.Client.GetHTML(htmlURL) + a.Equal(http.StatusInternalServerError, resp.Code) } From 72e97dd3749ef51b80b317a4bea048f4286de139 Mon Sep 17 00:00:00 2001 From: cblgh Date: Sun, 25 Apr 2021 16:40:28 +0200 Subject: [PATCH 4/8] refactor out unused reference --- roomsrv/init_network.go | 2 +- roomsrv/server.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/roomsrv/init_network.go b/roomsrv/init_network.go index 883db6e..631cb3f 100644 --- a/roomsrv/init_network.go +++ b/roomsrv/init_network.go @@ -35,7 +35,7 @@ func (s *Server) initNetwork() error { // if privacy mode is restricted, deny connections from non-members if pm == roomdb.ModeRestricted { - if _, err := s.authorizer.GetByFeed(s.rootCtx, *remote); err != nil { + if _, err := s.Members.GetByFeed(s.rootCtx, *remote); err != nil { return nil, fmt.Errorf("access restricted to members") } } diff --git a/roomsrv/server.go b/roomsrv/server.go index 6ed2aed..d51817a 100644 --- a/roomsrv/server.go +++ b/roomsrv/server.go @@ -61,8 +61,6 @@ type Server struct { public typemux.HandlerMux master typemux.HandlerMux - authorizer roomdb.MembersService - StateManager *roomstate.Manager Members roomdb.MembersService @@ -89,7 +87,6 @@ func New( opts ...Option, ) (*Server, error) { var s Server - s.authorizer = membersdb s.Members = membersdb s.DeniedKeys = deniedkeysdb From 346ba14ec951effa102979dc20646afd87ca9994 Mon Sep 17 00:00:00 2001 From: cblgh Date: Sun, 25 Apr 2021 16:49:33 +0200 Subject: [PATCH 5/8] test connection establishment for non-members in restricted rooms --- muxrpc/test/go/deny_test.go | 69 +++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/muxrpc/test/go/deny_test.go b/muxrpc/test/go/deny_test.go index 5c8ca12..a4f2f3d 100644 --- a/muxrpc/test/go/deny_test.go +++ b/muxrpc/test/go/deny_test.go @@ -84,3 +84,72 @@ func TestConnEstablishmentDeniedKey(t *testing.T) { cancel() } + +func TestConnEstablishmentDenyNonMembersRestrictedRoom(t *testing.T) { + // defer leakcheck.Check(t) + ctx, cancel := context.WithCancel(context.Background()) + theBots := createServerAndBots(t, ctx, 2) + + r := require.New(t) + a := assert.New(t) + + const ( + indexSrv = iota + indexA + indexB + ) + + serv := theBots[indexSrv].srv + botA := theBots[indexA].srv + botB := theBots[indexB].srv + + // make sure we are running in restricted mode on the server + err := serv.Config.SetPrivacyMode(ctx, roomdb.ModeRestricted) + r.NoError(err) + + // allow A, deny B + theBots[indexSrv].srv.Members.Add(ctx, botA.Whoami(), roomdb.RoleMember) + // since we want to verify denying connections for non members in a restricte room, let us: + // a) NOT add B as a member + // theBots[indexSrv].srv.Members.Add(ctx, botB.Whoami(), roomdb.RoleMember) + + // hack: allow bots to dial the server + theBots[indexA].srv.Members.Add(ctx, serv.Whoami(), roomdb.RoleMember) + theBots[indexB].srv.Members.Add(ctx, serv.Whoami(), roomdb.RoleMember) + + // dial up B->A and C->A + // should work (we allowed A) + err = botA.Network.Connect(ctx, serv.Network.GetListenAddr()) + r.NoError(err, "connect A to the Server") + + // shouldn't work (we banned B) + err = botB.Network.Connect(ctx, serv.Network.GetListenAddr()) + r.NoError(err, "connect B to the Server") // we dont see an error because it just establishes the tcp connection + + t.Log("letting handshaking settle..") + time.Sleep(1 * time.Second) + + var srvWho struct { + ID refs.FeedRef + } + + endpointB, has := botB.Network.GetEndpointFor(serv.Whoami()) + r.False(has, "botB has an endpoint for the server!") + if endpointB != nil { + a.Nil(endpointB, "should not have an endpoint on B (B is not a member, and the server is restricted)") + err = endpointB.Async(ctx, &srvWho, muxrpc.TypeJSON, muxrpc.Method{"whoami"}) + r.Error(err) + t.Log(srvWho.ID.Ref()) + } + + endpointA, has := botA.Network.GetEndpointFor(serv.Whoami()) + r.True(has, "botA has no endpoint for the server") + + err = endpointA.Async(ctx, &srvWho, muxrpc.TypeJSON, muxrpc.Method{"whoami"}) + r.NoError(err) + + t.Log("server whoami:", srvWho.ID.Ref()) + a.True(serv.Whoami().Equal(&srvWho.ID)) + + cancel() +} From c3ac3a167821b71e06be5a9ddddfc2dc8d8971da Mon Sep 17 00:00:00 2001 From: cblgh Date: Sun, 25 Apr 2021 16:55:24 +0200 Subject: [PATCH 6/8] test that non-members are not blocked from establishing connections for community mode servers --- muxrpc/test/go/deny_test.go | 73 ++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/muxrpc/test/go/deny_test.go b/muxrpc/test/go/deny_test.go index a4f2f3d..c0b9603 100644 --- a/muxrpc/test/go/deny_test.go +++ b/muxrpc/test/go/deny_test.go @@ -109,7 +109,7 @@ func TestConnEstablishmentDenyNonMembersRestrictedRoom(t *testing.T) { // allow A, deny B theBots[indexSrv].srv.Members.Add(ctx, botA.Whoami(), roomdb.RoleMember) - // since we want to verify denying connections for non members in a restricte room, let us: + // since we want to verify denying connections for non members in a restricted room, let us: // a) NOT add B as a member // theBots[indexSrv].srv.Members.Add(ctx, botB.Whoami(), roomdb.RoleMember) @@ -122,7 +122,7 @@ func TestConnEstablishmentDenyNonMembersRestrictedRoom(t *testing.T) { err = botA.Network.Connect(ctx, serv.Network.GetListenAddr()) r.NoError(err, "connect A to the Server") - // shouldn't work (we banned B) + // shouldn't work (we disallow B in restricted mode) err = botB.Network.Connect(ctx, serv.Network.GetListenAddr()) r.NoError(err, "connect B to the Server") // we dont see an error because it just establishes the tcp connection @@ -153,3 +153,72 @@ func TestConnEstablishmentDenyNonMembersRestrictedRoom(t *testing.T) { cancel() } + +func TestConnEstablishmentAllowNonMembersCommunityRoom(t *testing.T) { + // defer leakcheck.Check(t) + ctx, cancel := context.WithCancel(context.Background()) + theBots := createServerAndBots(t, ctx, 2) + + r := require.New(t) + a := assert.New(t) + + const ( + indexSrv = iota + indexA + indexB + ) + + serv := theBots[indexSrv].srv + botA := theBots[indexA].srv + botB := theBots[indexB].srv + + // make sure we are running in community mode on the server + err := serv.Config.SetPrivacyMode(ctx, roomdb.ModeCommunity) + r.NoError(err) + pm, err := serv.Config.GetPrivacyMode(ctx) + r.NoError(err) + r.EqualValues(roomdb.ModeCommunity, pm) + + // allow A, allow B + theBots[indexSrv].srv.Members.Add(ctx, botA.Whoami(), roomdb.RoleMember) + // since we want to verify allowing connections for non members in a community room, let us: + // a) NOT add B as a member + // theBots[indexSrv].srv.Members.Add(ctx, botB.Whoami(), roomdb.RoleMember) + + // hack: allow bots to dial the server + theBots[indexA].srv.Members.Add(ctx, serv.Whoami(), roomdb.RoleMember) + theBots[indexB].srv.Members.Add(ctx, serv.Whoami(), roomdb.RoleMember) + + // dial up B->A and C->A + // should work (we allowed A) + err = botA.Network.Connect(ctx, serv.Network.GetListenAddr()) + r.NoError(err, "connect A to the Server") + + // should work (we don't disallow B in community mode) + err = botB.Network.Connect(ctx, serv.Network.GetListenAddr()) + r.NoError(err, "connect B to the Server") + + t.Log("letting handshaking settle..") + time.Sleep(1 * time.Second) + + var srvWho struct { + ID refs.FeedRef + } + + endpointB, has := botB.Network.GetEndpointFor(serv.Whoami()) + r.True(has, "botB has no endpoint for the server") + err = endpointB.Async(ctx, &srvWho, muxrpc.TypeJSON, muxrpc.Method{"whoami"}) + r.NoError(err) + t.Log(srvWho.ID.Ref()) + + endpointA, has := botA.Network.GetEndpointFor(serv.Whoami()) + r.True(has, "botA has no endpoint for the server") + + err = endpointA.Async(ctx, &srvWho, muxrpc.TypeJSON, muxrpc.Method{"whoami"}) + r.NoError(err) + + t.Log("server whoami:", srvWho.ID.Ref()) + a.True(serv.Whoami().Equal(&srvWho.ID)) + + cancel() +} From cae80e385e7bf16c4fd0016f63dfa5882f2dc00b Mon Sep 17 00:00:00 2001 From: cblgh Date: Mon, 26 Apr 2021 08:28:42 +0200 Subject: [PATCH 7/8] make alias mockdb return alias entry --- web/errors/errhandler.go | 2 +- web/handlers/admin/aliases_test.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/web/errors/errhandler.go b/web/errors/errhandler.go index 4918f97..37b0d2d 100644 --- a/web/errors/errhandler.go +++ b/web/errors/errhandler.go @@ -34,7 +34,7 @@ func (eh *ErrorHandler) SetRenderer(r *render.Renderer) { func (eh *ErrorHandler) Handle(rw http.ResponseWriter, req *http.Request, code int, err error) { log := logging.FromContext(req.Context()) - level.Error(log).Log("event", "handling error","path", req.URL.Path, "err",err) + level.Error(log).Log("event", "handling error", "path", req.URL.Path, "err", err) var redirectErr ErrRedirect if errors.As(err, &redirectErr) { if redirectErr.Reason != nil { diff --git a/web/handlers/admin/aliases_test.go b/web/handlers/admin/aliases_test.go index 836915a..8bf6888 100644 --- a/web/handlers/admin/aliases_test.go +++ b/web/handlers/admin/aliases_test.go @@ -53,7 +53,13 @@ func TestAliasesRevoke(t *testing.T) { urlRevoke := ts.URLTo(router.AdminAliasesRevoke) overviewURL := ts.URLTo(router.AdminMembersOverview) + aliasEntry := roomdb.Alias{ + ID: ts.User.ID, + Feed: ts.User.PubKey, + Name: "Blobby", + } ts.AliasesDB.RevokeReturns(nil) + ts.AliasesDB.ResolveReturns(aliasEntry, nil) addVals := url.Values{"name": []string{"the-name"}} rec := ts.Client.PostForm(urlRevoke, addVals) From 3651432b42c12324c768095e5d2caafb801063ed Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 26 Apr 2021 08:59:43 +0200 Subject: [PATCH 8/8] use errHandler in admin tests --- web/handlers/admin/invites_test.go | 3 +-- web/handlers/admin/notices_test.go | 2 +- web/handlers/admin/setup_test.go | 10 +++++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/web/handlers/admin/invites_test.go b/web/handlers/admin/invites_test.go index 95a4327..7b6161f 100644 --- a/web/handlers/admin/invites_test.go +++ b/web/handlers/admin/invites_test.go @@ -159,8 +159,7 @@ func TestInvitesCreate(t *testing.T) { _, userID := ts.InvitesDB.CreateArgsForCall(totalCreateCallCount - 1) a.EqualValues(ts.User.ID, userID) } else { - // TODO: status should be http.StatusForbidden? see invites.go:79 - a.Equal(http.StatusInternalServerError, rec.Code) + a.Equal(http.StatusForbidden, rec.Code) r.Equal(totalCreateCallCount, ts.InvitesDB.CreateCallCount()) } return rec diff --git a/web/handlers/admin/notices_test.go b/web/handlers/admin/notices_test.go index 724fb8c..1ca75a1 100644 --- a/web/handlers/admin/notices_test.go +++ b/web/handlers/admin/notices_test.go @@ -89,7 +89,7 @@ func TestNoticeAddLanguageOnlyAllowsPost(t *testing.T) { // verify that a GET request is no bueno u := ts.URLTo(router.AdminNoticeAddTranslation, "name", roomdb.NoticeNews.String()) _, resp := ts.Client.GetHTML(u) - a.Equal(http.StatusMethodNotAllowed, resp.Code, "GET should not be allowed for this route") + a.Equal(http.StatusBadRequest, resp.Code, "GET should not be allowed for this route") // next up, we verify that a correct POST request actually works: id := []string{"1"} diff --git a/web/handlers/admin/setup_test.go b/web/handlers/admin/setup_test.go index 871e46b..e95d41f 100644 --- a/web/handlers/admin/setup_test.go +++ b/web/handlers/admin/setup_test.go @@ -130,10 +130,10 @@ func newSession(t *testing.T) *testSession { os.MkdirAll(sessionsPath, 0700) fsStore := sessions.NewFilesystemStore(sessionsPath, authKey, encKey) + // setup rendering flashHelper := weberrs.NewFlashHelper(fsStore, locHelper) - // setup rendering - + // template funcs // TODO: make testing utils and move these there testFuncs := web.TemplateFuncs(router, ts.netInfo) testFuncs["current_page_is"] = func(routeName string) bool { return true } @@ -152,11 +152,13 @@ func newSession(t *testing.T) *testSession { testFuncs["list_languages"] = func(*url.URL, string) string { return "" } testFuncs["relative_time"] = func(when time.Time) string { return humanize.Time(when) } + eh := weberrs.NewErrorHandler(locHelper, flashHelper) + renderOpts := []render.Option{ render.SetLogger(log), render.BaseTemplates("base.tmpl", "menu.tmpl", "flashes.tmpl"), render.AddTemplates(append(HTMLTemplates, "error.tmpl")...), - render.ErrorTemplate("error.tmpl"), + render.SetErrorHandler(eh.Handle), render.FuncMap(testFuncs), } renderOpts = append(renderOpts, locHelper.GetRenderFuncs()...) @@ -166,6 +168,8 @@ func newSession(t *testing.T) *testSession { t.Fatal(errors.Wrap(err, "setup: render init failed")) } + eh.SetRenderer(r) + handler := Handler( ts.netInfo, r,