-
-
Couldn't load subscription status.
- Fork 1.4k
Set Default Auth Session Length #3144
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
|
Thanks for the PR @Case-E , we will review it soon! |
|
Related issue with the discussion: #2433 . |
Signed-off-by: Mihovil Ilakovac <[email protected]>
|
Hey @Case-E thank you for contributing! I've tried running the tests locally and found some obvious issues - so it's good to have the project set up locally to be able to run Here's my commit I'll review the PR now and leave any comment I might have 👍 |
| onBeforeLogin :: Maybe ExtImport, | ||
| onAfterLogin :: Maybe ExtImport | ||
| onAfterLogin :: Maybe ExtImport, | ||
| sessionLength :: Maybe Integer |
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'd change the name from sessionLength to something you already have in other places - sessionExpiresInMs and I'd update all the places where this values is talked about to use the exact same name.
|
|
||
| userEntityName = AS.refName $ AS.Auth.userEntity auth | ||
| -- Default to 30 days in milliseconds if not specified | ||
| sessionExpiresInMs = maybe (30 * 24 * 60 * 60 * 1000) (* 1000) $ AS.Auth.sessionLength auth |
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.
In this line we implicitly defined that sessionLength is in seconds which wasn't obvious from the name. (* 1000) function told me that the value will be multiple by 1000 and then I'll get the value in ms.
I guess having sub-second session length doesn't really make any practical sense so the name might as well be sessionLengthSeconds and then we can adjust the TimeSpan to use the "s" instead of "ms" unit in lucia.ts file.
I'd probably define this like:
sessionExpiresInSeconds = fromMaybe defaultSessionDurationSeconds $ AS.Auth.sessionExpiresInSeconds auth
defaultSessionExpiresInSeconds = 30 * dayInSeconds
dayInSeconds = 24 * 60 * 60 * 1000| // sameSite: "lax", | ||
| // }, | ||
| // }, | ||
| sessionExpiresIn: new TimeSpan({= sessionExpiresInMs =}, "ms"), |
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.
Lucia library expects a TimeSpan when defined the session length.
|
Thanks for taking a look and all the comments/work here @infomiho! This was a draft exploratory PR that I wasn't sure if I was gonna complete, however because there's not much left to do here, I'll see if I can find sometime to wrap this up over the next couple of weeks. |
Description
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects
waspc, make sure you satisfy the following:ChangeLog.mdwith description of the change this PR introduces.waspcversion inwaspc.cabalto reflect changes I introduced, with regards to the version of the latest wasp release, if the bump was needed.Add a regression test if needed
If you did a bug fix, make sure you satisfy the following:
If you're unable to add a regression test, please explain why.
This likely indicates that our current testing setup needs improvement.
Update example apps if needed
If you did code changes and added a new feature, make sure you satisfy the following:
waspc/examples/todoAppand its e2e tests as needed and manually checked it works correctly.If you did code changes and updated an existing feature, make sure you satisfy the following:
waspc/examples/todoAppand its e2e tests as needed and manually checked it works correctly.Update starter apps if needed
If you did code changes and updated an existing feature, make sure you satisfy the following:
basicstarter as needed and manually checked it works correctly.todo-tsstarter as needed and manually checked it works correctly.embeddingsstarter as needed and manually checked it works correctly.saasstarter as needed and manually checked it works correctly.Update e2e tests if needed
If you did code changes and changed Wasp's code generation logic, make sure you satisfy the following: