[16.0][IMP]auth_jwt: Add the possibility to not renew the cookie in the responses#822
[16.0][IMP]auth_jwt: Add the possibility to not renew the cookie in the responses#822jesusVMayor wants to merge 1 commit intoOCA:16.0from
Conversation
|
Hi @sbidoul, |
jesusVMayor
force-pushed
the
16.0_auth_jwt_cookie_renew
branch
from
7e03ff2 to
255176e
Compare
|
Hi, I had not noticed #815 so far. Let's first discuss there. |
| default=True, help="Set to false only for development without https." | ||
| ) | ||
| renew_cookie_on_response = fields.Boolean( | ||
| help="Renew the cookie in every response", default=True |
There was a problem hiding this comment.
Expand the help text a bit to explain the risk of setting to True?
| "The sginature type should be secret if the cookie is " | ||
| "renewed on responses" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I don't think we should error in this case. Creating a http-only secure cookie once the client has been authenticated with the bearer token is a valid use case to me. If anything it makes the life of front-end developers easier (and more secure) as they don't need to worry with securely storing the JWT token, as the browser handles the cookie securely for them. I do agree with the caveat regarding session invalidation.
| _logger.error("database.secret system parameter is not set.") | ||
| raise ConfigurationError() | ||
| return secret | ||
| return self.secret_key |
There was a problem hiding this comment.
If the database secret is not adequate for your use case we could a new secret dedicated to signing/encrypting the cookie on the jwt validator record?
There was a problem hiding this comment.
Modified encode and decode functions to use the fields renew_cookie_secret and renew_cookie_algorithm
jesusVMayor
force-pushed
the
16.0_auth_jwt_cookie_renew
branch
from
255176e to
186faaf
Compare
| "way to invalidate sessions", | ||
| default=True, | ||
| ) | ||
| renew_cookie_secret = fields.Char() |
There was a problem hiding this comment.
| renew_cookie_secret = fields.Char() | |
| cookie_secret = fields.Char() |
This secret is not specific to whether the cookie lifetime is renewed or not.
There was a problem hiding this comment.
I see a relation with the renewal, if is not set, we dont want to change it.
For example, i have a domain.com who creates a cookie with the jwt for the domain, and i have odoo in odoo.domain.com, so the cookie will be shared between domain and subdomains. I want the validator to check that cookie against the secret/public key, not against that new secret, the cookie should only be checked against that secret if odoo creates its own cookie when the renew is checked.
There was a problem hiding this comment.
Yes that is your use case, but the two changes are logically independent. Deciding whether renewing the cookie automatically and how to sign it are two independent topics. Reasoning about it that way should make the change simpler while still supporting your use case I think.
| renew_cookie_algorithm = fields.Selection( | ||
| SECRET_ALGORITHM_SELECTION, | ||
| default="HS256", | ||
| ) |
There was a problem hiding this comment.
I'm not convinced we should propose an algorithm selection here.
For the JWT / bearer token, the algorithm must be agreed with the clients so an algorithm selection makes sense.
But here it's the server which protect the cookie for itself (the cookie is opaque to the clients), and we can let the server select the best algorithm.
|
Thanks for your work on this. Would you split this PR in two that we can discuss independently? As the cookie renewal and the cookie secret are two independent topics, it would be easier to review. |
| if not secret: | ||
| _logger.error("database.secret system parameter is not set.") | ||
| raise ConfigurationError() | ||
| return secret |
There was a problem hiding this comment.
I prefer keeping this method and return the validator cookie_secret if set else the database secret.
| raise UnauthorizedCompositeJwtError(exceptions) | ||
|
|
||
| if validator.cookie_enabled: | ||
| if validator.cookie_enabled and validator.renew_cookie_on_response: |
There was a problem hiding this comment.
This change does not seem necessary.
There was a problem hiding this comment.
i don't understand why, we need it to not renew the cookie.
| token = cls._get_cookie_token(validator.cookie_name) | ||
| assert token | ||
| return validator._decode(token, secret=validator._get_jwt_cookie_secret()) | ||
| return validator._decode(token, cookie_secret=True) |
| return jwks_client.get_signing_key(kid).key | ||
|
|
||
| def _encode(self, payload, secret, expire): | ||
| def _encode(self, payload, expire, secret=False): |
There was a problem hiding this comment.
I would not change this method. It is not necessary to change it if all the logic to get the secret is in _get_jwt_cookie_secret().
| "way to invalidate sessions", | ||
| default=True, | ||
| ) | ||
| renew_cookie_secret = fields.Char() |
There was a problem hiding this comment.
Yes that is your use case, but the two changes are logically independent. Deciding whether renewing the cookie automatically and how to sign it are two independent topics. Reasoning about it that way should make the change simpler while still supporting your use case I think.
|
Thanks for splitting the PR! |
3 participants
Fix the problems exposed in the issue #815