-
-
Notifications
You must be signed in to change notification settings - Fork 9
Improve login and cookie handling #76
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
base: main
Are you sure you want to change the base?
Conversation
| error($errors:OPERATION, "cookie:set: Cookie name and value must be set", ($name, $value)) | ||
| ) else | ||
| if (matches($name, $cookie:enforce-rfc2109)) then ( | ||
| error($errors:OPERATION, "cookie:set: Cookie name contains illegal charecters", $name) |
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.
characters
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 16 | ||
| node-version: 20 |
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.
lets do 22 (or 24) already
| declare variable $auth:DEFAULT_LOGIN_OPTIONS := map { | ||
| "asDba": true(), | ||
| "maxAge": xs:dayTimeDuration("P7D"), | ||
| "Path": request:get-context-path(), |
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.
inconsistent casing. can we make it consistent?
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.
Turns out, not, not really. Some options come from outside specifications.
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 love to come up with an easy use and easy to explain way for those.
| let $cookie-name := auth:read-cookie-name($request?spec) | ||
| return | ||
| if (empty($cookie-name)) then ( | ||
| error($errors:OPERATION, 'Cookie-name not specified in API-definition!') |
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.
The actual place where the cookie should be in the definition is components.securitySchemes.cookieAuth.name. Let's mention that in the error
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.
Yes, that's a great addition to the error message
| declare function auth:login-user ($user as xs:string, $password as xs:string, $options as map(*)) as xs:string? { | ||
| let $merged-options := | ||
| if (empty($options?name)) then ( | ||
| error($errors:OPERATION, 'Cookie-name not set in call to auth:login-user!') |
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.
Same here, error should mention components.securitySchemes.cookieAuth.name
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.
For this function that is actually not the case. It has to be in the options parameter. I will add this instead.
| ) else if ($merged-options?maxAge instance of xs:integer) then ( | ||
| xs:dayTimeDuration('PT' || $merged-options?maxAge || 'S') | ||
| ) else ( | ||
| error($errors:OPERATION, "the maxAge option value cannot be used", $merged-options?maxAge) |
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.
The maxAge option should either be an xs:dayTimeDuration or an xs:integer expressing the amount of seconds. The value YADAYADA cannot be used.
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.
What about
The value of
maxAgemust be of type xs:dayTimeDuration or xs:integer. The given value$merged-options?maxAgecannot be used
| declare function auth:logout-user ($options as map(*)) as empty-sequence() { | ||
| let $token := | ||
| if (empty($options?name)) then ( | ||
| error($errors:OPERATION, 'Cookie-name not set in call to auth:logout-user!') |
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.
components.securitySchemes.cookieAuth.name
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 error is repeated. Maybe make add
declare %private function auth:ensure-cookie-name (
$name as xs:string
) as xs:string {
if (exists($name)) then
$name
else
error(
$errors:OPERATION,
"The components.securitySchemes.cookieAuth.name is empty."
)
};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.
what about
declare %private function auth:ensure-cookie-name (
$name as xs:string?
) as xs:string {
head((
$name,
error(
$errors:OPERATION,
"The components.securitySchemes.cookieAuth.name is empty."
)
))
};- Use persistent login module directly to allow custom login and logout route handlers. - Logout route does not need to redirect anymore. - Add cookie.xqm utility module that is package private. - Add custom login and logout route handlers to test app. - Add and extend tests for login and logout.
When a route tries to call a handler function that does not exist, roaster will return with status code 500 and an actionable description: "Operation not found for operationId:<operationId>"
- use cookie-name everywhere and deprecate "login domain" - guard against cookie-name not being set
3b0ab4e to
fb33a35
Compare
|
@DrRataplan exciting news: It turns out all keys in cookies are case-insensitive. That means we could do all keys in lower case! source https://www.rfc-editor.org/rfc/rfc6265.html in Chapter 5.2. The Set-Cookie Header |
Please have a look at the documentation for the new feature:
https://github.com/eeditiones/roaster/blob/3b0ab4e867a21683bb00b93ab0be7809b6200887/doc/cookie-auth.md
FEATURES
HttpOnlyandSameSiteattributes on login domain cookies (implementation incontent/cookie.xqm)FIXES
logout=trueuser,password