fix(cookies): add {cookie-name}-parts cookie, style: inline formatting
#107
Conversation
| .get_cookie(&format!("{cookie_name}-parts")) | ||
| .unwrap_or_default() | ||
| .parse() | ||
| .map_err(|_| PluginError::SessionCookieNotFoundError)?; |
There was a problem hiding this comment.
Technically its not the Session Cookie right? We should add another Error for this type as Nonce also has its own error
There was a problem hiding this comment.
I don't think this is worth the effort. I would consider all of the cookies together "the session cookie" because it doesn't really make a difference to the user which is missing, the end result is the same. In fact, I would like to refactor the error handling completely to use anyhow instead of defining a custom error type with thiserror. Initially I thought we might match on the error variant somewhere and do different things depending on the error, but we don't actually do that. Furthermore, PluginError is just a catch-all error type anyway, that doesn't reflect which errors a function can actually throw. fn validate_cookie(&self) -> Result<AuthorizationState, PluginError> will never return Err(PluginError::CodeNotFoundInCallbackError), but the return type makes it look like it could.
| @@ -109,22 +108,17 @@ impl Session { | |||
| cookie_values.push(cookie_value); | |||
| } | |||
|
|
|||
| let num_parts = cookie_values.len(); | |||
There was a problem hiding this comment.
Maybe add a comment which explains what you are doing here :)
this avoids problems with leftover cookie parts preventing decryption
antonengelhardt
force-pushed
the
cookie-fix
branch
from
6b14989 to
942ca79
Compare
antonengelhardt
changed the title
{cookie-name}-parts} cookie, style: inline formatting
antonengelhardt
changed the title
{cookie-name}-parts} cookie, style: inline formatting{cookie-name}-parts cookie, style: inline formatting
antonengelhardt
merged commit a6ea31b
into
mulitple-open-id-providers
Please describe your changes and why you made them
This fixes an issue where old cookie parts can be left over, preventing decryption
Does this PR introduce a breaking change?
Nah
Other information and Screenshots (if appropriate)