Add switchback feature #5440

kimlisa · 2021-01-30T01:23:46Z

part of #4937

TODO: update apiserver_test.go, after base branch is merged

Description

For UI users, allow user to switch back to default from assuming access requests, by web client calling renewSession, that also now accepts values of session expiration and roles (these are saved in the browser on first session creation after login)

Create renewSessionRequest object to read from request in renew session
Change the signature of ExtendWebSession to accept a request struct that contains session information
Add Session field to session responses, that returns session expiration time and roles assigned

…eparately for clear separation

sessions are created and where existing sessions are managed. Remove ttlmap in favor of a simple map and handle expirations explicitly. Add web session management to gRPC server for the cache.

Fix web sessions unit tests.

has not yet been added to session cache. lib/cache: add event filter for web session in auth cache. lib/auth: propagate web session subkind in gRPC event.

…se fake time in web tests

the old behavior w.r.t access checking (e.g. implicit handling of the local user identity).

…ns using best-effort - first looking in the cache and falling back to a proxy client

the purpose of this PR. Also avoid a race between closing the cached clients and an existing reference to the session by letting the session linger for longer before removing it.

Fix the web UI assets configuration by removing DisableUI and instead use the presence of assets (HTTP file system) as an indicator that the web UI has been enabled.

… the expiration error message for errors other than not found

a-palchikov · 2021-02-01T10:23:34Z

lib/auth/auth.go

+		defaultExpiry := req.SwitchBackDefaults.Expires
+
+		if err := utils.StringSliceSubset(roles, defaultRoles); err != nil {
+			return nil, trace.Wrap(err)


The function's error message will be pretty generic - we should amend the message to include the relevant bits about role mismatch.

a-palchikov · 2021-02-01T10:26:19Z

lib/auth/auth.go

+		}
+
+		roles = req.SwitchBackDefaults.Roles
+		expiresAt = req.SwitchBackDefaults.Expires


Does that have to take the current session's expiration into account? I don't have a full context on the switching back to defaults (is it what it is?), so I'm wondering whether it's possible that request.SwitchBackDefaults.Expires > session.Expires?

a-palchikov · 2021-02-01T10:29:53Z

lib/auth/tls_test.go

@@ -1518,6 +1531,28 @@ func (s *TLSSuite) TestWebSessionWithApprovedAccessRequest(c *check.C) {

 	_, hasRole = mappedRole["test-request-role"]
 	c.Assert(hasRole, check.Equals, true)
+
+	// Test swtich back to default role and expiry.


Can you add this as an additional test case? Usually, the old tests are not changed if the base functionality was not modified to render it invalid. It would be additionally helpful to have as minimal test case as possible, sometimes the tests grow to become cumbersome to read and know what's being tested.

a-palchikov · 2021-02-01T10:40:04Z

lib/auth/apiserver.go

+	SwitchBackDefaults *WebSession `json:"session"`
+}
+
+// WebSession defines fields describing a session expiration and roles assigned to it.


WebSession is a pretty generic name. How does this differ from the WebSession from the API types package? As this is used to represent the attributes of the original web session used to switch user back to their defaults, I would try to keep this meaning as part of the type.

a-palchikov · 2021-02-01T10:40:11Z

lib/auth/apiserver.go

