EC-SEAL / interface-specs

Open API specifications
1 stars 4 forks source link

Changing the HTTP Request Methods in the Persistence Endpoints? #6

Open BPereira99 opened 3 years ago

BPereira99 commented 3 years ago

Hello everyone,

Although I'll mention @ross-little, @faragom, @Raul14 and @albbasdur on this issue, anyone can certainly comment on it. I would like to propose a change in the YAML specification file and in the overall design of the persistence.

In the specs file, it is stated the following:

/per/load:
    post:
      tags:
      - Persistence
      summary: Setup a persistence mechanism and load a secure storage into session.
      description: _
      requestBody:
        content:
          application/x-www-form-urlencoded:
            schema:
              required:
              - msToken
              properties:
                msToken:
                  type: string
                  description: The security token for ms to ms calls
        required: true
      responses:
        200:
          description: Persistence storage loaded
          content: {}
        404:
          description: Error loading persistence storage
          content: {}
  /per/store:
    post:
      tags:
      - Persistence
      summary: Save session data to the configured persistence mechanism (front channel).
      description: _
      requestBody:
        content:
          application/x-www-form-urlencoded:
            schema:
              required:
              - msToken
              properties:
                msToken:
                  type: string
                  description: The security token for ms to ms calls.
        required: true
      responses:
        200:
          description: Persistence storage saved
          content: {}
        404:
          description: Error saving persistence storage
          content: {}
  /per/load/{sessionToken}:
    post:
      tags:
      - Persistence
      summary: Silent setup of a persistence mechanism by loading a user-provided
        secure storage into session. (back channel).
      description: _
      parameters:
      - name: sessionToken
        in: path
        description: The session token
        required: true
        schema:
          type: string
      - name: cipherPassword
        in: query
        description: The password that needs to be used to decrypt the blob, if encrypted
        schema:
          type: string
      requestBody:
        description: The data store to load
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/dataStore'
        required: true
      responses:
        200:
          description: Persistence storage loaded
          content: {}
        404:
          description: Error loading persistence storage
          content: {}
      x-codegen-request-body-name: dataStore
  /per/store/{sessionToken}:
    get:
      tags:
      - Persistence
      summary: Save session data to the configured persistence mechanism (back channel).
        Might return the signed and possibly encrypted datastore
      description: _
      parameters:
      - name: sessionToken
        in: path
        description: The session token
        required: true
        schema:
          type: string
      - name: cipherPassword
        in: query
        description: The password to encrypt the blob
        schema:
          type: string
      responses:
        200:
          description: Persistence storage saved
          content: {}
        201:
          description: Persistence storage attached
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/dataStore'
        404:
          description: Error saving persistence storage
          content: {}

However, the way that it's currently implemented is by using a GET request on /per/load and /per/store endpoints, in which the msToken is sent as a query parameter. The msToken is then verified and its contents are read and validated, so that perseal can know the PDS location (mobile, google drive, etc).

The front-channel flow is now implemented to be performed this way. However, if we want to call upon the back-channel store and load, there are different ways:

  1. for the store, we can simply send the ciphered password in the query parameter as well. The perseal verifies that it exists and, therefore, executes the back-channel store flow;
  2. for the load, it is the only exception where a POST request has to be made, because we need to send the ciphered dataStore in the request Body, while also sending the msToken and cipherPassword in the query as normal

Whatever the conclusion of this discussion turns out to be, I believe we need to update the file because the /per/store l load/{sessionId} endpoints are deprecated. However, we need to decide whether we maintain the current design and I change the implementation or we change the design to be according to the current implementation.

I think that the main argument of using the GET it's because we can simply send the msToken as a query parameter instead of making use of the request body. However, if we want a more secure approach and keep it "hidden", then maybe the POST method is preferable and, in this way, everything including msToken, cipherPassword and cipheredDataStore will be sent there.

I also believe a GET request makes a somewhat more logic sense because what exactly happens, as it is, is the following:

  1. The perseal receives the GET request and validates the PDS location;
  2. The perseal presents a form for the user to insert the password and select the PDS file, for example;
  3. After clicking the "submit" button, an internal POST request occurs inside perseal, where the operations of decrypting the user's PDS using the data he or she inserted and loading it in session happen.

For this reason, I believe the GET makes sense, since we first get the HTML page to insert the data and then the proper operations occur.

Anyway, I ask that you provide your feedback and opinion on this matter, so that everything can be properly documented and coherent.

Thanks in advance!

ross-little commented 3 years ago

After this morning's teleconference discussion it seems also that there is an issue on the ionic implementation of the dashboard that is at present only able to open a window and perfom gets for front-channel communications from the mobile to SEAL endpoints for persistence, SSI, Request Manager so taking this into account it seems it is needed to support the front-channel communications by the GET methods.

