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 support #18

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

OAuth2 support #18

wants to merge 17 commits into from

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Jan 28, 2020

This PR brings OAuth2 support

Documentation: mautic/documentation#375

@npracht
Copy link
Member

npracht commented Jan 28, 2020

To give a bit more of context: we've been working with @kuzmany to enhance the Zapier plugin.

Today, it is using basic auth which is.... not nice it terms of security.
With this PR we suggest the move to OAuth2 authentification offered by mautic app.

@escopecz
Copy link
Sponsor Member

Can this be done without the BC break? If we release this then the Zapier users will have to go and reauthenticate all their Zaps. Ideally, it would be great if the integration would still support basic auth for existing Zaps and Oauth2 for new ones.

@npracht
Copy link
Member

npracht commented Jan 29, 2020

Or if not possible, create a V2 plugin, i've seen this behavior often.

@kuzmany
Copy link
Member Author

kuzmany commented Jan 29, 2020

@escopecz I think It works like you describe.
The current Zaps will use old version and new Zaps will use newer.
Then we can continue maintenance both version, or just new one (I prefer just new one - Oauth2 is more secure).

@npracht Can you send me example of V2 plugin. Because what I've read from docs, they allow just one public version for one brand.
We could try releaase Mautic Oauth2, but I am not sure if they approve it.

@npracht
Copy link
Member

npracht commented Jan 29, 2020

Then that means they changed their policy (which can be understandable).
@kuzmany can you ask their support concerning the fact that current Zaps are not broken when we deliver this kind of update ?

@kuzmany
Copy link
Member Author

kuzmany commented Jan 29, 2020

@npracht Based on docs, not broken for them. If you use Zap for Mautic Zapier 2.16 then you stay on that version forever. You can migrate that user in dev platform, but not necessary.

@escopecz
Copy link
Sponsor Member

We can have Zapier users on 2 versions simultaneously for short period of time. We can even announce (I think, never done that) that hey will have to reauthenticate if they want to migrate to the newer version. And schedule the old version to disappear after 6 months or so. But there is still the need for hundreds of users to do some manual work with low value (from basic user standpoint) they gain for it. I would like to avoid that if possible. This may be another topic to poll the community about.

@npracht
Copy link
Member

npracht commented Jan 29, 2020

Let do it like that. @escopecz there is a valuable reason: security. Basic Auth should not exists.

@escopecz
Copy link
Sponsor Member

Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Can you also update Readme with how to test now? For example how to generate the access token?

authentication.js Outdated Show resolved Hide resolved
test/authentication.js Show resolved Hide resolved
test/authentication.js Show resolved Hide resolved
@RCheesley
Copy link
Sponsor Member

I think that it would be best if we were to have a v2 of the app, and make it clear to folks when we are going to deprecate the v1.

That way it does not immediately break things but allows people the time to transition over to the new version.

Does that make sense?

polling sample in Zap includes fields not found in latest task history ; extra keys: "dateModified", "lastActive", "modifiedBy", "modifiedByUser" (T006)
for PHPStorm
@escopecz
Copy link
Sponsor Member

@kuzmany I tested this PR, but the majority of the automated functional tests are failing. I've created a script to get the access token (see 6fc918d) to make the testing faster. Are the tests passing for you?

@kuzmany
Copy link
Member Author

kuzmany commented Oct 5, 2021

@escopecz when I build it, test passed (without it we were not able to upload it ).
I can check it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants