Currently OIDC tokens are stored and checked in addition to the auth cookie.
This is not how OIDC tokens are supposed to work though. OIDC should work in one of the two ways:
- OIDC is simply used for identifying the user and then the application takes over the session management
- The application uses the session management of the OIDC provider
Currently neither is implemented properly.
Proper Implementation of Option 1
To implement Option 1 the OIDC access token should not be stored and thus the validity/expiration should not be checked.
This would mean the following lines need to be removed:
|
// clear any existing id token on the session, just because |
|
routeutils.ClearOidcIdTokenPayload(r, w) |
|
routeutils.SetOidcIdTokenPayload(idTokenPayload, r, w) // save to session, only used by middleware for automatic redirection upon expiry |
|
if m.tryHandleOidc(w, r) { |
|
// user has expired oidc token, thus is redirected to provider and will come back to callback endpoint |
|
// notably, if user does have a valid, non-expired id token, they will also have a valid auth cookie, so proceed as usual |
|
return |
|
} |
|
// redirect if oidc id token was found, but expired |
|
// returns true if further authentication can be skipped |
|
func (m *AuthenticateMiddleware) tryHandleOidc(w http.ResponseWriter, r *http.Request) bool { |
|
idToken := routeutils.GetOidcIdTokenPayload(r) |
|
if idToken == nil { |
|
return false |
|
} |
|
|
|
if !idToken.IsValid() { // expired |
|
provider, err := m.config.Security.GetOidcProvider(idToken.ProviderName) |
|
if err != nil { |
|
conf.Log().Request(r).Error("failed to get provider from id token", "provider", idToken.ProviderName, "sub", idToken.Subject) |
|
return false |
|
} |
|
|
|
if _, err := m.userSrvc.GetUserByOidc(provider.Name, idToken.Subject); err != nil { |
|
conf.Log().Request(r).Error("got expired oidc token for non-oidc user", "provider", idToken.ProviderName, "sub", idToken.Subject) |
|
return false |
|
} |
|
|
|
state := routeutils.SetNewOidcState(r, w) |
|
http.Redirect(w, r, provider.OAuth2.AuthCodeURL(state), http.StatusFound) |
|
return true |
|
} |
|
|
|
return false |
|
} |
Proper Implementation of Option 2
To implement Option 2 users authenticated with OIDC should not receive an authentication cookie. Instead they receive the OIDC access token and importantly the refresh token as well. (How they will be stored is an implementation detail, but separate cookies for both could work, importantly both do not need to be stored in encrypted/authenticated cookies)
Now to authenticate a logged in user the auth should work like this:
- Check if the auth_cookie is set
- if so use the existing flow
- Check if the OIDC cookies are present
- Get the access token and validate it
- if its valid and not expired, continue
- if its expired, try to get another token with the refresh token
- if that succeeds, set the cookie and continue
- redirect to the authentication provider
- fetch the user using the access token (ie via the subject field)
Currently OIDC tokens are stored and checked in addition to the auth cookie.
This is not how OIDC tokens are supposed to work though. OIDC should work in one of the two ways:
Currently neither is implemented properly.
Proper Implementation of Option 1
To implement Option 1 the OIDC access token should not be stored and thus the validity/expiration should not be checked.
This would mean the following lines need to be removed:
wakapi/routes/login.go
Lines 417 to 418 in 26c2e4e
wakapi/routes/login.go
Line 499 in 26c2e4e
wakapi/middlewares/authenticate.go
Lines 84 to 88 in 26c2e4e
wakapi/middlewares/authenticate.go
Lines 224 to 250 in 26c2e4e
Proper Implementation of Option 2
To implement Option 2 users authenticated with OIDC should not receive an authentication cookie. Instead they receive the OIDC access token and importantly the refresh token as well. (How they will be stored is an implementation detail, but separate cookies for both could work, importantly both do not need to be stored in encrypted/authenticated cookies)
Now to authenticate a logged in user the auth should work like this: