Add GET /sessions endpoint for retrieving all active sessions of authenticated user #228

kacper-kicinski · 2023-10-19T13:55:19Z

What type of PR is this?

feature

What this PR does / why we need it:

This PR adds a new GET /sessions endpoint for retrieving all active sessions of the authenticated user

Which issue(s) this PR fixes:

Fixes #101

Special notes for reviewers:

Changelog input

GET /sessions endpoint for retrieving all active sessions of authenticated user added.

…ns_of_authenticated_user Add GET /sessions endpoint for retrieving all active sessions for the given user

RandyLevensalor · 2023-10-20T03:48:15Z

@kacper-kicinski The linters failed. Please check the results above and address. Thanks!

kacper-kicinski · 2023-10-20T11:15:12Z

@kacper-kicinski The linters failed. Please check the results above and address. Thanks!

There is some issue that would be good to discuss here:

When a client requests for the list of active sessions, but there is no one, what HTTP code and response should be returned? We opt for returning just 200 and sessions = empty array. Another solution would be returning the 404 - Not Found, but we don't recommend it since it is not really an error, just the sessions list is empty.

But there is also another point: since we support pagination, the client could, in the request, specify a page value which is greater than the last available page (for example, there are pages 0,1,2 and the client requests for page 3, which does not exist). In that case, 404 could be returned. But here we go to the cause of linting failure - there is no Generic404 specification, which was referred here: https://github.com/kacper-kicinski/QualityOnDemand/blob/8f394bfa592e6aca23b9e952f959dbd842590d73/code/API_definitions/qod-api.yaml#L262

Possible solutions:

Simply add Generic404 specification
Change $ref from Generic404 to SessionNotFound404, which is already specified, but - does it really fit that case?
Add PageNotFound404 specification and refer to it instead of Generic404
Return in this case another response code, not 404 (but which one?)

For me, both 1 or 3 solutions could be applied. What do you think about it?

rartych · 2023-10-20T15:12:13Z

Summarizing our discussion during the call:

When there is no active session: return 200 and sessions = empty array
According to Design guidelines for page out of range the code 400 should be returned:
400: request outside the range of the resource list
It can cover:
- Invalid pagination parameters (page or per_page less than 1)
- Invalid page number (exceeding the total number of pages)

I think instead of

    Generic400:
      description: Invalid input

new response should be defined and used here:

    InvalidPagination400:
      description: Invalid pagination

What do you think?

kacper-kicinski · 2023-10-23T08:42:33Z

@rartych thanks for the comment! Yes, I agree with the 400 code for invalid pagination.

Instead of creating a new error response template (like InvalidPagination400), I simply added two examples of the 400 response: PageOutOfRange and InvalidPerPageValue, directly in the GET /sessions specification because the pagination is only supported by this endpoint:

QualityOnDemand/code/API_definitions/qod-api.yaml

Lines 255 to 273 in 3f9f2fb

    
           "400": 
        
             description: Invalid pagination for getSession operation 
        
             content: 
        
               application/json: 
        
                 schema: 
        
                   $ref: "#/components/schemas/ErrorInfo" 
        
                 examples: 
        
                   PageOutOfRange: 
        
                     summary: The page number is out of the range 
        
                     value: 
        
                       status: 400 
        
                       code: INVALID_PAGINATION 
        
                       message: "Invalid pagination: page number out of the range" 
        
                   InvalidPerPageValue: 
        
                     summary: The perPage value is less than 1 
        
                     value: 
        
                       status: 400 
        
                       code: INVALID_PAGINATION 
        
                       message: "Invalid pagination: perPage value can't be less than 1"

Do you agree with this solution, or should it be specified in another way?

eric-murray · 2023-10-23T10:43:39Z

Please see my comment in the associated issue

akoshunyadi · 2023-10-23T12:23:55Z

Returning only the sessionIds would make filters even more useful. E.g. for MSISDN

jlurien

