Fix zmartzone/lua-resty-openidc#157 : Update Id token in session afte…

[grrolland]

This pull request update the id token in session after refreshing tokens with token endpoint. Finally, the content of the session is coherent (access token, refresh token, id token).

[bodewig]

Thank you.

Should we remove an existing id token from the session if the token response after refresh does not contain a new id token? Section 12.2 of the OIDC Core Spec says

Upon successful validation of the Refresh Token, the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token.

[zandbelt]

I don't think an existing id_token should be removed:

• firstly I believe there are many OPs out there that don't return a new id_token in a refresh flow simply because the regular semantics of an id_token ("user just logged in") do not hold
• secondly the id_token's lifetime usually does not impact the validity of the claims for the user so there's no reason to discard those claims; it also does usually not correspond to the application session lifetime

[bodewig]

I agree, in that case the PR looks good to me.

[zandbelt]

It would be great to include a test for it... (never thought I would say that ;-))

[bodewig]

Unless @grrolland wants to take a stab at test I can try to carve out some time at the weekend.

[bodewig]

I've pushed the PR, a small refactoring (please have a look at 11a9f26 @grrolland ) and tests to the pr-158 branch.

While thinking through the case where the token endpoint returns an invalid id token on refresh I feel the handling is inconsistent right now. openidc_access_token (correctly IMHO) returns an error in that case but we keep the access token stored inside the session. It seems we should remove all tokens from the session if we want to discard the token endpoint's response.

[zandbelt]

I don't see that last implication: whatever access token is returned from the token endpoint may be stored in the session; even if there's an error in a subsequent call to the token endpoint, we can still choose to preserve the old one under the assumption "if it still valid it is ok otherwise we'll find out sooner or later"

[bodewig]

As the current code stands the old access token has been replaced by the one that was returned from the token endpoint inside of the session. The inconsistency I see is that we don't return the access token because id token verification failed but we still store it inside the session. I'd be fine with either removing the access token completely or keeping the old one, but the current state feels wrong to me.

[zandbelt]

Ah right, I misunderstood at first but agree with you. I'd like to store the new access token.

[bodewig]

What would be your preferred option, remove clear the tokens completely or restore the old ones?

I don't think the old ones will be useful as the refresh token has already been used by now and the access token has likely expired, so would opt for clearing the tokens.

[zandbelt]

Yeah, you're right: the reason for refreshing should be that the client can't use the existing access token so there should be no reason to keep it regardless of the result. That is different from the id_token.

[bodewig]

OK, I've changed the branch, see a968e76

[zandbelt]

So now the id_token is also set to nil where it doesn't need to #158 (comment)

[bodewig]

Ah, thanks, you are right. 551564f

[zandbelt]

the log message "ngx.log(ngx.ERR, "invalid id token, discaring refreshed id token")
" is off (+ a spelling error)

[bodewig]

Not sure I know what you mean with "off", see b27009f

Unfortunately I've got some second thoughts again, sorry for that. Wouldn't it be cleaner if we verified an id_token returned while refreshing regardless of whether we are going to store it inside the session? In that case we could treat it like a "normal" token refresh error and simply leave the whole session alone (see https://github.com/zmartzone/lua-resty-openidc/blob/master/lib/resty/openidc.lua#L1090 ).

[bodewig]

regarding those latest thoughts see 13516c4 in yet another branch that I'll need to clean up once we are happy :-)

[bodewig]

merged, thanks @grrolland !