@faragom @albbasdur @Raul14 Should we thus update the interface spec to support GET methods on the front-channel communications? For the back-channel then maybe it is better security to keep as a post.

I copy @mvj66 in case of any change meaning an update to teh API GW.

Please treat this as urgent so that it does not delay further the implemenatation on the mobile dashboard.

raulocana commented 3 years ago

As pointed out, the Ionic mobile application is encountering some issues regarding the opening of an external window through a POST method, therefore change those calls to GET could be not just a solution, but a simpler way to manage external pages without sensible content, as @BPereira99 has commented previously.

In any case, this means a refactoring of both the interface-specs.yaml and the API GW, since the API must spread those changes informing in its responses to the Dashboard-Backend about the new methods. For all of that, we will wait for @faragom answer on this matter before making any other change.

This is, as @ross-little said, a very urgent question regarding the continuation of the mobile application.

faragom commented 3 years ago

Hi,

Sorry for the late reply. I missed this e-mail until you pointed it out this morning.

There is no problem in supporting multiple HTTP methods for an api call. The only reason to use a POST when defining them was to avoid the risk of the msToken being truncated if passed as a query parameter, but if the ionic implementation has restrictions, it is fine to use GET. As openapi does not support specifying multiple methods at once, and I believe it is a bit lame to duplicate the interface for the get and post methods, I will add a comment besides it to mark this fact that both GET and POST should be supported. Is this fine for all?

The only point I'm not following is the one involving the /per/load/{sessionToken}: and /per/store/{sessionToken} calls. They were added to support back-channel cases like the mobile storage, where the client would manage the UI and then send the PDS to the module. If they are not used in this implementation, it's fine, but they should be kept on the specs to support potential future use cases. In any case, if they are not used, we should just ignore them.

albbasdur commented 3 years ago

Good morning,

In fact, we need that GET method would be also supported for the calls that implies an external window, not only for the perseal ms, but for all the calls because we would find this issue with the external windows mentioned. @faragom, is there any inconvenience for this?

Thanks!

faragom commented 3 years ago

Hi,

No, of course, you cando GET calls in all POST apis that only have the msToken requets attribute. As I said, the only concern was the risk of cropping the query string.

I believe my modules support both, but we should check others. Maybe, implement the change and we see where it breaks.

ross-little commented 3 years ago

@albbasdur Please can you specify all back-end methods that now also need to additionally support the get method for the mobile implementation. When the API GW returns the trigger it will still return post for these methods but the mobile dashboard will know it also supports get and implement get. Or does the trigger need to be updated to show that either post or get can be used for these frontchannel APIs?

@faragom @endimion @BPereira99 @mvj66 @msirvent2 We need all front-channel APIs that suport communication from the Dashboard to additionally also support these APIs with a GET method as well as the existing post (for the web dashboard). Please can you indicate the changes you will make here in this issue. The sooner this is done the sooner we have a working mobile dashboard so please can you all address this, this week.

raulocana commented 3 years ago

Hi everyone,

We have been working on retrieving all the methods that are called in the process of opening the external windows. A far majority of them are already GET methods, or at least they support it. However, there are other ones that are not. The good part is that there is not a parameter different from the msToken involved in these calls, so we believe is 'safety' to support them also by GET.

These are the methods used in the ext. windows opening:

Load a PDS (Uses GET already): https://vm.project-seal.eu:8082/per/load?msToken=

Store a PDS (Uses GET already): https://vm.project-seal.eu:8082/per/store?msToken=

SSI access (Uses GET already): https://vm.project-seal.eu:4000/vc/didAuth?msToken=

Derive an Identity (Uses GET already): https://vm.project-seal.eu:9020/idboot/generate?msToken=

Issue VC (Uses GET already): https://vm.project-seal.eu:4000/vc/issue/edugain?msToken=

Retrieve an Identity (Uses POST): Body parameters: [‘msToken’] https://vm.project-seal.eu:10081/as/authenticate

Link_Identities (Uses POST): Body parameters: [‘msToken’] https://vm.project-seal.eu:9050/link/request/submit

Having this list in mind, it looks like there are only two methods that need GET support, as/authenticate and link/request/submit.

Apart from this, we might rise another issue on this same matter, discussing the need to open an external window every time, even in use cases where it is not really interactive, like presenting a message that could be addressed using a browser alert. This will improve and enhance the user experience and also, the user interface for both the Web Dashboard and the Mobile App.

Please make sure you let us know as soon as both these ones are updated or whether they already are. Thank you so much!

miryamvillegas commented 3 years ago

Hi @Raul14 , Could you please check as/authenticate? It supports GET too... (https://vm.project-seal.eu:10081/as/authenticate)

faragom commented 3 years ago

@mvj66 has deployed the new version of the reconciliation ms as well. You can try it.