The topic of sessions ownership and requirements for implementations to check access tokens is now critical with the new endpoint, but it is probably something that we may need for previous GET /session/{sessionId} as well. We assumed sessionId was a "secret", but it makes sense to validate also that the sessionId was created by the client_id and for for the user linked to the access token.

jlurien · 2023-10-23T15:11:44Z

code/API_definitions/qod-api.yaml

+        sessions:
+          description: Array of sessions to be returned.
+          type: array
+          items:


Suggested change

items:

minItems: 0

items:

jlurien · 2023-10-23T15:16:47Z

code/API_definitions/qod-api.yaml

@@ -433,6 +493,23 @@ components:
            - expiresAt
            - qosStatus

+    SessionInfoList:
+      description: Paged response of active sessions created by the user


Here we mention created by the user, and above created by the client. This topic is very relevant for this endpoint as discussed in #101. The logic must be to return the sessions that are allowed to be discovered by the access token. In the most restrictive case, for three-legged tokens, that includes checking both client_id and user.
Returning just sessionIds or all session details does not change the picture, if knowing sessionIds is enough to get session details in a separate operation.

jlurien · 2023-11-09T16:44:11Z

code/API_definitions/qod-api.yaml

+      tags:
+        - QoS sessions
+      summary: Get all active sessions information
+      description: Get information about all active sessions that were created by the client


Here, we should indicate that the returned sessions will be those allowed to be discovered with the provided access token, which will depend on the type of access token provided, 2-legged or 3-legged. This has to be clarified in the implementation guidelines and aligned with Identity&Consent Management guidelines, but to progress with the spec we could add something like:

Suggested change

description: Get information about all active sessions that were created by the client

description: Get information about all active sessions authorized to be retrieved by the provided access token.

jlurien · 2023-11-09T16:45:29Z

code/API_definitions/qod-api.yaml

+      summary: Get all active sessions information
+      description: Get information about all active sessions that were created by the client
+      operationId: getSessions
+      parameters:


It could be interesting to add additional filters, specially to allow filtering sessions for some specific device

jlurien · 2023-11-09T16:47:14Z

code/API_definitions/qod-api.yaml

@@ -433,6 +493,23 @@ components:
            - expiresAt
            - qosStatus

+    SessionInfoList:
+      description: Paged response of active sessions created by the user


Suggested change

description: Paged response of active sessions created by the user

description: Paged response of active sessions authorized to be retrieved by the provided access token.

jlurien · 2023-11-09T16:51:00Z

code/API_definitions/qod-api.yaml

+      description: Get information about all active sessions that were created by the client
+      operationId: getSessions
+      parameters:
+        - name: perPage


Paging could be interesting for 2-legged scenarios where some client_id may potentially create lots of sessions. For 3-legged scenarios, paging is probably not that necessary, as each user will likely have a limited number of active sessions. Until we fully clarify which kind of tokens will be allowed for QoD, I would not add paging parameters yet, we could add them in a subsequent review if they are needed.

kacper-kicinski · 2023-11-17T10:20:18Z

Thank you for the comments. I removed pagination, changed some descriptions, allowed filtering of sessions by phoneNumber. For now I left the response as a list of whole sessions. However, we can continue to discuss wheter it would be better to return only sessionsIds.

RandyLevensalor · 2023-11-17T15:24:31Z

code/API_definitions/qod-api.yaml

+        required: false
+        description: Allows filtering of sessions by device (phone number).
+        schema:
+          $ref: "#/components/schemas/PhoneNumber"


Add two more spaces to resolve linting issue.

hdamker · 2023-11-17T15:31:32Z

@kacper-kicinski Would you mind to declare the PR "ready for review"?

We discussed in today's QoD meeting that the PR will make it into the v0.10.0 release candidate only if it can be finished (= including final reviews and approvals) together with the other three open PRs within the next week (until Friday Nov 24th). The goal is to create the release candidate at the end of next week. If the PR will be not ready to be merged until then it will not make it into v0.10.0.

RandyLevensalor · 2023-11-17T15:49:12Z

