Add session endpoints #5730

eddumelendez · 2016-04-19T01:22:09Z

I have signed the CLA

rwinch · 2016-04-19T04:29:30Z

...actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/SessionMvcEndpoint.java

+	/**
+	 * Mark if the endpoint exposes sensitive information.
+	 */
+	private Boolean sensitive;


It may be wise to make this always sensitive

jgoldhammer · 2016-04-23T11:23:13Z

+1 for integrating it...

philwebb · 2016-04-24T19:43:54Z

Does this expose session data for all users? Can you provide a bit more background about what use-cases are?

vpavic · 2016-04-24T20:28:33Z

@philwebb See spring-projects/spring-session#407 for some background.

Exposing session data for all users is not possible since SessionRepository does not offer such functionality ATM.

philwebb · 2016-04-24T20:38:01Z

@vpavic Thanks for the pointer.

philwebb · 2016-04-24T20:39:00Z

@rwinch Any security considerations regarding exposing session data in this way?

rwinch · 2016-04-25T14:35:14Z

@philwebb

I think this is going to be a very useful feature and would like to see it integrated. Eventually, I'd like to extend it to support log levels per user (I have a prototype of this working already).

Since this will allow access to any user's session, I view this as an opt in feature.

philwebb · 2016-04-25T18:38:11Z

@rwinch OK, so perhaps like the shutdown endpoint users should specifically enable this one.

jgoldhammer · 2016-04-25T19:33:48Z

Anything to share, Rob? Just wanted to implement that for our application on top of a logbook and sifting appended to have a trac log for each user...

joshiste · 2016-04-25T19:47:52Z

...actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/SessionMvcEndpoint.java

+	private FindByIndexNameSessionRepository<? extends ExpiringSession> sessionRepository;
+
+	@RequestMapping(path = "/{username}", method = RequestMethod.GET)
+	public Collection<? extends ExpiringSession> result(@PathVariable String username) {


In my opinion it would be a better REST-style to turn username into a requestParameter.
I personally would expect a /{sessionId} to get a specific session and a ?username={username} to search for sessions asscoiated with the user.

rwinch · 2016-04-25T21:41:08Z

@jgoldhammer Nothing new to report on that. I still haven't found time to put something formal together. You can find a sample at https://github.com/rwinch/spring-session-logging

philwebb · 2016-04-27T18:09:08Z

I think we'll need to refine this a little more. There are a couple of problems that we can foresee:

Returning ExpiringSession seems like it might cause problems since we're relying on Jackson to marshal it correctly. We should probably wrap it in a JSON friendly structure.
The rest calls are a little inconsistent. Calling GET /session/{var} expects a user, but DELETE /session/{var} expects the session ID. We should probably just use the session ID (as @joshiste suggests above).

eddumelendez · 2016-04-28T04:56:56Z

Thanks all for feedback. @philwebb I have updated the PR with your comments.

joshiste · 2016-04-28T05:51:55Z

Currently your PR only provides a MvcEndpoint implementation, which doesn't gets registered as MBean and therefore is not accessible via JMX.
I would like to see the implementation split apart into a Endpoint implementation providing the functionality and the MvcEndpoint just providing the http interface for it. So it's also accessible with JMX.

... or is the SessionRepository already exported to JMX?

vpavic · 2016-04-28T05:53:23Z

...actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/SessionMvcEndpoint.java

+ * @since 1.4.0
+ */
+@ConfigurationProperties("endpoints.session")
+public class SessionMvcEndpoint extends WebMvcConfigurerAdapter


Any particular reason to extend the WebMvcConfigurerAdapter here?

Thanks to let me know. I completely forget to remove this.

vpavic · 2016-04-28T06:01:42Z

...actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/SessionMvcEndpoint.java

+	/**
+	 * Session properties.
+	 */
+	public static class Session {


IMO the representation of session should be modeled with more care considering the needs of potential consumer:

creationTime and lastAccessedTime should be converted to Date

is maxInactiveIntervalInSeconds really needed here?

consider providing at least some info about session attributes

You are right maxInactiveIntervalInSeconds doesn't make sense here.

Thanks for the suggestions

wilkinsona · 2016-04-28T06:02:23Z

Thanks, @eddumelendez. I agree with @joshiste's latest comment. We don't do anything with the repository to export is as an MBean. I don't know if Spring Session does. For consistency, having our own JMX endpoint is appealing anyway.

A couple of other things to consider:

Should a delete of a non-existent session return a 404 rather than a 200 response?
Returning an array of sessions isn't very extensible. For example, it prevents the addition of HATEOAS links. I think it would be better to return a map with a single sessions entry.

One other thing that may just be my ignorance. How does an operator know the usernames to use when listing sessions? I realise that Spring Session does not provide an API for it, but would listing all sessions not be useful?

vpavic · 2016-04-28T06:08:10Z

Any security considerations regarding exposing session data in this way?

OK, so perhaps like the shutdown endpoint users should specifically enable this one.

I'd like to see a more sophisticated approach to this:

it makes sense to provide every user with the ability to read and delete their own session(s)
reading and deleting sessions for any user should be limited to some role/authority

IMO this is better than simply disabling the endpoint by default.

joshiste · 2016-04-28T06:25:48Z

it makes sense to provide every user with the ability to read and delete their own session(s)

What's the reasoning behind this? Usually this is exactly what a logout-button is for...
Since it's easier to have a all or nothing approach I don't think it should be done this way.

vpavic · 2016-04-28T06:29:07Z

+1 for the JMX support, Spring Session does not export anything there by itself.

One other thing that may just be my ignorance. How does an operator know the usernames to use when listing sessions? I realise that Spring Session does not provide an API for it, but would listing all sessions not be useful?

Generally this is tied to security since this is where the Spring Session gets the info about the user. Spring Session itself and its APIs are not tied to security (check the FindByIndexNameSessionRepository#findByIndexNameAndIndexValue javadoc or reference manual on FindByIndexNameSessionRepository)

vpavic · 2016-04-28T06:31:16Z

What's the reasoning behind this? Usually this is exactly what a logout-button is for...

Logout does the job for the current session. It is a common requirement to have an option to invalidate the session(s) you have on other clients.

See here (Sessions) as an example.

joshiste · 2016-04-28T06:43:12Z

Logout does the job for the current session. It is a common requirement to have an option invalidate the session(s) you have on other clients.

Ok. Don't you think, when it's for your users (not your admins!), you should implement this with your own controller in your "business code" and not within an actuator endpoint?
Think of securing all endpoints a specific way (e.g. management.security.role) or binding them to some other port which doesn't get exposed to the user...

vpavic · 2016-04-28T07:01:59Z

@joshiste I understand your reasoning but I don't see how having an option to do GET request that retrieves sessions for the currently authenticated user interferes with other functionalities?
Ultimately it's up to the user to decide how to use any given endpoint, that's why there are options to customize the behavior.

wilkinsona · 2016-04-28T07:06:11Z

I agree with @joshiste. Actuator endpoints are intended for the operators of a service and not its users. If an app wants to support a user logging out across all devices, it should provide the UI and controller for it with appropriate authentication.

joshiste · 2016-04-28T07:41:59Z

@joshiste I understand your reasoning but I don't see how having an option to do GET request that retrieves sessions for the currently authenticated user interferes with other functionalities?

My point was the finegrained security permissions and to keep a feature out, which is not in the scope of administration.

vpavic · 2016-04-28T07:53:30Z

@joshiste Yes, the more I think of this the more I agree with you. Sorry for the noise and thanks for discussing!

eddumelendez · 2016-05-03T06:44:28Z

Thanks all for the feedback and great discussion about this topic.

@wilkinsona I have updated the PR.

snicoll · 2017-09-20T15:39:50Z

Duplicate of #8342

eddumelendez mentioned this pull request Apr 19, 2016

Add actuator endpoint to find and delete sessions spring-projects/spring-session#480

Closed

1 task

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 19, 2016

rwinch reviewed Apr 19, 2016
View reviewed changes

eddumelendez force-pushed the session-endpoints branch 2 times, most recently from c000863 to ff4e449 Compare April 20, 2016 05:46

philwebb added status: waiting-for-feedback We need additional information before we can continue for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 24, 2016

philwebb removed the status: waiting-for-feedback We need additional information before we can continue label Apr 24, 2016

philwebb added this to the 1.4.0.M3 milestone Apr 25, 2016

joshiste reviewed Apr 25, 2016
View reviewed changes

philwebb added the status: on-hold We can't start working on this issue yet label Apr 27, 2016

philwebb removed the for: team-attention An issue we'd like other members of the team to review label Apr 27, 2016

eddumelendez force-pushed the session-endpoints branch 2 times, most recently from adb3382 to 94c9eda Compare April 28, 2016 04:56

vpavic reviewed Apr 28, 2016
View reviewed changes

eddumelendez force-pushed the session-endpoints branch from 94c9eda to ba6d67d Compare April 28, 2016 06:02

eddumelendez force-pushed the session-endpoints branch from ba6d67d to 18b3716 Compare April 29, 2016 08:52

Add session endpoints

6351eea

eddumelendez force-pushed the session-endpoints branch from 18b3716 to 6351eea Compare May 3, 2016 06:42

philwebb modified the milestones: 1.4.0.RC1, 1.4.0.M3 May 14, 2016

philwebb force-pushed the master branch from 928511b to cd365bc Compare May 14, 2016 03:09

philwebb added the for: team-attention An issue we'd like other members of the team to review label May 25, 2016

philwebb removed this from the 1.4.0.RC1 milestone May 25, 2016

philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jun 22, 2016

wilkinsona mentioned this pull request Feb 13, 2017

Create Actuator Endpoint To Manage Active Spring-Sessions #8278

Closed

vpavic mentioned this pull request Feb 20, 2017

Add actuator endpoint for finding and deleting sessions #8342

Closed

eddumelendez closed this Sep 20, 2017

snicoll marked this as a duplicate of #8342 Sep 20, 2017

snicoll added status: duplicate A duplicate of another issue and removed status: on-hold We can't start working on this issue yet labels Sep 20, 2017

eddumelendez deleted the session-endpoints branch January 17, 2018 13:43

Add session endpoints #5730

Add session endpoints #5730

Uh oh!

Conversation

eddumelendez commented Apr 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwinch Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

jgoldhammer commented Apr 23, 2016

Uh oh!

philwebb commented Apr 24, 2016

Uh oh!

vpavic commented Apr 24, 2016

Uh oh!

philwebb commented Apr 24, 2016

Uh oh!

philwebb commented Apr 24, 2016

Uh oh!

rwinch commented Apr 25, 2016

Uh oh!

philwebb commented Apr 25, 2016

Uh oh!

jgoldhammer commented Apr 25, 2016

Uh oh!

joshiste Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch commented Apr 25, 2016

Uh oh!

philwebb commented Apr 27, 2016

Uh oh!

eddumelendez commented Apr 28, 2016

Uh oh!

joshiste commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic Apr 28, 2016

Choose a reason for hiding this comment

Uh oh!

eddumelendez Apr 28, 2016

Choose a reason for hiding this comment

Uh oh!

vpavic Apr 28, 2016

Choose a reason for hiding this comment

Uh oh!

eddumelendez Apr 28, 2016

Choose a reason for hiding this comment

Uh oh!

wilkinsona commented Apr 28, 2016

Uh oh!

vpavic commented Apr 28, 2016

Uh oh!

joshiste commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic commented Apr 28, 2016

Uh oh!

vpavic commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshiste commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic commented Apr 28, 2016

Uh oh!

wilkinsona commented Apr 28, 2016

Uh oh!

joshiste commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic commented Apr 28, 2016

Uh oh!

eddumelendez commented May 3, 2016

Uh oh!

snicoll commented Sep 20, 2017

Uh oh!

Uh oh!

eddumelendez commented Apr 19, 2016 •

edited

Loading

joshiste Apr 25, 2016 •

edited

Loading

joshiste commented Apr 28, 2016 •

edited

Loading

joshiste commented Apr 28, 2016 •

edited

Loading

vpavic commented Apr 28, 2016 •

edited

Loading

joshiste commented Apr 28, 2016 •

edited

Loading

joshiste commented Apr 28, 2016 •

edited

Loading