eclipse-thingweb / node-wot

A fast and extensible framework to connect any device with your browser and backend applications
https://thingweb.io
Other
166 stars 80 forks source link

OAuth2 "client" security not working correctly #890

Open ilovemilk opened 2 years ago

ilovemilk commented 2 years ago

Hi,

I'm trying to use OAuth2 with "client_credentials" grant and have found one bug:

Here is my configuration:

Thing description

"id": "urn:1234",
"securityDefinitions": {
    "oauth2_sc": {
        "scheme": "oauth2",
        "flow": "client",
        "authorization": "https://***/protocol/openid-connect/auth",
        "token": "https://***/protocol/openid-connect/token",
        "scopes": [
            "email",
            "profile"
        ]
    }
},
"security": "oauth2_sc",

Consumer

servient.addClientFactory(new HttpClientFactory(undefined));
const credentials: Record<string, any> = {
    "urn:1234":
    { 
        clientId: "xyz",
        clientSecret: "secret"
    }
};
servient.addCredentials(credentials);

There is a bug here https://github.com/eclipse/thingweb.node-wot/blob/22f7b2a4e0a1fdc1c8954b6c59ffd80bc4c2fcae/packages/binding-http/src/oauth-manager.ts#L53 If the access_token is too long and split into two entries in the array toString() will add a comma to the access_token. This could be fixed by replacing toString() with join('').

ilovemilk commented 1 year ago

I debugged more and noticed that the second problem was raised by a bug on my side! :) The configuration works. I edited the first comment.

ilovemilk commented 1 year ago

I found another unexpected behavior:

If I read a property with a specific formIndex

const interactionOutput = await thing.readProperty("test", { formIndex: 0 });

and if a client already exists I get a client for that protocol without credentials set. If no client exists everything works fine.

https://github.com/eclipse/thingweb.node-wot/blob/07494392e1def691f06adeae4224afac7cd4cd65/packages/core/src/consumed-thing.ts#L491 This retrieves the client from servient leading to a client without credentials.

https://github.com/eclipse/thingweb.node-wot/blob/07494392e1def691f06adeae4224afac7cd4cd65/packages/core/src/consumed-thing.ts#L493 This instead returns a valid client with credentials set.

Should the client always be retrieved with this.getClients().get(scheme) like here? https://github.com/eclipse/thingweb.node-wot/blob/07494392e1def691f06adeae4224afac7cd4cd65/packages/core/src/consumed-thing.ts#L512

danielpeintner commented 1 year ago

I debugged more and noticed that the second problem was raised by a bug on my side! :) The configuration works. I edited the first comment.

Great. @ilovemilk do you think you can provide a PR with the fix? In the best case adding a test-case that fails with the current code and passes with the fix 👍

I found another unexpected behavior:

I created a new issue https://github.com/eclipse/thingweb.node-wot/issues/891 since it is a different topic.

ilovemilk commented 1 year ago

I would love to contribute code to this project but I'm currently not allowed to do so due to some company regulations. I'm working on getting permission to contribute.

relu91 commented 1 year ago

I would love to contribute code to this project but I'm currently not allowed to do so due to some company regulations. I'm working on getting permission to contribute.

That would be great! thank you for your efforts!! I self-assigned this issue to myself but lately, I don't have so much time. Feel free to ping us here if you have permission before I bake a PR.