-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor auth middleware for supporting bearer, cookie and query #560
Conversation
src/controller/middleware/auth.rs
Outdated
pub fn extract_token_from_cookie(parts: &Parts) -> eyre::Result<String> { | ||
let jar = cookie::CookieJar::from_headers(&parts.headers); | ||
Ok(jar | ||
.get("token") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would create some kind of unique Loco session ID key name.
in Rails it is: _APPNAME_session
and is also configurable:
config.session_store :cookie_store, key: '_somethingelse_session'
so we might need to reach into AppContext
and then configuration for the cookie to grab the name of the cookie, and then when generating a new app give it a default name of _APP_NAME_loco-session
(naming it loco-session
to avoid conflict with rails)
src/controller/middleware/auth.rs
Outdated
/// # Errors | ||
/// when token value from cookie is not found | ||
pub fn extract_token_from_cookie(parts: &Parts) -> eyre::Result<String> { | ||
let jar = cookie::CookieJar::from_headers(&parts.headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if possible, to take only httponly
cookie or else put out an error. this will forcefully educate users to do the right thing
src/controller/middleware/auth.rs
Outdated
/// | ||
/// # Errors | ||
/// when token value from cookie is not found | ||
pub fn extract_token_from_cookie(parts: &Parts) -> eyre::Result<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure we want eyre::Result
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function returns an error if the token cannot strip the value to get only the token.
I prefer not to return an Option and return an error with a message that explains where the error came from.
you prefer Result<String, String> instead of eyre::Result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
Extract JWT token from cookie