-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add GET /sessions endpoint for retrieving all active sessions of authenticated user #228
Conversation
…ns_of_authenticated_user Add GET /sessions endpoint for retrieving all active sessions for the given user
@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:
For me, both 1 or 3 solutions could be applied. What do you think about it? |
Summarizing our discussion during the call:
I think instead of
new response should be defined and used here:
What do you think? |
@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
Do you agree with this solution, or should it be specified in another way? |
Please see my comment in the associated issue |
Returning only the sessionIds would make filters even more useful. E.g. for MSISDN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
sessions: | ||
description: Array of sessions to be returned. | ||
type: array | ||
items: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items: | |
minItems: 0 | |
items: |
code/API_definitions/qod-api.yaml
Outdated
@@ -433,6 +493,23 @@ components: | |||
- expiresAt | |||
- qosStatus | |||
|
|||
SessionInfoList: | |||
description: Paged response of active sessions created by the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
code/API_definitions/qod-api.yaml
Outdated
tags: | ||
- QoS sessions | ||
summary: Get all active sessions information | ||
description: Get information about all active sessions that were created by the client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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. |
summary: Get all active sessions information | ||
description: Get information about all active sessions that were created by the client | ||
operationId: getSessions | ||
parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be interesting to add additional filters, specially to allow filtering sessions for some specific device
code/API_definitions/qod-api.yaml
Outdated
@@ -433,6 +493,23 @@ components: | |||
- expiresAt | |||
- qosStatus | |||
|
|||
SessionInfoList: | |||
description: Paged response of active sessions created by the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
code/API_definitions/qod-api.yaml
Outdated
description: Get information about all active sessions that were created by the client | ||
operationId: getSessions | ||
parameters: | ||
- name: perPage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
code/API_definitions/qod-api.yaml
Outdated
required: false | ||
description: Allows filtering of sessions by device (phone number). | ||
schema: | ||
$ref: "#/components/schemas/PhoneNumber" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add two more spaces to resolve linting issue.
@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. |
code/API_definitions/qod-api.yaml
Outdated
description: Get information about all active sessions authorized to be retrieved by the provided access token. | ||
operationId: getSessions | ||
parameters: | ||
- name: phoneNumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to filter on $ref: "#/components/schemas/Device and not just phone number.
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. |
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
|
Co-authored-by: Randy Levensalor <randymartini@gmail.com>
This is not aligned with the decision we took during last meeting:
|
This reverts commit 2aacae2.
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" |
Setting the PR again to "draft" as the discussions within #101 are not yet concluded and still active. |
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. |
This stale PR is superseded by #325. |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #101
Special notes for reviewers:
Changelog input