Remove support for disabling the non-PKCE OIDC authentication flow #2713

jvanderhoof · 2023-02-02T18:49:12Z

Desired Outcome

Removes backward compatibility for setting State and Nonce values. The functionality removed here was disabled by default for the last release.

Implemented Changes

This PR migrates the functionality encapsulated in the pkce_support_feature namespace to the default v2 namespace.

*Describe how the desired outcome above has been achieved with this PR. In

Connected Issue/Story

N/A

CyberArk internal issue ID: [insert issue ID]

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a
CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code
changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: [insert issue ID]
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Disabling the feature flag was never documented, thus, there is no need to update the docs.

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

codeclimate · 2023-02-02T20:43:01Z

app/domain/authentication/authn_oidc/v2/views/provider_context.rb

-                )
-              }
-            end
+              begin


Redundant begin block detected.

codeclimate · 2023-02-02T20:43:01Z

app/domain/authentication/authn_oidc/v2/client.rb

+            bearer_token = oidc_client.access_token!(**access_token_args)
+          rescue Rack::OAuth2::Client::Error => e
+            # Only handle the expected errors related to access token retrieval.
+            case e.message


Authentication::AuthnOidc::V2::Client#callback calls 'e.message' 2 times

codeclimate · 2023-02-02T20:43:01Z

app/domain/authentication/authn_oidc/v2/client.rb

+              raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed,
+                    'Authorization code is invalid'
+            end
+            raise e


Authentication::AuthnOidc::V2::Client#callback calls 'raise e' 2 times

codeclimate · 2023-02-02T20:43:02Z

app/domain/authentication/authn_oidc/v2/views/provider_context.rb

-              }
-            end
+              begin
+                nonce = @random.hex(25)


Authentication::AuthnOidc::V2::Views::ProviderContext#call calls '@random.hex(25)' 2 times

codeclimate · 2023-02-02T20:43:02Z

app/domain/authentication/authn_oidc/v2/views/provider_context.rb

+                  )
+                }
+              rescue Errors::Authentication::OAuth::ProviderDiscoveryFailed,
+                Errors::Authentication::OAuth::ProviderDiscoveryTimeout


Align the elements of an array literal if they span more than one line.

codeclimate · 2023-03-20T18:34:09Z

app/domain/authentication/authn_oidc/v2/strategy.rb

