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

[kacper-kicinski]

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.

[RandyLevensalor]

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

[kacper-kicinski]

@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:

1. Simply add Generic404 specification
2. Change $ref from Generic404 to SessionNotFound404, which is already specified, but - does it really fit that case?
3. Add PageNotFound404 specification and refer to it instead of Generic404
4. 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]

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]

@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: https://github.com/camaraproject/QualityOnDemand/blob/3f9f2fbefcf8b3b3a2ff0b7c04fb6127cad63ea6/code/API_definitions/qod-api.yaml#L255-L273

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

[eric-murray]

Please see my comment in the associated issue

[akoshunyadi]

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

[kacper-kicinski]

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.

[hdamker]

@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]

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]

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)

[jlurien]

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.

[kacper-kicinski]

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]

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

[eric-murray]

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]

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]

This stale PR is superseded by #325.