feat: Add session renew capabilities, #615 #2146

abador · 2022-01-14T12:43:55Z

Add session renew to kratos:

whoami
renew by session id
renew by current session

Related issue(s)

#615

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

It's a follow-up after this pr on our repo: Wikia#48

aeneasr

Great job! Just a few notes :) Could you please also add a guide to the docs how to use this feature?

driver/config/config.go

internal/testhelpers/handler_mock.go

driver/config/config.go

session/session.go

abador · 2022-01-18T10:01:15Z

@aeneasr tests are failing, but this doesn't seem to be caused by my changes(except for prettier). More docs added in here

aeneasr · 2022-01-21T15:29:40Z

It looks like there's a lint issue: https://app.circleci.com/pipelines/github/ory/kratos/5952/workflows/0452509a-af7c-4dc4-88ac-87c189d19b29/jobs/31412 :)

session/handler_test.go

session/handler.go

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

session/handler.go

aeneasr

Thank you! This is looking grand! I just have two minor things regarding the API route structure :)

aeneasr · 2022-02-03T16:22:51Z

session/handler.go

+	ID string `json:"id"`
+}
+
+// swagger:route PATCH /sessions/refresh/{id} v0alpha2 adminSessionRefresh


We try to have structure with REST resource model, so this would be:

Suggested change

// swagger:route PATCH /sessions/refresh/{id} v0alpha2 adminSessionRefresh

// swagger:route PATCH /sessions/{id}/refresh v0alpha2 adminRefreshSession

or

Suggested change

// swagger:route PATCH /sessions/refresh/{id} v0alpha2 adminSessionRefresh

// swagger:route PATCH /sessions/{id}?refresh=true v0alpha2 adminRefreshSession

aeneasr · 2022-02-03T16:23:16Z

session/handler.go

+// Calling this endpoint refreshes a given session.
+// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.


Suggested change

// Calling this endpoint refreshes a given session.

// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.

// Calling this endpoint refreshes the given session ID.

// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.

aeneasr · 2022-02-03T16:23:29Z

session/handler.go

+// This endpoint is useful for:
+//
+// - Session refresh


Suggested change

// This endpoint is useful for:

//

// - Session refresh

aeneasr · 2022-02-03T16:26:48Z

session/handler.go

+// swagger:route PATCH /sessions/refresh v0alpha2 adminCurrentSessionRefresh
+//
+// Calling this endpoint refreshes a given session.
+// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.
+//
+// This endpoint is useful for:
+//
+// - Session refresh
+//
+//     Schemes: http, https
+//
+//     Security:
+//       oryAccessToken:
+//
+//     Responses:


I think this route would currently cause a conflict with the other session routes? I would suggest:

Suggested change

// swagger:route PATCH /sessions/refresh v0alpha2 adminCurrentSessionRefresh

//

// Calling this endpoint refreshes a given session.

// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.

//

// This endpoint is useful for:

//

// - Session refresh

//

// Schemes: http, https

//

// Security:

// oryAccessToken:

//

// Responses:

// swagger:route PATCH /sessions?refresh=true v0alpha2 adminRefreshSessionFromCredentials

//

// Calling this endpoint you can refresh an Ory Session Cookie or Ory Session Token. You must

// include the Ory Session Token in the `Cookie` header or the Ory Session Token in the `X-Session-Token`

// header.

//

// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.

//

// Schemes: http, https

//

// Security:

// oryAccessToken:

//

// Responses:

Please also add these keys (x-session-token, cookie) to the argument this method can receive (see the whoami endpoint for an example) :)

hero101 · 2022-02-18T16:00:46Z

What is the status of this PR - is it stale? We are also investigating this and are interested in it.

aeneasr · 2022-02-18T19:12:44Z

If you want to pick this up please feel free to do so! :)

codecov · 2022-03-28T09:27:39Z

Codecov Report

Merging #2146 (890765b) into master (b5b1361) will decrease coverage by 0.02%.
The diff coverage is 91.89%.