@@ -14,12 +13,18 @@ def initialize(
        end

        def callback(args)
-          # TODO: Check that `code` and `state` attributes are present
-          raise Errors::Authentication::AuthnOidc::StateMismatch unless args[:state] == @authenticator.state
+          # Note: `code_verifier` param is optional


Annotation keywords like Note should be all upper case, followed by a colon, and a space, then a note describing the problem.

codeclimate · 2023-03-20T18:34:09Z

app/domain/authentication/authn_oidc/v2/views/provider_context.rb

+                code_verifier = @random.hex(25)
+                code_challenge = @digest.base64digest(code_verifier).tr("+/", "-_").tr("=", "")
+                {
+                  service_id: authenticator.service_id,


Authentication::AuthnOidc::V2::Views::ProviderContext#call calls 'authenticator.service_id' 2 times

codeclimate · 2023-03-20T18:34:09Z

app/domain/authentication/authn_oidc/v2/views/provider_context.rb

          end

-          def generate_redirect_url(client:, authenticator:)
+          def generate_redirect_url(client:, authenticator:, nonce:, code_challenge:)


Authentication::AuthnOidc::V2::Views::ProviderContext#generate_redirect_url has 4 parameters

codeclimate · 2023-03-20T18:34:10Z

CHANGELOG.md

@@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
  [cyberark/slosilo#31](https://github.com/cyberark/slosilo/pull/31)
  [cyberark/conjur#2718](https://github.com/cyberark/conjur/pull/2718)

+### Changed
+- Removes support for disabling the `CONJUR_FEATURE_PKCE_SUPPORT_ENABLED` flag.


Lists should be surrounded by blank lines

micahlee

This LGTM! I only have one note on the changelog/version before approving.

micahlee · 2023-03-21T12:43:45Z

CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Nothing should go in this section, please add to the latest unreleased version
  (and update the corresponding date), or add a new version.

-## [1.19.3] - 2023-01-26
+## [1.19.2] - 2023-02-01


1.19.2 is already published, so 1.19.3 (or greater) is actually correct. This seems like more than a patch level change. So moving it to 1.20.0 might be appropriate here.

This commit removes the temporary PKCE flag required to safely release the feature in conjunction with the UI changes.

micahlee

LGTM!

codeclimate · 2023-03-21T14:54:42Z

CHANGELOG.md

+## [1.19.3] - 2023-03-21
+
+### Changed
+- Removes support for disabling the `CONJUR_FEATURE_PKCE_SUPPORT_ENABLED` flag.


Lists should be surrounded by blank lines

codeclimate · 2023-03-21T14:54:45Z

Code Climate has analyzed commit ef89314 and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2
Style	3
Complexity	7

The test coverage on the diff in this pull request is 89.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 90.2% (-1.3% change).

View more on Code Climate.

jvanderhoof force-pushed the revert-pkce-flag branch 2 times, most recently from e7a15ef to d6694ca Compare February 2, 2023 20:17

codeclimate bot reviewed Feb 2, 2023

View reviewed changes

jvanderhoof mentioned this pull request Feb 3, 2023

Tag oidc unit tests #2706

Merged

13 tasks

jvanderhoof force-pushed the revert-pkce-flag branch from d6694ca to 1487394 Compare March 20, 2023 17:57

codeclimate bot reviewed Mar 20, 2023

View reviewed changes

jvanderhoof force-pushed the revert-pkce-flag branch 2 times, most recently from a6a9b93 to c3ea3f9 Compare March 20, 2023 22:02

jvanderhoof marked this pull request as ready for review March 20, 2023 22:53

jvanderhoof requested a review from a team as a code owner March 20, 2023 22:53

micahlee reviewed Mar 21, 2023

View reviewed changes

Removes support for disabling the non-PKCE OIDC authentication flow

ef89314

This commit removes the temporary PKCE flag required to safely release the feature in conjunction with the UI changes.

jvanderhoof force-pushed the revert-pkce-flag branch from c3ea3f9 to ef89314 Compare March 21, 2023 14:28

micahlee approved these changes Mar 21, 2023

View reviewed changes

codeclimate bot reviewed Mar 21, 2023

View reviewed changes

jvanderhoof merged commit 92bb7a1 into master Mar 21, 2023

jvanderhoof deleted the revert-pkce-flag branch March 21, 2023 14:57

john-odonnell mentioned this pull request Apr 21, 2023

Release v1.19.3+suite.1 cyberark/conjur-oss-suite-release#265

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove support for disabling the non-PKCE OIDC authentication flow #2713

Remove support for disabling the non-PKCE OIDC authentication flow #2713

jvanderhoof commented Feb 2, 2023 •

edited

Loading

codeclimate bot Feb 2, 2023

codeclimate bot Feb 2, 2023

codeclimate bot Feb 2, 2023

codeclimate bot Feb 2, 2023

codeclimate bot Feb 2, 2023

codeclimate bot Mar 20, 2023

codeclimate bot Mar 20, 2023

codeclimate bot Mar 20, 2023

codeclimate bot Mar 20, 2023

micahlee left a comment

micahlee Mar 21, 2023

micahlee left a comment

codeclimate bot Mar 21, 2023

codeclimate bot commented Mar 21, 2023

Remove support for disabling the non-PKCE OIDC authentication flow #2713

Remove support for disabling the non-PKCE OIDC authentication flow #2713

Conversation

jvanderhoof commented Feb 2, 2023 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Feb 2, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 2, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 2, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 2, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 2, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 20, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 20, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 20, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 20, 2023

Choose a reason for hiding this comment

micahlee left a comment

Choose a reason for hiding this comment

micahlee Mar 21, 2023

Choose a reason for hiding this comment

micahlee left a comment

Choose a reason for hiding this comment

codeclimate bot Mar 21, 2023

Choose a reason for hiding this comment

codeclimate bot commented Mar 21, 2023

jvanderhoof commented Feb 2, 2023 •

edited

Loading