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

[TPROD-168] Remove OAuth1 support #10111

Merged
merged 8 commits into from
Jun 16, 2021

Conversation

dennisameling
Copy link
Member

@dennisameling dennisameling commented May 28, 2021

Q A
Branch? features
Bug fix? no
New feature? no
Deprecations? no
BC breaks? yes
Automated tests included? N/A
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#202
Issue(s) addressed Fixes #...

Description:

As discussed in https://mautic.atlassian.net/browse/TPROD-190, this PR removes OAuth1 support from Mautic due to the increasing workload to keep supporting it

Steps to test this PR:

  1. Load up this PR
  2. In the top right corner of the screen, click the gear icon > API credentials
  3. In every dropdown, you will now only have the OAuth2 option left

image

image

Ensure OAuth2 is still working by doing the following:

  1. Go to the API credentials screen mentioned above
  2. Click "new"
  3. Enter the following data (we won't use the Redirect URI thanks to TPROD-170: Enabling 2-legged authentication for Mautic #9837, so you can fill out anything there)

image

  1. You will now see a public and secret key that you can use
  2. Open Postman
  3. Make sure you exactly copy the options below:

image

  1. After you got the token (which is the confirmation that OAuth2 still works), you can execute the API call with "Send"

@dennisameling dennisameling added WIP PR's that are not ready for review and are currently in progress dependencies Pull requests that update a dependency file mautic-4 Relates to Mautic 4.x labels May 28, 2021
@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label May 28, 2021
@dennisameling dennisameling added this to the 4.0-rc milestone May 28, 2021
@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #10111 (8782943) into features (4df7500) will increase coverage by 0.05%.
The diff coverage is 41.30%.

Impacted file tree graph

@@              Coverage Diff               @@
##             features   #10111      +/-   ##
==============================================
+ Coverage       41.56%   41.62%   +0.05%     
+ Complexity      34581    34469     -112     
==============================================
  Files            2063     2050      -13     
  Lines          111574   111196     -378     
==============================================
- Hits            46378    46281      -97     
+ Misses          65196    64915     -281     
Impacted Files Coverage Δ
.../bundles/ApiBundle/Controller/ClientController.php 0.00% <0.00%> (ø)
app/bundles/ApiBundle/Event/ClientEvent.php 0.00% <0.00%> (ø)
.../bundles/ApiBundle/EventListener/ApiSubscriber.php 51.38% <ø> (ø)
app/bundles/ApiBundle/MauticApiBundle.php 100.00% <ø> (ø)
app/bundles/ApiBundle/Model/ClientModel.php 10.63% <0.00%> (+0.43%) ⬆️
app/bundles/ApiBundle/Form/Type/ClientType.php 70.58% <65.51%> (+21.60%) ⬆️

Copy link
Contributor

@nickveenhof nickveenhof left a comment

Choose a reason for hiding this comment

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

Other than this comment I didn't test nor review the rest. But I'm all for removing this and I love seeing the composer.json cleanup. Only thing left is FOSOAuthServerBundle. Can that potentially be tackled in the same PR?

@@ -8,7 +8,7 @@
<IfModule mod_rewrite.c>
RewriteEngine On

# Set Authorization header for OAuth1a for when php is running under fcgi
# Set Authorization header for OAuth for when php is running under fcgi
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency is key, shouldn't this be OAuth2? :)

@RCheesley
Copy link
Sponsor Member

Only thing left is FOSOAuthServerBundle. Can that potentially be tackled in the same PR?

I think we cannot do that until they merge the PR that was made to reinstate PHP template support. That is why we have the forked repo.

@nickveenhof
Copy link
Contributor

Only thing left is FOSOAuthServerBundle. Can that potentially be tackled in the same PR?

I think we cannot do that until they merge the PR that was made to reinstate PHP template support. That is why we have the forked repo.

Right - although it is confusing, as the branch that is used in Mautic is not the mautic-compat branch but the dev-doctrine-fix branch. I'm not sure if that even has anything to do with the php template thing?

@nickveenhof
Copy link
Contributor

FriendsOfSymfony/FOSOAuthServerBundle@master...dennisameling:doctrine-fix this is the difference afaik, not sure if it has anything to do with oauth1 or not.

@dennisameling
Copy link
Member Author

Hi @nickveenhof - I rebased this PR but am getting an error on composer update --lock:

$ composer update --lock
Loading composer repositories with package information
Updating dependencies                                 
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires mautic/core-lib == 3.3.9999999.9999999-dev, it is found mautic/core-lib[3.3.x-dev] in the lock file and mautic/core-lib[dev-master] from path repo (app) but it does not match your constraint and is therefore not installable. Make sure you either fix the constraint or avoid updating this package to keep the one from the lock file.

Any suggestions?

@dennisameling
Copy link
Member Author

dennisameling commented Jun 9, 2021

Only thing left is FOSOAuthServerBundle. Can that potentially be tackled in the same PR?

Nope, sorry. TL;DR: we need that until the FOSOAuthServerBundle team releases a new version (which might take months), or for Mautic to fork it and maintain it ourselves.

Let's not confuse two things here:

An option would be to fork FOSOAuthServerBundle 1.x in the Mautic org, try to see if it's easy to add PHP 8 support to it, then release it to Composer. Would be a workaround for the time being. But I don't have any capacity to look into that now.

@dennisameling dennisameling added ready-to-test PR's that are ready to test and removed WIP PR's that are not ready for review and are currently in progress labels Jun 14, 2021
@dennisameling dennisameling changed the title [WIP] Remove OAuth1 support Remove OAuth1 support Jun 14, 2021
@dennisameling dennisameling marked this pull request as ready for review June 14, 2021 19:13
@dennisameling dennisameling requested a review from a team June 14, 2021 19:13
"url": "app",
"options": {
"versions": {
"mautic/core-lib": "3.2.x-dev"
Copy link
Member Author

Choose a reason for hiding this comment

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

The folks at Symfony also do it this way. Don't ask me why or how it works 🙈

Here's the official Composer documentation for this setting: https://getcomposer.org/doc/05-repositories.md#path

Copy link
Member Author

Choose a reason for hiding this comment

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

@dennisameling dennisameling changed the title Remove OAuth1 support [TPROD-168] Remove OAuth1 support Jun 14, 2021
@dennisameling dennisameling added the T2 Medium difficulty to fix (issue) or test (PR) label Jun 14, 2021
Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

Checked that the dropdowns now only include OAuth2 - no more OAuth1 🎉

Was able to authenticate with OAuth2 per the instructions and was able to retrieve the contacts list!

Thanks for making this PR @dennisameling - just needs another tester and a code review from @mautic/core-team

@RCheesley RCheesley added code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Jun 14, 2021
@RCheesley RCheesley requested a review from a team June 14, 2021 20:24
@ts-navghane
Copy link
Contributor

Reviewed and tested, LGTM! Works as expected. Thank you @dennisameling.

@ts-navghane ts-navghane added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged code-review-needed PR's that require a code review before merging labels Jun 16, 2021
@RCheesley RCheesley merged commit 959c72f into mautic:features Jun 16, 2021
@dennisameling dennisameling deleted the remove-oauth1 branch June 16, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The PR contributors have signed the contributors agreement dependencies Pull requests that update a dependency file mautic-4 Relates to Mautic 4.x ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T2 Medium difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants