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: option to update session cookie expiry time on session refresh #2848

Closed
wants to merge 2 commits into from

Conversation

aarmam
Copy link
Contributor

@aarmam aarmam commented Nov 10, 2021

This pull request introduces feature to update session cookie expiry time on session refresh request.

Use case: We want to keep session duration quite short (15 minutes) and force client applications to periodically extend the session by performing authentication requests with prompt=none. Each subsequent authentication request produces a new identity token with lifetime of 15 minutes. But as a security measure we want that browser session cookie would not be kept alive any longer than necessary - therefore browser session cookie duration should be periodically extended, each time by 15 minutes (the same lifetime as each new identity token).

Current situation: Browser session cookie (oauth2_authentication_session) expiration is set from first acceptLoginRequest's remember_for value.
When performing subsequent session update requests (authentication requests with prompt=none), then browser session cookie expiration cannot be changed.

Proposed solution: Add refresh_remember_for parameter for PUT /oauth2/auth/requests/login/accept request body. When refresh_remember_for=true, session cookie expiry will be reset.

Related issue(s)

#1690
#1557
#2246

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Tests and documentation will be commited after inital acceptance of the proposed feature.

@aarmam aarmam requested a review from aeneasr as a code owner November 10, 2021 07:55
@aarmam aarmam marked this pull request as draft November 11, 2021 13:01
@aarmam aarmam force-pushed the feature/refresh-remember-for branch from 829dace to 219d381 Compare November 24, 2021 08:31
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #2848 (05b2183) into master (3ea014f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 05b2183 differs from pull request most recent head c8f5009. Consider uploading reports for the commit c8f5009 to get more accurate results

@@            Coverage Diff             @@
##           master    #2848      +/-   ##
==========================================
+ Coverage   76.74%   76.81%   +0.07%     
==========================================
  Files         123      123              
  Lines        9022     9071      +49     
==========================================
+ Hits         6924     6968      +44     
- Misses       1657     1660       +3     
- Partials      441      443       +2     
Impacted Files Coverage Δ
consent/types.go 78.12% <ø> (ø)
consent/strategy_default.go 69.59% <100.00%> (ø)
flow/flow.go 92.26% <100.00%> (-0.92%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aarmam aarmam force-pushed the feature/refresh-remember-for branch from 219d381 to a3899d7 Compare December 8, 2021 09:51
@aarmam aarmam force-pushed the feature/refresh-remember-for branch 2 times, most recently from c26c329 to a59aa69 Compare March 21, 2022 14:51
@aarmam aarmam force-pushed the feature/refresh-remember-for branch 2 times, most recently from 737f3b8 to 2e15430 Compare March 23, 2022 07:26
@aarmam aarmam marked this pull request as ready for review March 23, 2022 07:47
@aarmam aarmam force-pushed the feature/refresh-remember-for branch 2 times, most recently from 03cd8ce to b50aba5 Compare March 30, 2022 16:30
@aarmam aarmam force-pushed the feature/refresh-remember-for branch 2 times, most recently from d9a22b2 to 657981b Compare April 20, 2022 14:40
@aarmam aarmam force-pushed the feature/refresh-remember-for branch from 657981b to a584f09 Compare May 11, 2022 05:33
@aarmam aarmam marked this pull request as draft September 6, 2022 10:48
@aarmam aarmam force-pushed the feature/refresh-remember-for branch 4 times, most recently from c708b57 to 0d931c4 Compare September 14, 2022 13:06
@aarmam aarmam marked this pull request as ready for review September 14, 2022 13:46
@aarmam aarmam force-pushed the feature/refresh-remember-for branch 2 times, most recently from 70a1117 to 8f1b39c Compare November 4, 2022 10:56
@aarmam aarmam force-pushed the feature/refresh-remember-for branch from 7a0b09a to 0c146ea Compare December 21, 2022 16:59
@aarmam aarmam marked this pull request as draft December 21, 2022 17:47
@aarmam aarmam marked this pull request as ready for review December 21, 2022 17:47
@@ -265,6 +265,10 @@ type HandledLoginRequest struct {
// authorization will be remembered for the duration of the browser session (using a session cookie).
RememberFor int `json:"remember_for"`

// RefreshRememberFor, if set to true, session cookie expiry time will be updated when session is
// refreshed (login skip=true).
RefreshRememberFor bool `json:"refresh_remember_for"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with the existing values, it is better to name this property refresh_remember since it is a boolean value.

Existing props are following this format:

  • remember - boolean
  • remember_for - number

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, maybe even clearer extend_session_lifespan. Could also be a time value alternatively (although it would be conflicting with remember_for)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on vacation and will come back to this in few weeks! :)

@aeneasr aeneasr closed this in 7511436 Mar 16, 2023
harnash pushed a commit to Wikia/ory-hydra that referenced this pull request Apr 12, 2023
It is now possible to extend session lifespans when accepting login challenges.

Closes ory#1690
Closes ory#1557
Closes ory#2246
Closes ory#2848

Co-authored-by: Mart Aarma <mart.aarma@nortal.com>
Co-authored-by: Henning Perl <henning.perl@gmail.com>
Co-authored-by: ory-bot <60093411+ory-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants