Browser cookie sessions #1178
Browser cookie sessions #1178
Conversation
… refresh-sessions
… refresh-sessions
internal/auth/oauth2.go
Outdated
| @@ -84,6 +119,14 @@ func NewAuthenticationService(ctx context.Context, params AuthenticationServiceP | |||
| return nil, errors.E(op, errors.CodeServerConfiguration, err, "failed to create oidc provider") | |||
| } | |||
|
|
|||
| if params.SessionCookieMaxAge == 0 { | |||
There was a problem hiding this comment.
Just a thought but could an age of 0 maybe be useful if we want sessions or cookies that only last the lifetime of a session? For whatever reason.
There was a problem hiding this comment.
After a little ready I found another stackoverflow post:
Setting setMaxAge to 0 will delete the cookie. Setting it to -1 will preserve it until the browser is closed.
Not sure if this is 100%, but think we'll need to use 0 for deletion. So will remove this check.
… refresh-sessions
… refresh-sessions
|
Remove WIP from the PR title and commit messages and refactor the modifySession method, then i'll LGTM it :) |
| } | ||
| if sessionCookieExpiryInt < 0 { | ||
| return errors.E("jimm session cookie expiry cannot be less than 0") | ||
| if sessionCookieMaxAgeInt < 0 { |
There was a problem hiding this comment.
What about zero itself? Shouldn't this be <= 0?
There was a problem hiding this comment.
I guess if someone silly enough to put 0 and turn cookies off entirely it's kinda their own fault? :D
| @@ -125,13 +125,13 @@ func start(ctx context.Context, s *service.Service) error { | |||
| secureSessionCookies = true | |||
| } | |||
|
|
|||
| sessionCookieExpiry := os.Getenv("JIMM_SESSION_COOKIE_EXPIRY") | |||
| sessionCookieExpiryInt, err := strconv.Atoi(sessionCookieExpiry) | |||
| sessionCookieMaxAge := os.Getenv("JIMM_SESSION_COOKIE_MAX_AGE") | |||
There was a problem hiding this comment.
I see we're updating config but not updating the charms. Can we make a card to "Update JIMM charms with new config options"
| return context.WithValue(ctx, sessionIdentityContextKey{}, sessionIdentityId) | ||
| } | ||
|
|
||
| // SessionIdentityFromContext returns the session identity key from the context. |
There was a problem hiding this comment.
Feel free to ignore this one, just being pendantic - "returns the session identity value" right?
|
|
||
| if authErr != nil { | ||
| zapctx.Error(ctx, "browser authentication error", zap.Any("err", authErr), zap.Stack("stack")) | ||
| writeInternalServerErrorClosure(ctx, conn, authErr) |
There was a problem hiding this comment.
Interesting, why are we opting to return a websocket error instead of an HTTP error if we get an authErr?
There was a problem hiding this comment.
Can explain in call as other comment says
| } | ||
|
|
||
| if h.Server == nil { | ||
| writeNormalClosure(ctx, conn) |
There was a problem hiding this comment.
If h.Server == nil doesn't that constitute an error?
There was a problem hiding this comment.
Can explain this in call as had some issues basically with websocket tests
| // we need a dial websocket for juju containing a custom cookie jar to send cookies to | ||
| // a new server url when testing LoginWithSessionCookie. As such this closure simply | ||
| // passes the jar through. | ||
| func getDialWebsocketWithCustomCookieJar(jar *cookiejar.Jar) func(ctx context.Context, urlStr string, tlsConfig *tls.Config, ipAddr string) (jsoncodec.JSONConn, error) { |
Description
Implements browser sessions via cookies and persistent postgres storage. Also includes the facade method LoginWithSessionCookie.
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers