-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Browser cookie sessions #1178
Browser cookie sessions #1178
Changes from all commits
7fc476c
4bb9df3
131ac99
56784b9
2f55e01
1c0610d
b9cb2ed
a6cb504
e9475ae
4c07c2a
dd76d9e
c8a42ea
0d5f321
2b17654
f730268
2efd6c7
6de58e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
sessionCookieMaxAgeInt, err := strconv.Atoi(sessionCookieMaxAge) | ||
if err != nil { | ||
return errors.E("unable to parse jimm session cookie expiry") | ||
return errors.E("unable to parse jimm session cookie max age") | ||
} | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about zero itself? Shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess if someone silly enough to put 0 and turn cookies off entirely it's kinda their own fault? :D |
||
return errors.E("jimm session cookie max age cannot be less than 0") | ||
} | ||
|
||
jimmsvc, err := jimm.NewService(ctx, jimm.Params{ | ||
|
@@ -159,15 +159,15 @@ func start(ctx context.Context, s *service.Service) error { | |
JWTExpiryDuration: jwtExpiryDuration, | ||
InsecureSecretStorage: insecureSecretStorage, | ||
OAuthAuthenticatorParams: jimm.OAuthAuthenticatorParams{ | ||
IssuerURL: issuerURL, | ||
ClientID: clientID, | ||
ClientSecret: clientSecret, | ||
Scopes: scopesParsed, | ||
SessionTokenExpiry: sessionTokenExpiryDuration, | ||
IssuerURL: issuerURL, | ||
ClientID: clientID, | ||
ClientSecret: clientSecret, | ||
Scopes: scopesParsed, | ||
SessionTokenExpiry: sessionTokenExpiryDuration, | ||
SessionCookieMaxAge: sessionCookieMaxAgeInt, | ||
}, | ||
DashboardFinalRedirectURL: os.Getenv("JIMM_DASHBOARD_FINAL_REDIRECT_URL"), | ||
SecureSessionCookies: secureSessionCookies, | ||
SessionCookieExpiry: sessionCookieExpiryInt, | ||
}) | ||
if err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,11 +12,14 @@ import ( | |
"context" | ||
"encoding/base64" | ||
stderrors "errors" | ||
"fmt" | ||
"net/http" | ||
"net/mail" | ||
"strings" | ||
"time" | ||
|
||
"github.com/coreos/go-oidc/v3/oidc" | ||
"github.com/gorilla/sessions" | ||
"github.com/juju/zaputil/zapctx" | ||
"github.com/lestrrat-go/jwx/v2/jwa" | ||
"github.com/lestrrat-go/jwx/v2/jwt" | ||
|
@@ -28,6 +31,31 @@ import ( | |
"github.com/canonical/jimm/internal/errors" | ||
) | ||
|
||
const ( | ||
// SessionName is the name of the gorilla session and is used to retrieve | ||
// the session object from the database. | ||
SessionName = "jimm-browser-session" | ||
|
||
// SessionIdentityKey is the key for the identity value stored within the | ||
// session. | ||
SessionIdentityKey = "identity-id" | ||
) | ||
|
||
type sessionIdentityContextKey struct{} | ||
|
||
func contextWithSessionIdentity(ctx context.Context, sessionIdentityId any) context.Context { | ||
return context.WithValue(ctx, sessionIdentityContextKey{}, sessionIdentityId) | ||
} | ||
|
||
// SessionIdentityFromContext returns the session identity key from the context. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to ignore this one, just being pendantic - "returns the session identity value" right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep you're right |
||
func SessionIdentityFromContext(ctx context.Context) string { | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
v := ctx.Value(sessionIdentityContextKey{}) | ||
if v == nil { | ||
return "" | ||
} | ||
return v.(string) | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// AuthenticationService handles authentication within JIMM. | ||
type AuthenticationService struct { | ||
oauthConfig oauth2.Config | ||
|
@@ -37,7 +65,12 @@ type AuthenticationService struct { | |
// sessionTokenExpiry holds the expiry time for JIMM minted session tokens (JWTs). | ||
sessionTokenExpiry time.Duration | ||
|
||
// sessionCookieMaxAge holds the max age for session cookies. | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sessionCookieMaxAge int | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
db IdentityStore | ||
|
||
sessionStore sessions.Store | ||
} | ||
|
||
// Identity store holds the necessary methods to get and update an identity | ||
|
@@ -62,6 +95,8 @@ type AuthenticationServiceParams struct { | |
Scopes []string | ||
// SessionTokenExpiry holds the expiry time of minted JIMM session tokens (JWTs). | ||
SessionTokenExpiry time.Duration | ||
// SessionCookieMaxAge holds the max age for session cookies. | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
SessionCookieMaxAge int | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// RedirectURL is the URL for handling the exchange of authorisation | ||
// codes into access tokens (and id tokens), for JIMM, this is expected | ||
// to be the servers own callback endpoint registered under /auth/callback. | ||
|
@@ -71,6 +106,9 @@ type AuthenticationServiceParams struct { | |
// to fetch and update identities. I.e., their access tokens, refresh tokens, | ||
// display name, etc. | ||
Store IdentityStore | ||
|
||
// SessionStore holds the store for creating, getting and saving gorrila sessions. | ||
SessionStore sessions.Store | ||
} | ||
|
||
// NewAuthenticationService returns a new authentication service for handling | ||
|
@@ -93,8 +131,10 @@ func NewAuthenticationService(ctx context.Context, params AuthenticationServiceP | |
Scopes: params.Scopes, | ||
RedirectURL: params.RedirectURL, | ||
}, | ||
sessionTokenExpiry: params.SessionTokenExpiry, | ||
db: params.Store, | ||
sessionTokenExpiry: params.SessionTokenExpiry, | ||
db: params.Store, | ||
sessionStore: params.SessionStore, | ||
sessionCookieMaxAge: params.SessionCookieMaxAge, | ||
}, nil | ||
} | ||
|
||
|
@@ -277,6 +317,8 @@ func (as *AuthenticationService) UpdateIdentity(ctx context.Context, email strin | |
|
||
u.AccessToken = token.AccessToken | ||
u.RefreshToken = token.RefreshToken | ||
u.AccessTokenExpiry = token.Expiry | ||
u.AccessTokenType = token.TokenType | ||
if err := db.UpdateIdentity(ctx, u); err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
@@ -335,3 +377,156 @@ func (as *AuthenticationService) VerifyClientCredentials(ctx context.Context, cl | |
} | ||
return nil | ||
} | ||
|
||
// CreateBrowserSession creates a session and updates the cookie for a browser | ||
// login callback. | ||
func (as *AuthenticationService) CreateBrowserSession( | ||
ctx context.Context, | ||
w http.ResponseWriter, | ||
r *http.Request, | ||
secureCookies bool, | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
email string, | ||
) error { | ||
const op = errors.Op("auth.AuthenticationService.CreateBrowserSession") | ||
|
||
session, err := as.sessionStore.Get(r, SessionName) | ||
if err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
session.IsNew = true // Sets cookie to a fresh new cookie | ||
session.Options.MaxAge = as.sessionCookieMaxAge // Expiry in seconds | ||
session.Options.Secure = secureCookies // Ensures only sent with HTTPS | ||
session.Options.HttpOnly = false // Allow Javascript to read it | ||
|
||
session.Values[SessionIdentityKey] = email | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err = session.Save(r, w); err != nil { | ||
return errors.E(op, err) | ||
} | ||
return nil | ||
} | ||
|
||
// AuthenticateBrowserSession updates the session for a browser, additionally | ||
// retrieving new access tokens upon expiry. If this cannot be done, the cookie | ||
// is deleted and an error is returned. | ||
func (as *AuthenticationService) AuthenticateBrowserSession(ctx context.Context, w http.ResponseWriter, req *http.Request) (context.Context, error) { | ||
const op = errors.Op("auth.AuthenticationService.AuthenticateBrowserSession") | ||
|
||
session, err := as.sessionStore.Get(req, SessionName) | ||
if err != nil { | ||
return ctx, errors.E(op, err, "failed to retrieve session") | ||
} | ||
|
||
identityId, ok := session.Values[SessionIdentityKey] | ||
if !ok { | ||
return ctx, errors.E(op, "session is missing identity key") | ||
} | ||
|
||
err = as.validateAndUpdateAccessToken(ctx, identityId) | ||
if err != nil { | ||
if err := as.deleteSession(session, w, req); err != nil { | ||
return ctx, errors.E(op, err) | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return ctx, errors.E(op, err) | ||
} | ||
|
||
ctx = contextWithSessionIdentity(ctx, identityId) | ||
|
||
if err := as.extendSession(session, w, req); err != nil { | ||
return ctx, errors.E(op, err) | ||
} | ||
|
||
return ctx, nil | ||
} | ||
|
||
// validateAndUpdateAccessToken validates the access tokens expiry, and if it cannot, then | ||
// it attempts to refresh the access token. | ||
func (as *AuthenticationService) validateAndUpdateAccessToken(ctx context.Context, email any) error { | ||
const op = errors.Op("auth.AuthenticationService.validateAndUpdateAccessToken") | ||
|
||
emailStr, ok := email.(string) | ||
if !ok { | ||
return errors.E(op, fmt.Sprintf("failed to cast email: got %T, expected %T", email, emailStr)) | ||
} | ||
|
||
db := as.db | ||
u := &dbmodel.Identity{ | ||
Name: emailStr, | ||
} | ||
if err := db.GetIdentity(ctx, u); err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
t := &oauth2.Token{ | ||
AccessToken: u.AccessToken, | ||
RefreshToken: u.RefreshToken, | ||
Expiry: u.AccessTokenExpiry, | ||
TokenType: u.AccessTokenType, | ||
} | ||
|
||
// Valid simply checks the expiry, if the token isn't valid, | ||
// we attempt to refresh the identities tokens and update them. | ||
if t.Valid() { | ||
ale8k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return nil | ||
} | ||
|
||
if err := as.refreshIdentitiesToken(ctx, emailStr, t); err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// refreshIdentitiesToken creates a token source based on the expired token and performs | ||
// a manual token refresh, updating the identity afterwards. | ||
// | ||
// This is to be called only when a token is expired. | ||
func (as *AuthenticationService) refreshIdentitiesToken(ctx context.Context, email string, t *oauth2.Token) error { | ||
const op = errors.Op("auth.AuthenticationService.refreshIdentitiesToken") | ||
|
||
tSrc := as.oauthConfig.TokenSource(ctx, t) | ||
|
||
// Get a new access and refresh token (token source only has Token()) | ||
newToken, err := tSrc.Token() | ||
if err != nil { | ||
return errors.E(op, err, "failed to refresh token") | ||
} | ||
|
||
if err := as.UpdateIdentity(ctx, email, newToken); err != nil { | ||
return errors.E(op, err, "failed to update identity") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (as *AuthenticationService) deleteSession(session *sessions.Session, w http.ResponseWriter, req *http.Request) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems deleteSession and extendSession are doing pretty much the same thing.. both could call something like
with different values for maxAge.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer the names as it's easier on the eye when reading the code, I like self-documenting code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please reduce the line count.. have one method, as i suggested.. then two separate methods, deleteSession and extendSession, that call the modifySession method.. reduces line count, everybody happy |
||
const op = errors.Op("auth.AuthenticationService.deleteSession") | ||
|
||
if err := as.modifySession(session, w, req, -1); err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (as *AuthenticationService) extendSession(session *sessions.Session, w http.ResponseWriter, req *http.Request) error { | ||
const op = errors.Op("auth.AuthenticationService.extendSession") | ||
|
||
if err := as.modifySession(session, w, req, as.sessionCookieMaxAge); err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (as *AuthenticationService) modifySession(session *sessions.Session, w http.ResponseWriter, req *http.Request, maxAge int) error { | ||
const op = errors.Op("auth.AuthenticationService.modifySession") | ||
|
||
session.Options.MaxAge = maxAge | ||
|
||
if err := session.Save(req, w); err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we're updating config but not updating the charms. Can we make a card to "Update JIMM charms with new config options"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!