Skip to content

Upgrade Doorkeeper gem to 5.0.2

Josianne Hyson requested to merge jhyson/doorkeeper_upgrade_5 into master

What does this MR do?

Upgrade the doorkeeper gem to 5.0.2 and doorkeeper-openid_connect to 1.5.4 -> this addresses security vulnerability CVE-2019-9837.

This is addressing #27383 (closed)

Migration Notes:

Taken from https://github.com/doorkeeper-gem/doorkeeper/wiki/Migration-from-old-versions#from-4x-to-5x

Database changes:

  • Doorkeeper::Application now has a new boolean column named confidential that is true by default and has NOT NULL CONSTRAINT. This column is required to allow creating Public & Private Clients as mentioned in Section 8.5 of draft-ietf-oauth-native-apps-12 of OAuth 2 RFC which was previously unavailable. If you are migrating from the Doorkeeper <= 5.0, then you can easily add this column by generating a proper migration file using the following command: rails g doorkeeper:confidential_applications.

API changes:

# https://gitlab.com/oauth/token/info  - doorkeeper 4.4.3 (production)
{
    "resource_owner_id": 1,
    "scopes": ["api"],
    "expires_in_seconds": null,
    "application": {"uid": "1cb242f495280beb4291e64bee2a17f330902e499882fe8e1e2aa875519cab33"},
    "created_at": 1575890427
}
# http://localhost:3000/oauth/token/info  - doorkeeper 5.0.2
{
    "resource_owner_id": 1,
    "scope": ["api"],
    "expires_in": null,
    "application": {"uid": "1cb242f495280beb4291e64bee2a17f330902e499882fe8e1e2aa875519cab33"},
    "created_at": 1575890427
}
  • Doorkeeper#configured?, Doorkeeper#database_installed?, and Doorkeeper#installed? methods were removed, so any Doorkeeper ORM extension doesn't need to support these methods starting from 5.0.
    • As far as I can tell, this doesn't affect us. None of these methods are being called in the codebase.
  • Many memoized and other instance variables (like @token in doorkeeper_token method for Doorkeeper::Helpers::Controller) were renamed during refactoring, so if you are using them — just don't do it and call the original methods (helpers, etc) in order to get the required value.
    • I can't find any obvious instances of us using these methods or instance variables, so I don't think there's anything to do here.
  • Test suite now has a refactored infrastructure: spec_helper_integration now renamed to industry-standard spec_helper.
    • This was internal to the gem, so I don't think it needs any changes on our side.
  • custom_access_token_expires_in option now provides a Doorkeeper::OAuth::Authorization::Context object (|context|) instead of raw params (|client, grant_type, scopes|). The context object has all these variables and you can access them in the block (like context.grant_type or context.client).
    • We are not using this method, so no changes required here.
  • admin_authenticator block now returns "403 Forbidden" response by default if developer didn't declare another behavior explicitly.
    • This was affecting the Oauth::ApplicationsController, which inherits from Doorkeeper::ApplicationsController. As this controller is not for the admins, but for the user, and it already has and authenitcate_user action, I have added the skip_before_action :authenticate_admin. This does not affect our admin panel applications, as Admin::ApplicationsController inherits from Admin::ApplicationController, which already enforces admin access.
  • Previously authorization code response route was /oauth/authorize/<code>, now it is oauth/authorize/native?code=<code> (in order to help applications to automatically find the code value). 
# doorkeeper 4.3.2 (master)
   native_oauth_authorization GET    /oauth/authorize/native(.:format)            oauth/authorizations#show
          oauth_authorization GET    /oauth/authorize(.:format)                   oauth/authorizations#new
                              DELETE /oauth/authorize(.:format)                   oauth/authorizations#destroy
                              POST   /oauth/authorize(.:format)                   oauth/authorizations#create
oauth_authorized_applications GET    /oauth/authorized_applications(.:format)     oauth/authorized_applications#index
 oauth_authorized_application DELETE /oauth/authorized_applications/:id(.:format) oauth/authorized_applications#destroy
         oauth_jira_authorize GET    /login/oauth/authorize(.:format)             oauth/jira/authorizations#new
# doorkeeper 5.0.2
   native_oauth_authorization GET    /oauth/authorize/native(.:format)            oauth/authorizations#show
          oauth_authorization GET    /oauth/authorize(.:format)                   oauth/authorizations#new
                              DELETE /oauth/authorize(.:format)                   oauth/authorizations#destroy
                              POST   /oauth/authorize(.:format)                   oauth/authorizations#create
oauth_authorized_applications GET    /oauth/authorized_applications(.:format)     oauth/authorized_applications#index
 oauth_authorized_application DELETE /oauth/authorized_applications/:id(.:format) auth/authorized_applications#destroy
         oauth_jira_authorize GET    /login/oauth/authorize(.:format)             oauth/jira/authorizations#new

Other changes:

  • Bootstrap CSS was updated from 3.x to 4.0.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by 🤖 GitLab Bot 🤖

Merge request reports