Skip to content
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

oAuth2.0 security schema client side implementation #201

Merged
merged 35 commits into from
May 21, 2020

Conversation

relu91
Copy link
Member

@relu91 relu91 commented Apr 6, 2020

Description

This PR introduces the support for oAuth2.0 protocol as a security schema in node-wot. I decided to focus on the client-side first while leaving the server-side for another PR (if it makes sense).

Changes

Currently, I have implemented basic support for client_credentials and password flow. I tried to be as conservative as possible, but I had to change the method setSecurity as shown in the commit ff03b31.

The idea is that during the client initialization the HTTP client bootstraps the oAuth2 protocol and obtains a valid token. After that, every consequent call uses that token. If the token expires, the client refreshes it and perform again the HTTP operation.

Moreover, I decide to use an OAuth client library to handle the details of the flow. This adds a new dependency to the http-binding module. I evaluated the pro and cons, the library seems well supported and adopted with just two (lightweight) dependencies. Furthermore, I added as Dev dependency an oAuth2.0 server to test the changes. The server depends on express which is a quite big package but since it used only for dev proposes it should influence t0o much the production package of node-wot.

Future work discussion

I am planning to add also the code flow but it is more challenging with respect to the other two. I have pinpointed three use cases for node-wot as a oAuth2.0 client:

  1. node-wot used as a client in a web browser
  2. node-wot used as a native client (ie. Node application) in a desktop environment
  3. node-wot used as a client in a device or in a remote host

The code flow can be applied for use cases 1 and 2 while for the use case 3 IMHO client_credentials and password flows fit better. However, code flow requires user interaction with a web page, how can we handle such scenario inside client library?

The naive solution is to display the "oAuth authorization" page automatically every time node-wot calls a Property/Action/Event with oAuth2.0 schema and which is not already initialization with a valid token. However, this might influence negatively the user experience and it makes quite difficult to create web applications with node-wot. On the other hand, the current implementation requests the token once for every TD, which is less invasive than the naive solution. Still, it could pause the execution of a script for a very long time (i.e. until the user authorizes the application).

I think that a more consistent method should be developed to address this kind of flow.. which it might require a modification of Scripting API (i.e. create an Event for authentication requests (?) )

TODOs:

  • Automatically refresh an expired token.
    • refactor: introduce a single function to make an HTTP request so that it is easier to call again the operation after the refresh token operation is completed.
  • Test refresh token
  • Handle multiple security schemes (?)
  • Implement code flow
    • ??

relu91 added 4 commits April 4, 2020 19:48
implement a first draft of token exchange for
client_credential and password flow

Signed-off-by: reluc <[email protected]>
@danielpeintner
Copy link
Member

@relu91 may I ask you to resolve the conflict with cert/key files that popped up because #203 was merged

Question: do you plan to work on the TODOs in this PR?
Please let us know once you think the PR is ready for review...

@relu91
Copy link
Member Author

relu91 commented Apr 20, 2020

Thank you for point that out! Sure I am gonna get rid of them soon.

Question: do you plan to work on the TODOs in this PR?

yes, for sure I will refine the implementation of client_credentials and password flow. However, I have still some doubts about the code flow, as in the last internal call, we didn't really achieve a consensus about it. Probably, we might need to open the discussion to the WG.

As soon I have fished to test the refresh token functionality, the PR should be ready for a first review.

Meanwhile, what do you think about this:

Moreover, I decide to use an OAuth client library to handle the details of the flow. This adds a new dependency to the http-binding module. I evaluated the pro and cons, the library seems well supported and adopted with just two (lightweight) dependencies. Furthermore, I added as Dev dependency an oAuth2.0 server to test the changes. The server depends on express which is a quite big package but since it used only for dev proposes it should influence t0o much the production package of node-wot.

Are you ok about these two new dependencies?

@danielpeintner
Copy link
Member

Are you ok about these two new dependencies?

I am OK with it. I even think it is better to use a promising library instead of trying to implement it ourselves.

@egekorkan @sebastiankb any other opinion?

