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

Problems with non-HMAC Webhooks Auth #1422

Closed
weberjm opened this issue Jun 9, 2022 · 2 comments · Fixed by #1425
Closed

Problems with non-HMAC Webhooks Auth #1422

weberjm opened this issue Jun 9, 2022 · 2 comments · Fixed by #1425
Assignees
Labels
bug Something isn't working webhooks

Comments

@weberjm
Copy link
Member

weberjm commented Jun 9, 2022

From Slack:
We are encountering an issue with the Webhooks Service authentication when using the Bearer token or Basic authentication methods, where the authentication always fails.

Both Bearer and Basic webhook authentication (but not HMAC) make a call to the method checkPermissions(), which in turn calls the iam-utils function isOwnerOf().
isOwnerOf() is passed the flow object pulled from the webhooks MongoDB collection (the entity) and the user data returned from iamUtils.getUserData() (the user).
There are two problems that occur within isOwnerOf() relating to the array of objects stored in entity.owners:

  1. the entity owner id is a Mongo ObjectId(), but user.sub, the value it is compared to, is of type string, so the comparison always fails
  2. the entity owner type is a lowercase string 'user' / 'tenant', but it is compared to CONSTANTS.ENTITY.USER / CONSTANTS.ENTITY.TENANT, both of which are uppercase, so the comparison always fails.

Is there something that I'm missing or misconfiguring? I used an admin user with ['all'] permissions to create the flow, start the flow, and then used that user's token for Bearer auth to try and rule out a permissions configuration issue.
If I understand the issue correctly I see two possible solutions:

  • This can be fixed in the webhooks service by changing what is passed to isOwnerOf() - instead of the original flow object, a new object can be passed that has mapped the owners array to have objects where id is of type string, and type is uppercased.
  • This could also be fixed in the iam-utils library by converting the id to a string and converting the type to uppercase within isOwnerOf() before comparing. The only other service that uses isOwnerOf() is the Metadata Repository, which appears to pass the expected data type and letter case. It would not be affected by changing the owner.id to a string (it is already a string) or by setting the owner.type to uppercase (it is already uppercase).

Using openintegrationhub/webhooks:latest (v22.1.0)
Links:

  • checkPermissions() method in Webhooks Service link
  • isOwnerOf() function in iam utils library link
@weberjm
Copy link
Member Author

weberjm commented Jun 9, 2022

Back when this was first implemented, I think both Robb and I tested the functionality, but looks like maybe it was not properly tested across services.

As you said, the only other service directly using this call is the meta-data-repository, but also a variety of other services use an entity constant which is uppercase. The flow-repository, however, passes a hardcoded string which is lowercase.

It has also not escaped my attention that various locations store object keys as strings instead of as ObjectIDs in Mongo. This is another area which could be adjusted so we do not have to type convert so often in the code. I don't know if there are situations currently where users save "non-ID" strings in owner arrays, for instance.

@weberjm weberjm added bug Something isn't working webhooks labels Jun 9, 2022
@weberjm weberjm self-assigned this Jun 9, 2022
@weberjm
Copy link
Member Author

weberjm commented Jun 10, 2022

The owners array in the webhooks service for Flows (https://github.com/openintegrationhub/openintegrationhub/blob/master/services/webhooks/src/models/Flow.js) is typed as an ObjectId. Although this is "correct," at least in the majority of cases, the owners' IDs of most schemas throughout the framework are set as Strings. Especially as this model would contain a copy of a Flow from the flow-repository, where the flow owners' IDs are set as strings, then the owners' IDs in the webhooks service should also be stored as Strings. This will eliminate one of the type mismatches (without having to change the models of every service in the system...).

@weberjm weberjm mentioned this issue Jun 15, 2022
@weberjm weberjm linked a pull request Jun 21, 2022 that will close this issue
@weberjm weberjm closed this as completed Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working webhooks
Development

Successfully merging a pull request may close this issue.

1 participant