code/API_definitions/qod-api.yaml

+      description: Get information about all active sessions authorized to be retrieved by the provided access token.
+      operationId: getSessions
+      parameters:
+      - name: phoneNumber


It would be better to filter on $ref: "#/components/schemas/Device and not just phone number.

RandyLevensalor · 2023-11-17T16:46:06Z

Returning only the sessionIds would make filters even more useful. E.g. for MSISDN

Only returning sessionIds could be a much faster and more responsive API call than returning the complete session object. Especially with filtering, where the sessionId may be sufficient. Plus, there is an existing API call to get the session information based on sessionId.

hdamker · 2023-11-20T08:18:14Z

Please see my comment within the issue where I summarized the open discussions based on our QoD call on Friday.

From that it is yet to be decided for v0.10.0

Paging or not
Filtering on phoneNumber only vs filtering on device object (comment above)

code/API_definitions/qod-api.yaml

Co-authored-by: Randy Levensalor <randymartini@gmail.com>

jlurien · 2023-11-22T11:24:36Z

I changed the response from a list of whole session objects to a list of only session IDs. Please read Herbert's comment. There are some open points for further discussion, pagination, security, filtering etc. MR is now ready for review. Kindly ask reviewers to take a look.

This is not aligned with the decision we took during last meeting:

Should the operation deliver the full session information or just the session ids?
Result of the discussion: return the full session information
Rational: this is the more developer friendly option, as otherwise the API consumer would need to call getSession one by one to find the information they are looking for. From privacy perspective has the API consumer provided and seen all personal information already during the respective session creations. It should be enough that the API consumer was authorized to create the sessionId(s) to assume consent for the get operation.

This reverts commit 2aacae2.

kacper-kicinski · 2023-11-24T09:03:43Z

I reverted commit about changing the response from whole session objects to only sessionIds. It was a mistake. As Herbert wrote in his comment - "Result of the discussion: return the full session information"

hdamker · 2023-11-27T06:50:45Z

Setting the PR again to "draft" as the discussions within #101 are not yet concluded and still active.

eric-murray · 2024-01-17T15:33:56Z

Issue #101 is now active again, and I think @patrice-conil and myself have landed on a proposal that we both can live with. Can others review and comment?

The main difference is that the new endpoint will only return session details that were created by the API consumer for a given end user device. The end user device will be identified from the OAuth token, so will not need to be explicitly specified in the API definition.

For those that don't understand how this will work, see here.

jlurien · 2024-01-24T12:15:09Z

Issue #101 is now active again, and I think @patrice-conil and myself have landed on a proposal that we both can live with. Can others review and comment?

The main difference is that the new endpoint will only return session details that were created by the API consumer for a given end user device. The end user device will be identified from the OAuth token, so will not need to be explicitly specified in the API definition.

For those that don't understand how this will work, see here.

I have replied as well in the other issue thread, and I have drafted a proposal to change this operation to a POST method, that would ease many problems that we face with the GET. Regarding whether the device has to be provided explicitly or may be identified by the token, I think that this question is not specific to the new operation. For createSession, we should have the same requirements regarding security, in order to check that the access token allows the client to create session for certain device, and in some cases the device could be known from the access token directly, so we could make device optional for createSession as well.

hdamker · 2024-08-01T13:32:50Z

This stale PR is superseded by #325.

kacper-kicinski added 5 commits October 18, 2023 11:55

Update qod-api.yaml

239ad1c

Update with the comments

c55fa13

Make page params not mandatory

a77a92e

Add 404 as a possible error code for GET /sessions

8f394bf

Merge pull request #2 from kacper-kicinski/feature/list_active_sessio…

f1011d8

…ns_of_authenticated_user Add GET /sessions endpoint for retrieving all active sessions for the given user

kacper-kicinski requested review from hdamker, eric-murray and RandyLevensalor as code owners October 19, 2023 13:55

Change error code for invalid pagination

3f9f2fb

eric-murray marked this pull request as draft October 23, 2023 10:43

