[keycloak-dev] Reset password and verify email links are to long

[Stian Thorgersen]

----- Original Message -----
> From: "Marek Posolda" <mposolda at redhat.com>
> To: "Stian Thorgersen" <stian at redhat.com>, "Bill Burke" <bburke at redhat.com>
> Cc: keycloak-dev at lists.jboss.org
> Sent: Thursday, 17 July, 2014 6:09:54 PM
> Subject: Re: [keycloak-dev] Reset password and verify email links are to long
> 
> On 17.7.2014 17:00, Stian Thorgersen wrote:
> > Just started on this. Quick summary of my attack plan:
> >
> > 1. Remove AccessToken from AccessCode. Instead add List<String>
> > rolesRequested and create AccessToken in TokenService.accessCodeToToken.
> > Any reasons why this wouldn't work?
> > 2. Move some info from AccessCode to UserSession, and remove anything
> > redundant. For example username used to login, auth-method (social or
> > form), remember-me.
> > 3. Add info from AccessCode to UserSessionProvider and send signed key
> > instead as the code query param.
> >
> > With regards to the required info here's what I've come up with:
> >
> > To verify the code:
> >
> > * Need to know it hasn't been done before - can either be done by having a
> > timestamp on ClientAssociation that is incremented every time a code is
> > requested/used, or by having a ClientAssociation per-code. As we'll need
> > to store more specific to a code than just the fact it has been used or
> > not, I think it's best to just have a ClientAssociation per-code
> > * Not expired - no prob
> > * Session active - no prob
> > * redirect_uri when retrieving code matches redirect_uri query param when
> > swapping for token - we don't do this currently I think, but spec requires
> > it. This would require a ClientAssociation per-code.
> > * Correct client_id - no prob, just make sure code belongs to the client_id
> > query param
> >
> > Then to create the token we need:
> >
> > * List of roles requested - the union of user roles and app/client scope.
> > This would require a ClientAssociation per-code as the role-mappings/scope
> > could change between user granted the roles to the client and the code is
> > exchanged. Also in the future once we add a scope query param this will be
> > required.
> Did you mean intersection rather than union?
> > * Realm - no prob, user-session is associated with a realm
> > * Client - no prob, ClientAssociation is associated with a client
> > * User - no prob, user-session is associated with a user
> > * Session - no prob, user-session is a session;)
> >
> > All in all it seems like a bit more work than I initially considered. Or am
> > I attacking this completely wrong?!?
> To me it makes sense and should work.
> 
> I wonder about a bit less-time consuming flow to implement, when
> AccessCode is generated together with AccessToken after authentication
> as it's now (in OAuthFlows.processAccessCode) . Only new thing in
> OAuthFlows.processAccessCode will be that ClientAssociation with just
> key-value pair of <signed-codeId>:<base64-encoded-AccessCode> will be
> associated with UserSession. Signed codeId will be then saved as "code"
> parameter .
> 
> Then once client will want to exchange code, Keycloak will perform
> lookup of correct UserSession based on codeId (new method on
> UserSessionProvider like: getSessionbyCode). Then when UserSession is
> successfully looked-up, ClientAssociation is immediatelly deleted, so
> that it's not possible to exchange same code twice. In
> TokenService.accessCodeToToken, we will still have AccessCode with
> wrapped AccessToken so we can do all verification is it's done now. Only
> new thing might be redirect_uri verification, which is not currently done.
> 
> Defacto it will be same as current flow, only new thing will be:
> - ClientAssociation with the pair
> <signed-codeId>:<base64-encoded-AccessCode>, created when code is
> generated and deleted when code is exchanged.
> - redirect_uri validation during exchange code to token.
> 
> I think that this approach will help us to address 2 main issues, which
> we currently have (long "code" parameter and same code is possible to
> exchange more times). Only thing is, that base64-encoded-AccessCode will
> be quite long and will contain redundant info, but is it big problem? If
> we want later add new info to AccessCode, we don't need to do any
> changes in UserSessionProvider model as it will be still just base64
> encoded string.
> 
> Not sure which approach is better, just a though:-)

I think that's still a fair amount of work, but a bit of a "hack".

With regards to changes to UserSessionProvider that doesn't bother me much. I'd say that's more an internal SPI, so doesn't have to be as stable, also upgrading should be trivial as we don't really care about existing data when upgrading to a new KC version. 

> 
> Marek
>