-type createWebSessionReq struct {
-	PrevSessionID   string `json:"prev_session_id"`
+// CreateWebSessionReq defines fields for requesting to create new web session.
+type CreateWebSessionReq struct {


This is used to extend a web session, ExtendWebSessionRequest could be a more apt name.

a-palchikov · 2021-02-01T10:43:51Z

lib/web/apiserver_test.go

@@ -1875,6 +1873,14 @@ func TestWebSessionsRenew(t *testing.T) {
 	prevSessionID := decodeSessionCookie(t, prevSessionCookie.Value)
 	activeSessionID := decodeSessionCookie(t, sessionCookie.Value)
 	require.NotEmpty(t, cmp.Diff(prevSessionID, activeSessionID))
+	require.NotEmpty(t, cmp.Diff(pack.session.ExpiresIn, newPack.session.ExpiresIn))


Is this accurate? I think the session expiration should be the same.

a-palchikov · 2021-02-01T10:49:11Z

lib/auth/tls_test.go

@@ -1518,6 +1531,28 @@ func (s *TLSSuite) TestWebSessionWithApprovedAccessRequest(c *check.C) {

 	_, hasRole = mappedRole["test-request-role"]
 	c.Assert(hasRole, check.Equals, true)
+
+	// Test swtich back to default role and expiry.


Suggested change

// Test swtich back to default role and expiry.

// Test switch back to default role and expiry.

a-palchikov · 2021-02-01T10:52:02Z

lib/web/apiserver_test.go

+
+	s := auth.WebSession{
+		Expires: newPack.session.Session.Expires,
+		Roles:   []string{"foo"},


I don't really understand how this is working here.
There's a number of access request test cases in auth/tls_test.go - is there any intersection with this? Do you have to test this here as well?

a-palchikov · 2021-02-01T10:53:26Z

lib/web/apiserver.go

-	requestID := params.ByName("requestId")
+	req := &renewSessionRequest{}
+	if r.Body != http.NoBody {
+		if err := httplib.ReadJSON(r, &req); err != nil {


nit: this should not use the address of the pointer, I don't see a reason for indirection. Just use a local value of type renewSessionRequest:

var req renewSessionRequest

a-palchikov added 30 commits January 29, 2021 22:18

Use fake clock consistently in units tests.

d4c98da

wip: web sessions caching layer

2441fe1

Split web session management into two interfaces and implement them s…

71f22bc

…eparately for clear separation

Split session management into New/Validate to make it aparent where the

1ff8c9e

sessions are created and where existing sessions are managed. Remove ttlmap in favor of a simple map and handle expirations explicitly. Add web session management to gRPC server for the cache.

Reintroduce web sessions APIs under a getter interface.

95dbe12

Add SubKind to WatchKind for gRPC and add conversions from/to protobuf.

979b498

Fix web sessions unit tests.

lib/web: create/insert session context in ValidateSession if the session

12b6e85

has not yet been added to session cache. lib/cache: add event filter for web session in auth cache. lib/auth: propagate web session subkind in gRPC event.

Add implicit migrations for legacy web session key path for queries.

a4962dd

wip: adding web token resource

4b8a61d

Integrate web token in lib/web

ccec922

Add a bearer token when upserting a web session

b697f4e

Fix tests. Use fake clock wherever possible.

1484d5b

Converge session cache handling in lib/web

d148c90

Clean up and add doc comments where necessary

cf452e3

Use correct form of sessions/tokens controller for ServerWithRoles. U…

1a7c1b8

…se fake time in web tests

Converge the web sessions/tokens handling in lib/auth to match

c3c9044

the old behavior w.r.t access checking (e.g. implicit handling of the local user identity).

Use cached reads and waiters only when necessary. Query sessions/toke…

a8da3cc

…ns using best-effort - first looking in the cache and falling back to a proxy client

Properly propagate events about deletes for values with subkind.

8cd16f5

Update to retrofit changes after recent teleport API refactorings

8443437

fix lint warnings

15b16e2

Update comment on removing legacy code to move the deadline to 7.x

a4b2b85

Do not close the resources on the session when it expires - this beats

cfdea45

the purpose of this PR. Also avoid a race between closing the cached clients and an existing reference to the session by letting the session linger for longer before removing it.

Address review comments

db5f7cc

Move web session/token request structs to the api client proto package

234b646

Address review comments

7729562

Address review comments

1fe0048

Only set HTTP fs on the web handler if the UI is enabled

08b2a3c

Address review comments

2846d93

Properly tear down web session test by releasing resources at the end.

080592e

Fix the web UI assets configuration by removing DisableUI and instead use the presence of assets (HTTP file system) as an indicator that the web UI has been enabled.

Decrease the expired session cache clean up threshold to 2m. Only log…

9ce86d0

… the expiration error message for errors other than not found

Add switchback feature

e75b082

kimlisa requested review from a-palchikov, alex-kovoy, awly, fspmarshall, klizhentas, r0mant, russjones and webvictim as code owners January 30, 2021 01:23

a-palchikov reviewed Feb 1, 2021

View reviewed changes

kimlisa marked this pull request as draft February 1, 2021 20:04

a-palchikov force-pushed the dmitri/web-ui-disconnects branch 3 times, most recently from 2b8de2e to b82abfa Compare February 4, 2021 10:58

Base automatically changed from dmitri/web-ui-disconnects to master February 4, 2021 15:50

kimlisa closed this Apr 14, 2021

kimlisa deleted the lisa/role-switchback branch April 14, 2021 03:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add switchback feature #5440

Add switchback feature #5440

kimlisa commented Jan 30, 2021 •

edited

Loading

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

a-palchikov Feb 1, 2021

	// Test swtich back to default role and expiry.
	// Test switch back to default role and expiry.

Add switchback feature #5440

Add switchback feature #5440

Conversation

kimlisa commented Jan 30, 2021 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa commented Jan 30, 2021 •

edited

Loading