@egekorkan
Copy link
Member

Thank you @relu91 ! I am ok with the dependencies. Some other things that caught my eye:

  • Could you document somewhere, at least in the examples, which flows are supported? Currently, a user will not know which he/she can use.
  • Just a question but is it fine for the server.key and server.cert to be published to npm? Shouldn't they be in examples or test?

@relu91
Copy link
Member Author

relu91 commented Apr 20, 2020

Could you document somewhere, at least in the examples, which flows are supported? Currently, a user will not know which he/she can use.

Sure! I guess the best place is binding-http README.md file.

Just a question but is it fine for the server.key and server.cert to be published to npm? Shouldn't they be in examples or tests?

You're right, it makes no sense to have server.key and server.cert inside the root folder of the HTTP package. They are meant only for testing purposes; I'll move them inside the test folder.

@egekorkan
Copy link
Member

Sure! I guess the best place is binding-http README.md file.

I totally agree :)

The keys can be also useful for the people experimenting with it. Can you mention in the documentation that they are in the test folder?

@relu91 relu91 marked this pull request as draft April 23, 2020 16:44
@relu91 relu91 marked this pull request as ready for review May 7, 2020 17:39
@relu91
Copy link
Member Author

relu91 commented May 7, 2020

Flies. We kill flies. :)

for sure is a less cruel image 🤣 here we don't even use kill, we say catch two birds...

If based on this experience you have any suggestions on how could we explore this synergy in the Scripting API, the doors are open!
I'd like to explore a neat subset of Response/Body on how we expose interaction data.

I'd be really glad to join the discussion! I just need some more time to keep up with the current specification described in w3c/wot-scripting-api#201 and in relative PR. I think that node-fetch is a very elegant solution, but I still not have the whole picture clear.

@relu91
Copy link
Member Author

relu91 commented May 7, 2020

Going back to draft mode, using node-fetch seems to break the browserified package. This might be the solution to the problem. I'll check it tomorrow. Sorry again for the delay.

@relu91 relu91 marked this pull request as draft May 7, 2020 19:25
introduce two simple polyfil functions for Header.row and
Response.buffer

Signed-off-by: reluc <[email protected]>
@relu91 relu91 marked this pull request as ready for review May 8, 2020 15:21
@danielpeintner
Copy link
Member

Thanks @relu91 !

Who is suited to review this rather big PR @egekorkan @sebastiankb ?

There are some changes in core (see setSecurity --> initSecurity with promise in protocol-interfaces.ts)

@relu91
Copy link
Member Author

relu91 commented May 11, 2020

Yes, unfortunately, the PR is quite big... I suggest using the commit messages for guidance they are quite explicative. I don't know about who is the most appropriate to review; @sebastiankb asked for this feature in the first place and @egekorkan had already done some review work.

@danielpeintner
Copy link
Member

One aspect is the code size, but what makes it even more dangerous is the change in a core API (I understand though the need).

At least it uses a "different" name which guides TS users to the difference but I guess if people solely use JS it will break at runtime :-(

@relu91
Copy link
Member Author

relu91 commented May 11, 2020

but what makes it even more dangerous is the change in a core API (I understand though the need).

What about the change below? It will allow maintaining the same old API but it will defer the actual initialization of the protocol. Another concern that I have is that it might be also difficult to handle lazy initialization if we will choose to provide an API for security as @zolkis hinted in w3c/wot-scripting-api#214 (comment).

I could do a lazy initialization, which means that the protocol is not started until the affordance is called.

@relu91
Copy link
Member Author

relu91 commented May 12, 2020

To increase the probability of the acceptance of this PR, I reverted back to the previous interface for setSecurity. Now the oAuth2.0 is deferred until the actual accordance is called.

We will see where the discussion about code flow will bring us, meanwhile, we can test this subset of the oAuth2.0 protocol.

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments and unfortunately I failed to run the example due to expired certificate?

@danielpeintner danielpeintner merged commit 17a8692 into eclipse-thingweb:master May 21, 2020
@ajs124 ajs124 mentioned this pull request Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants