Upgrade Doorkeeper gem to 5.0.2
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 namedconfidential
that istrue
by default and hasNOT 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
.- This is being addressed separately in the 4.4.3 upgrade: !20953 (merged)
API changes:
-
[IMPORTANT]: Doorkeeper JSON responses changed: scopes
field was replaced withscope
,expires_in_seconds
toexpires_in
to be consistent and match the RFC.- Result: For backwards compatibility I've elected to include the old params in the new response as well, so that we can communicate a proper deprecation of the replaced attributes in a future release.
- This was updated in this pull request: https://github.com/doorkeeper-gem/doorkeeper/pull/1114
- The documentation for this endpoint is available as part of the gem here: https://github.com/doorkeeper-gem/doorkeeper/wiki/API-endpoint-descriptions-and-examples#get----oauthtokeninfo
- We also briefly mention it here: https://docs.gitlab.com/ee/api/oauth2.html#implicit-grant-flow
- I can't see any usages of this endpoint on gitlab.com - but this might still be used on self hosted instances?
- This is affecting us, as when I make this request on
doorkeeper 4.4.3 (master)
anddoorkeeper 5.0.2
I see the updated API responses:
# 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?
, andDoorkeeper#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
indoorkeeper_token
method forDoorkeeper::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-standardspec_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 aDoorkeeper::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 (likecontext.grant_type
orcontext.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 fromDoorkeeper::ApplicationsController
. As this controller is not for the admins, but for the user, and it already has andauthenitcate_user
action, I have added theskip_before_action :authenticate_admin.
This does not affect our admin panel applications, asAdmin::ApplicationsController
inherits fromAdmin::ApplicationController
, which already enforces admin access.
- This was affecting the
-
Previously authorization code response route was /oauth/authorize/<code>
, now it isoauth/authorize/native?code=<code>
(in order to help applications to automatically find the code value).-
This was actually originally updated in 4.3 (https://github.com/doorkeeper-gem/doorkeeper/wiki/Migration-from-old-versions#from-4x-to-43x) - which we're already running on master. It was added to these release notes as part of https://github.com/doorkeeper-gem/doorkeeper/issues/1143 which added the option to opt-out of this breaking change in future releases.
Running
rails routes | grep "oauth/authorize"
produces the following outputs - showing there is no difference here from what's running on master.
-
# 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
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
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