❗ Current head 890765b differs from pull request most recent head 60fab42. Consider uploading reports for the commit 60fab42 to get more accurate results

@@            Coverage Diff             @@
##           master    #2146      +/-   ##
==========================================
- Coverage   76.62%   76.59%   -0.03%     
==========================================
  Files         318      318              
  Lines       17190    17217      +27     
==========================================
+ Hits        13171    13187      +16     
- Misses       3087     3097      +10     
- Partials      932      933       +1

Impacted Files	Coverage Δ
session/handler.go	`71.34% <87.50%> (+1.27%)`	⬆️
driver/config/config.go	`81.75% <100.00%> (+0.06%)`	⬆️
internal/testhelpers/handler_mock.go	`91.01% <100.00%> (+0.31%)`	⬆️
session/session.go	`71.11% <100.00%> (-10.07%)`	⬇️
persistence/sql/persister_courier.go	`88.33% <0.00%> (+3.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5b1361...60fab42. Read the comment docs.

aeneasr · 2022-03-28T09:45:33Z

I don't think that we can add the refresh functionality as it is right now to /session/whoami. It is basically vulnerable to CSRF attacks where an attacker could trick a user into indefinitely refreshing a session. I will probably merge the possibility to refresh a session given the session ID using the admin API only.

identity/handler.go

hero101 · 2022-03-28T10:25:45Z

I don't think that we can add the refresh functionality as it is right now to /session/whoami. It is basically vulnerable to CSRF attacks where an attacker could trick a user into indefinitely refreshing a session. I will probably merge the possibility to refresh a session given the session ID using the admin API only.

Great to see this is making progress. I believe that the admin API refresh capabilities will cover most of the use cases.
Any ideas when will this be released in order for us to push it down our pipelines as well?

aeneasr · 2022-03-28T10:39:42Z

Great to see this is making progress. I believe that the admin API refresh capabilities will cover most of the use cases.
Any ideas when will this be released in order for us to push it down our pipelines as well?

I'll try to get this merged before my vacation

Closes ory#615 Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

Add session renew capabilities, issue-615

cf26cef

abador requested review from aeneasr and zepatrik as code owners January 14, 2022 12:43

abador changed the title ~~Add session renew capabilities, issue-615~~ feat: Add session renew capabilities, issue-615 Jan 14, 2022

abador changed the title ~~feat: Add session renew capabilities, issue-615~~ feat: Add session renew capabilities, #615 Jan 14, 2022

abador added 2 commits January 14, 2022 14:19

Docs, issue-615

b86eb5f

CI fix, issue-615

9aa0d49

abador force-pushed the issue-615-session-renew branch from 647d112 to 9aa0d49 Compare January 14, 2022 14:20

aeneasr reviewed Jan 14, 2022

View reviewed changes

abador added 3 commits January 18, 2022 09:29

CR fixes to field names

2d8639d

CR fixes to docs

f293ebf

Prettier

c8e911b

abador added 2 commits January 18, 2022 14:52

Merge branch 'master' into issue-615-session-renew

9d98f0f

Fix test

9913002

aeneasr reviewed Jan 21, 2022

View reviewed changes

session/handler_test.go Outdated Show resolved Hide resolved

aeneasr reviewed Jan 21, 2022

View reviewed changes

session/handler.go Outdated Show resolved Hide resolved

abador and others added 2 commits January 25, 2022 09:34

Merge branch 'master' into issue-615-session-renew

eab16cc

Apply suggestions from code review

e9d7092

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

abador commented Jan 25, 2022

View reviewed changes

session/handler.go Outdated Show resolved Hide resolved

abador and others added 5 commits January 25, 2022 10:15

Update session/handler.go

ced7746

Merge branch 'master' into issue-615-session-renew

da4fed8

Check issue cookie

47b77c7

Merge branch 'master' into issue-615-session-renew

6bccd66

chore: code review

d535f97

aeneasr force-pushed the issue-615-session-renew branch from 919f315 to d535f97 Compare February 3, 2022 16:21

aeneasr requested changes Feb 3, 2022

View reviewed changes

hero101 mentioned this pull request Feb 18, 2022

Spike: Investigate session token refresh to extend logins alkem-io/client-web#1160

Closed

5 tasks

techsmyth mentioned this pull request Feb 21, 2022

Extend user sessions alkem-io/client-web#1785

Closed

1 task

abador and others added 4 commits March 15, 2022 11:42

Resolve conflicts and merge master

15b2894

Fix tests

36ae38a

Merge branch 'master' into issue-615-session-renew

7930f09

chore: code review

9b4cbb0

chore: code review

eec39f4

chore: remove whoami refresh

8c3fc21

aeneasr mentioned this pull request Mar 28, 2022

docs: move session and prepare for new feature ory/docs#711

Merged

docs: move to ory/docs

199fff2

aeneasr reviewed Mar 28, 2022

View reviewed changes

identity/handler.go Outdated Show resolved Hide resolved

docs: update typo

60fab42

aeneasr approved these changes Mar 28, 2022

View reviewed changes

aeneasr merged commit 4348b86 into ory:master Mar 28, 2022

harnash pushed a commit to Wikia/kratos that referenced this pull request Mar 28, 2022

feat: add session renew capabilities (ory#2146)

11158ae

Closes ory#615 Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

vinckr mentioned this pull request Apr 27, 2022

AdminExtendSession not working as expected #2427

Closed

6 tasks

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023

feat: add session renew capabilities (ory#2146)

323309a

Closes ory#615 Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add session renew capabilities, #615 #2146

feat: Add session renew capabilities, #615 #2146

abador commented Jan 14, 2022 •

edited

Loading

aeneasr left a comment

abador commented Jan 18, 2022

aeneasr commented Jan 21, 2022

aeneasr left a comment

aeneasr Feb 3, 2022

aeneasr Feb 3, 2022

aeneasr Feb 3, 2022

aeneasr Feb 3, 2022

hero101 commented Feb 18, 2022

aeneasr commented Feb 18, 2022

codecov bot commented Mar 28, 2022 •

edited

Loading

aeneasr commented Mar 28, 2022

hero101 commented Mar 28, 2022 •

edited

Loading

aeneasr commented Mar 28, 2022

	// swagger:route PATCH /sessions/refresh/{id} v0alpha2 adminSessionRefresh
	// swagger:route PATCH /sessions/{id}/refresh v0alpha2 adminRefreshSession

	// swagger:route PATCH /sessions/refresh/{id} v0alpha2 adminSessionRefresh
	// swagger:route PATCH /sessions/{id}?refresh=true v0alpha2 adminRefreshSession

		// Calling this endpoint refreshes a given session.
		// If `session.earliest_refresh` is set it will only refresh the session after this time has passed.

feat: Add session renew capabilities, #615 #2146

feat: Add session renew capabilities, #615 #2146

Conversation

abador commented Jan 14, 2022 • edited Loading

Related issue(s)

Checklist

Further Comments

aeneasr left a comment

Choose a reason for hiding this comment

abador commented Jan 18, 2022

aeneasr commented Jan 21, 2022

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Feb 3, 2022

Choose a reason for hiding this comment

aeneasr Feb 3, 2022

Choose a reason for hiding this comment

aeneasr Feb 3, 2022

Choose a reason for hiding this comment

aeneasr Feb 3, 2022

Choose a reason for hiding this comment

hero101 commented Feb 18, 2022

aeneasr commented Feb 18, 2022

codecov bot commented Mar 28, 2022 • edited Loading

Codecov Report

aeneasr commented Mar 28, 2022

hero101 commented Mar 28, 2022 • edited Loading

aeneasr commented Mar 28, 2022

abador commented Jan 14, 2022 •

edited

Loading

codecov bot commented Mar 28, 2022 •

edited

Loading

hero101 commented Mar 28, 2022 •

edited

Loading