Skip to content
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

feat: remember me #1661

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Mujhtech
Copy link

@Mujhtech Mujhtech commented Oct 2, 2024

Description

Fixes #1656

Implementation

Tests

Todos

Additional context

@FlxMgdnz
Copy link
Member

@Mujhtech what's the status on this?

@Mujhtech
Copy link
Author

Hi @FlxMgdnz, I forgot to drop the comments here but can you check this comment?

#1656 (comment)
#1656 (comment)

@lfleischmann
Copy link
Member

Hi @FlxMgdnz, I forgot to drop the comments here but can you check this comment?

#1656 (comment) #1656 (comment)

Hi @Mujhtech,

regarding your first comment: yes, that would be a way to do it
regarding your second comment: I'm not entirely sure what you're getting at. Are you referring to the browser's behaviour when making the cookie a session cookie? If so, I don't see what one could do about it? How would the js-cookie library play into this? Can you update the PR, so we can check it out for ourselves?

This commit adds the remember me functionality to the login element. It includes the following changes:
- In the `config_default.go` file, the `Session` struct now has a new field `EnableRememberMe` set to `false`.
- A new file `action_remember_me.go` is added to the `credential_usage` package, which implements the remember me action.
- The `flows.go` file in the `flow` package now includes the `RememberMe` action in the `CredentialUsageSubFlow`.
- Two new constants `ActionRememberMe` and `StashPathRememberMe` are added to the `const_action_names.go` and `const_stash_paths.go` files respectively.
- The `LoginInitPage.tsx` and `LoginPasswordPage.tsx` files in the `pages` directory are modified to include a remember me checkbox.
@Mujhtech
Copy link
Author

Mujhtech commented Oct 17, 2024

If you check Cookie.ts file in frontend sdk, it use js-cookie to store the authentication token which I believe js-cookie makes use of session cookie, only hanko-session is stored in browser local storage

setAuthCookie(token: string, options?: CookieAttributes) {

@lfleischmann

@lfleischmann
Copy link
Member

lfleischmann commented Oct 17, 2024

If you check Cookie.ts file in frontend sdk, it use js-cookie to store the authentication token which I believe js-cookie makes use of session cookie, only hanko-session is stored in browser local storage

setAuthCookie(token: string, options?: CookieAttributes) {

@lfleischmann

In the current implementation, the code that calls this method (HttpClient.processHeaders) also provides an expiry, so it is not a session cookie. Calling the method without an expiry (or max age) would make it a session cookie.

Also btw.: Cookies can also be set in the backend if the relying party frontend/client runs on the same domain as the Hanko backend. So these would have to be adjusted, too.

@Mujhtech

@Mujhtech
Copy link
Author

OK, I will update the the logic, as that's the only thing remaining

@lfleischmann

@Mujhtech Mujhtech marked this pull request as ready for review October 18, 2024 16:35
@Mujhtech
Copy link
Author

@lfleischmann ready for review

@FlxMgdnz
Copy link
Member

/award 750 Thanks @Mujhtech we'll take it from here

Copy link

oss-gg bot commented Oct 29, 2024

Awarding Mujhtech: 750 points 🕹️ Well done! Check out your new contribution on oss.gg/Mujhtech

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

Remember me
3 participants