jlurien reviewed Oct 23, 2023

View reviewed changes

jlurien reviewed Nov 9, 2023

View reviewed changes

kacper-kicinski added 2 commits November 17, 2023 11:03

Remove pagination

26d3bca

Allow filtering sessions by phoneNumber

36bdf5f

RandyLevensalor reviewed Nov 17, 2023

View reviewed changes

This was referenced Nov 19, 2023

Release v0.10.0 - Release Candidate due Nov 24th #235

Closed

List endpoint for active sessions of authenticated user #101

Closed

Return only session IDs

2aacae2

RandyLevensalor reviewed Nov 20, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

Fix linting issue

75aa001

Co-authored-by: Randy Levensalor <randymartini@gmail.com>

kacper-kicinski marked this pull request as ready for review November 21, 2023 08:02

Revert "Return only session IDs"

a44faf8

This reverts commit 2aacae2.

fix indentation errors

42131e1

hdamker marked this pull request as draft November 27, 2023 06:52

eric-murray mentioned this pull request Nov 30, 2023

Creation of release candicate v0.10.0-rc #236

Merged

jlurien mentioned this pull request Jul 19, 2024

List sessions endpoint for quality-on-demand #325

Merged

hdamker closed this Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GET /sessions endpoint for retrieving all active sessions of authenticated user #228

Add GET /sessions endpoint for retrieving all active sessions of authenticated user #228

kacper-kicinski commented Oct 19, 2023

RandyLevensalor commented Oct 20, 2023

kacper-kicinski commented Oct 20, 2023 •

edited

Loading

rartych commented Oct 20, 2023

kacper-kicinski commented Oct 23, 2023 •

edited

Loading

eric-murray commented Oct 23, 2023

akoshunyadi commented Oct 23, 2023

jlurien left a comment

jlurien Oct 23, 2023

jlurien Oct 23, 2023

jlurien Nov 9, 2023

jlurien Nov 9, 2023

jlurien Nov 9, 2023

jlurien Nov 9, 2023

kacper-kicinski commented Nov 17, 2023

RandyLevensalor Nov 17, 2023

hdamker commented Nov 17, 2023 •

edited

Loading

RandyLevensalor Nov 17, 2023

RandyLevensalor commented Nov 17, 2023

hdamker commented Nov 20, 2023

jlurien commented Nov 22, 2023

kacper-kicinski commented Nov 24, 2023

hdamker commented Nov 27, 2023

eric-murray commented Jan 17, 2024

jlurien commented Jan 24, 2024

hdamker commented Aug 1, 2024

	description: Get information about all active sessions that were created by the client
	description: Get information about all active sessions authorized to be retrieved by the provided access token.

	description: Paged response of active sessions created by the user
	description: Paged response of active sessions authorized to be retrieved by the provided access token.

Add GET /sessions endpoint for retrieving all active sessions of authenticated user #228

Add GET /sessions endpoint for retrieving all active sessions of authenticated user #228

Conversation

kacper-kicinski commented Oct 19, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Changelog input

RandyLevensalor commented Oct 20, 2023

kacper-kicinski commented Oct 20, 2023 • edited Loading

rartych commented Oct 20, 2023

kacper-kicinski commented Oct 23, 2023 • edited Loading

eric-murray commented Oct 23, 2023

akoshunyadi commented Oct 23, 2023

jlurien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kacper-kicinski commented Nov 17, 2023

Choose a reason for hiding this comment

hdamker commented Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

RandyLevensalor commented Nov 17, 2023

hdamker commented Nov 20, 2023

jlurien commented Nov 22, 2023

kacper-kicinski commented Nov 24, 2023

hdamker commented Nov 27, 2023

eric-murray commented Jan 17, 2024

jlurien commented Jan 24, 2024

hdamker commented Aug 1, 2024

kacper-kicinski commented Oct 20, 2023 •

edited

Loading

kacper-kicinski commented Oct 23, 2023 •

edited

Loading

hdamker commented Nov 17, 2023 •

edited

Loading