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

Fix #7854 Adds authorization code grant type flow to Api V8 OAuth2 #8291

Open
wants to merge 10 commits into
base: hotfix-7.10.x
Choose a base branch
from

Conversation

HVStechnik
Copy link

@HVStechnik HVStechnik commented Nov 21, 2019

As discussed in #7854 this adds the OAuth2 authorization code grant type flow to the Api V8. The code leverages thephpleague's OAuth2 Server and follows the implementation guidelines.

The PR covers the following main changes:

  • It adds a new module to store OAuth2 authorization codes. The module key is OAuth2AuthCodes
  • It implements the authorization code grant flow in the Api V8 middleware and stores the authorization codes in the newly created OAuth2AuthCodes module

The OAuth2 authorization code flow includes the following steps:

  1. A client redirects the user to the SuiteCRM instance (path-to-instance/Api/authorize) with a client_id and redirect_uri parameter.
  2. SuiteCRM validates that the client_id and the redirect_uri correspond to a valid OAuth2 Client and redirects the user to the login page (if not logged-in already). Once logged-in the user is requested to authorize the client. This takes place on a custom view (view.authorize.php) in the new module. The user can:
    a. decline the request, which redirects the user back to the client (the redirect contains an approapriate hint)
    b. authorize the client, which creates a new authorization code. The code is saved as a new record in the OAuth2AuthCode module. The user is redirected to the client. The redirect url contains the authorization code.
  3. The client than sends a direct request containing the authorization code to SuiteCRMs Api/access_token access point. The OAuth2 Server then again validates the client and additionally the client secret and the authorization code. If both are valid and match a new access_token on behalf of the authorizing user is returned. This access_token now grants the client access to the V8 Api. The client thereby acts as the authorizing user.

Motivation and Context

Access to the V8 Api is currently granted by either a client credentials grant or a password grant. The client credentials grant has the downside, that it allows to access the Api on behalf of a dedicated user only (the user is defined in the admin area). The password grant has the downside that the client system needs to handle the user's SuiteCRM password to obtain access to the Api, which is a potential security risk. The authoriazion code grant solves these issues.

Additional feature and implementation description

  1. In line with similar implementations in other systems, we offer the user a third option (besides authorizing and declining the request): The user can choose to save the authorization decision for future requests. We thenflag the OAuth2AuthCode record. The flag first prevents that the record is deleted once the authorization code is expired (the code though does expire). Second, if a new authorization code is requested by that particular client AND if the corresponding user is logged-in into SuiteCRM a new authorization code is returned without showing the decision page. The authorize view looks as follows (my color scheme is blue...):
    grafik

  2. The OAuth2AuthCodes module appears in the module list, because users can access the list view. For normal users, the list view lists the authorization codes for that particular user. For admins, the list view lists all authorization codes. The list view is accessible for normal users, because they can revoke their authorization codes there. This is in particular relevant for the authoriation codes that have been saved for automatic approval of future requests (see point 4).
    grafik

  3. If a client redirects a user to the SuiteCRM instance (to obtain an auth code) and if that user is not logged-in already, the user will be redirected to the login page (the user logs-in in order to obtain the authorization code for the client). In this case, the user is logged-out automatically once the authorization request has been approved or declined. This is because we do not want to keep a logged-in SuiteCRM session, the user might not be aware of, because once the user is redirected to the client, there is no open SuiteCRM window any more.

How To Test This

  • The OAuth2AuthCode module should be visible, if activated in the menu.
  • Run the Api V8 activation robo task
  • Create a new authoriation code client in the admin OAuth2 Clients section
  • Ensure that the htaccess RewriteRule is in place.
  • Access: path-to-instance/Api/authorize?response_type=code&client_id=[client identifier]&redirect_uri=[redirect_url]
  • Once an authoriation code is returned, you can use this auth code with the client id and the client secret to obtain an access token. This can be tested in Postman.

Please ask, if there are any questions! I'm happy to furhter explain and improve the code in case of any recommendations.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@HVStechnik HVStechnik force-pushed the feature/7854-api-authorization-code-grand branch from ed747b2 to d317328 Compare November 21, 2019 21:01
Copy link
Contributor

@Dillon-Brown Dillon-Brown left a comment

Choose a reason for hiding this comment

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

Left a few minor comments, nice work!

Api/V8/OAuth2/Repository/AuthCodeRepository.php Outdated Show resolved Hide resolved
Api/V8/OAuth2/Repository/AuthCodeRepository.php Outdated Show resolved Hide resolved
Api/V8/OAuth2/Repository/AuthCodeRepository.php Outdated Show resolved Hide resolved
Api/V8/OAuth2/Repository/AuthCodeRepository.php Outdated Show resolved Hide resolved
modules/OAuth2AuthCodes/language/en_us.lang.php Outdated Show resolved Hide resolved
modules/OAuth2AuthCodes/language/en_us.lang.php Outdated Show resolved Hide resolved
'view_print' => false,
);

public function display()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required but it would be good to reduce the cognitive complexity of this function.

Copy link
Author

Choose a reason for hiding this comment

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

That is absolutely true. I thought about moving the decision points (show the authorization page or redirect once the decision has been made) to the controller class. However, we need the \Slim\App and \AuthorizationServer instances to validate the request, show the decision form, and to send the response. Since I cannot send the $app and $server instances to the display() function as params (the display function is called outside the module), there is no way to use the same AuthorizationServer instance in the controller and the view classes. I did not want to create those instances twice and thus ended up doing everything in the display function.
There are two options that might work:

  1. Attach the \Slim\App and \AuthorizationServer instances to an instance of \OAuth2AuthCodes, which is created at the beginning of the display() function. We could then use multiple functions of the \OAuth2AuthCodes class to do the logic.
  2. Split the display function into several function within the Oauth2AuthCodesViewAuthorize class. That might be easier to read, but still has the logic within the view class, where is should not be...

@Dillon-Brown: Do you have a preferred option? Or shall I leave the complex display function as it is for now?

modules/OAuth2AuthCodes/views/view.authorize.php Outdated Show resolved Hide resolved
@Dillon-Brown
Copy link
Contributor

Also looks like Travis is failing the acceptance tests:

1) CompanyModuleCest: Select record from list view
 Test  tests/acceptance/Core/CompanyModuleCest.php:testScenarioViewRecordFromListView
                                                                                                                                                                                                                                                                                                                                    
  [Facebook\WebDriver\Exception\NoSuchElementException] no such element: Unable to locate element: {"method":"css selector","selector":".listViewBody"}
  (Session info: headless chrome=78.0.3904.108)
  (Driver info: chromedriver=2.36.540471 (9c759b81a907e70363c6312294d30b6ccccc2752),platform=Linux 4.15.0-1028-gcp x86_64)  
                                                                                                                                                                                                                                                                                                                                    
Scenario Steps:
 15. $I->waitForElementVisible(".listViewBody") at tests/_support/Step/Acceptance/ListView.php:51
 14. $I->click("CompanyTestModule","#toolbar.desktop-toolbar  > ul.nav.navb...") at tests/_support/Step/Acceptance/NavigationBar.php:95
 13. $I->waitForElementVisible("#toolbar.desktop-toolbar  > ul.nav.navbar-n...") at tests/_support/Step/Acceptance/NavigationBar.php:94
 12. $I->click("All","#toolbar.desktop-toolbar  > ul.nav.navbar-nav > li.to...") at tests/_support/Step/Acceptance/NavigationBar.php:92
 11. $I->waitForElementVisible("#toolbar.desktop-toolbar  > ul.nav.navbar-n...") at tests/_support/Step/Acceptance/NavigationBar.php:91
 10. $I->executeJS("return Math.max(document.documentElement.clientWidth, w...") at tests/_support/Page/Design.php:50
#1  /home/travis/build/salesagility/SuiteCRM/vendor/facebook/webdriver/lib/Exception/WebDriverException.php:102
#2  /home/travis/build/salesagility/SuiteCRM/vendor/facebook/webdriver/lib/Remote/HttpCommandExecutor.php:320
#3  /home/travis/build/salesagility/SuiteCRM/vendor/facebook/webdriver/lib/Remote/RemoteWebDriver.php:535
#4  /home/travis/build/salesagility/SuiteCRM/vendor/facebook/webdriver/lib/Remote/RemoteWebDriver.php:176
#5  /home/travis/build/salesagility/SuiteCRM/vendor/facebook/webdriver/lib/WebDriverExpectedCondition.php:186
#6  Facebook\WebDriver\WebDriverExpectedCondition::Facebook\WebDriver\{closure}
#7  /home/travis/build/salesagility/SuiteCRM/vendor/facebook/webdriver/lib/WebDriverWait.php:67
#8  Codeception\Module\WebDriver->waitForElementVisible
#9  /home/travis/build/salesagility/SuiteCRM/tests/_support/_generated/AcceptanceTesterActions.php:2386
#10 /home/travis/build/salesagility/SuiteCRM/tests/_support/Step/Acceptance/ListView.php:51

travis

@HVStechnik HVStechnik force-pushed the feature/7854-api-authorization-code-grand branch from a9607ba to 52704b4 Compare November 22, 2019 09:49
code-ph0y
code-ph0y previously approved these changes Nov 22, 2019
Dillon-Brown
Dillon-Brown previously approved these changes Nov 22, 2019
@codecov-io
Copy link

codecov-io commented Nov 27, 2019

Codecov Report

Merging #8291 into hotfix-7.10.x will increase coverage by 0.01%.
The diff coverage is 3.01%.

@@                Coverage Diff                @@
##           hotfix-7.10.x    #8291      +/-   ##
=================================================
+ Coverage          10.57%   10.58%   +0.01%     
=================================================
  Files               3226     3240      +14     
  Lines             239933   240356     +423     
=================================================
+ Hits               25361    25444      +83     
- Misses            214572   214912     +340

@HVStechnik
Copy link
Author

HVStechnik commented Nov 27, 2019

I just fould and solved the issue that caused Travis to fail.

@samus-aran
Copy link
Contributor

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 6
           

See the complete overview on Codacy

@HVStechnik
Copy link
Author

Hi SalesAgility team,
how do we proceed with this PR? Do I have to rebase to a different branch?
I have this code running in a production system. Merging this PR in one of the next releases would simplify the update process on this production system significantly. Thanks!

@codecov-commenter
Copy link

Codecov Report

Merging #8291 into hotfix-7.10.x will decrease coverage by 0.00%.
The diff coverage is 3.95%.

@@                Coverage Diff                @@
##           hotfix-7.10.x    #8291      +/-   ##
=================================================
- Coverage          10.70%   10.69%   -0.01%     
=================================================
  Files               3230     3243      +13     
  Lines             241058   241384     +326     
=================================================
+ Hits               25798    25811      +13     
- Misses            215260   215573     +313     

@HVStechnik
Copy link
Author

I've resolved conflics that emerged due to recent updates on the hotfix branch.
@Dillon-Brown @code-ph0y: Could you please take another look at this one, so that we can proceed? As mentioned previously, we're using this in a production system. Updates to that system would become much easier, if this PR get's merged...
Thanks!

@SuiteBot
Copy link

SuiteBot commented Aug 27, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ code-ph0y
❌ HamburgischerVereinSeefahrt
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

None yet

8 participants