Prevent status webservices from being returned on the providers endpoint #2640

mFelgate · 2022-08-31T17:44:05Z

Desired Outcome

Duplictae authenticators show up in the providers endpoint due to not removing the status webservices from the list of providers. This pr removes web services that end in /status from the list of authenticators

Implemented Changes

Describe how the desired outcome above has been achieved with this PR. In
particular, consider:

What's changed? Why were these changes made?
Added the regex used in InstalledAuthenticators to remove the status webservices
How should the reviewer approach this PR, especially if manual tests are required?
That the tests work effectively and the code is reasonably made
Are there relevant screenshots you can add to the PR description?
No

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue link: ONYX-25530

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

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

micahlee · 2022-08-31T20:32:28Z

app/db/repository/authenticator_repository.rb

+        ).where(
+          Sequel.like(
+            identifier,
+            %r{conjur/(authn(?:-[^/]+)?(?:/[^/]+)?)$}


It would be worth adding a comment here explaining how this regex prevents the status resources from being included in the result. (No forward slashes are allowed after authn).

Also, given this isn't an urgent change, would should improve this re-use beyond just copying the regex between this location and https://github.com/cyberark/conjur/blob/master/app/domain/authentication/installed_authenticators.rb#L31.

For example, it should probably be a constant in one class or the other, and just reference in the other location:

# installed_authenticators.rb ... class InstalledAuthenticators ... AUTHN_RESOURCE_PREFIX = "conjur/authn-" AUTHN_STATUS_FILTER = %r{conjur/(authn(?:-[^/]+)?(?:/[^/]+)?)$} ... # filter out nested status webservice .map { |id| id[AUTHN_STATUS_FILTER, 1] } ...

# authenticator_repository.rb ... ... Sequel.like( identifier, # filter out nested status webservice Authentication::InstalledAuthenticators::AUTHN_STATUS_FILTER ) ...

micahlee · 2022-08-31T20:33:21Z

spec/app/db/repository/authenticator_repository_spec.rb

+                )
+              end
+
+            it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec').length).to eq(2) }


How cumbersome would it be to actually verify the contents of the list, rather than just the count?

Being explicit about the expected outcome would be a more valuable assurance than just the number of results.

would not work since we and not brining over the real resource_id in the repository, but a created one based on the service_id which is the same between the status and normal web-services.

imheresamir · 2022-09-01T00:37:55Z

When you get a chance could you fill out the PR description @mFelgate ? Thanks

codeclimate · 2022-10-04T16:17:56Z

app/domain/authentication/installed_authenticators.rb

@@ -4,6 +4,7 @@ module Authentication
  class InstalledAuthenticators

    AUTHN_RESOURCE_PREFIX = "conjur/authn-"
+    AUTHN_STATUS_FILTER = %r{conjur/(authn(?:-[^/]+)?(?:/[^/]+)?)$}


Freeze mutable objects assigned to constants.

codeclimate · 2022-10-04T17:42:06Z

CHANGELOG.md


 ### Changed
 - AWS Access Key Rotation now preserves only one key

+### Fixed
+- Removed Status webservices from the list providers endpoint


Lists should be surrounded by blank lines

jvanderhoof · 2022-10-07T17:24:11Z

CHANGELOG.md

@@ -35,6 +39,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Changed
 - Reduces debug log verbosity.
  [cyberark/conjur#2639](https://github.com/cyberark/conjur/pull/2639)
+- Remove status webservices from providers endpoints


Each changelog entry should go under a single header, not multiple. I think this makes more sense under Fixed.

jvanderhoof · 2022-10-07T17:25:40Z

CHANGELOG.md


 ### Changed
 - AWS Access Key Rotation now preserves only one key

+### Fixed
+- Removed Status webservices from the list providers endpoint


I'd recommend something like:

Suggested change

- Removed Status webservices from the list providers endpoint

- OIDC Provider endpoint no longer includes duplicates when Status is enabled.

jvanderhoof · 2022-10-07T18:55:02Z

I'd actually recommend making the changes in the DB::Repository::AuthenticatorRepository class. If we updated the initializer and the find_all method to something like:

module DB
  module Repository
    class AuthenticatorRepository
      def initialize(data_object:, resource_repository: ::Resource, logger: Rails.logger, enabled_authenticators: Rails.application.config.conjur_config.authenticators)
        @resource_repository = resource_repository
        @data_object = data_object
        @logger = logger
        @enabled_authenticators = enabled_authenticators
      end

      def find_all(type:, account:)
        enabled_authenticator_types = @enabled_authenticators
          .select { |authenticator| authenticator.match(/^#{type}/) }
          .map { |authenticator| "#{account}:webservice:conjur/#{authenticator}" }

        @resource_repository.where(
          Sequel.like(
            :resource_id,
            "#{account}:webservice:conjur/#{type}/%"
          )
        )
          .all
          .select { |webservice| enabled_authenticator_types.include?(webservice.id) }
          .map do |webservice|
            load_authenticator(account: account, id: webservice.id.split(':').last, type: type)
          end
          .compact
      end
      ...

we can limit the available authenticators to those enabled AND strip out the Status webservices.

codeclimate · 2022-10-11T15:00:00Z

app/db/repository/authenticator_repository.rb

-        @resource_repository.where(
+        enabled_authenticator_types = @enabled_authenticators.select { |authenticator| authenticator.match("#{type}") }
+                .map { |authenticator| "#{account}:webservice:conjur/#{authenticator}" }
+       @resource_repository.where(


Inconsistent indentation detected.

codeclimate · 2022-10-11T15:00:00Z

app/db/repository/authenticator_repository.rb

      end

      def find_all(type:, account:)
-        @resource_repository.where(
+        enabled_authenticator_types = @enabled_authenticators.select { |authenticator| authenticator.match("#{type}") }
+                .map { |authenticator| "#{account}:webservice:conjur/#{authenticator}" }


Use 2 (not 8) spaces for indenting an expression in an assignment spanning multiple lines.

codeclimate · 2022-10-11T15:00:00Z

app/db/repository/authenticator_repository.rb

          Sequel.like(
            :resource_id,
            "#{account}:webservice:conjur/#{type}/%"
          )
-        ).all.map do |webservice|
+        ).all.select { |webservice|enabled_authenticator_types.include?(webservice.resource_id) }.map do |webservice|


Space after closing | missing.

codeclimate · 2022-10-11T15:00:01Z

app/db/repository/authenticator_repository.rb

      end

      def find_all(type:, account:)
-        @resource_repository.where(
+        enabled_authenticator_types = @enabled_authenticators.select { |authenticator| authenticator.match("#{type}") }


Prefer to_s over string interpolation.

codeclimate · 2022-10-11T15:00:03Z

Code Climate has analyzed commit de1e574 and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	7
Complexity	1

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

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

View more on Code Climate.

mFelgate requested a review from a team as a code owner August 31, 2022 17:44

mFelgate force-pushed the Provider-status-filter branch from 7f33977 to 1dc8916 Compare August 31, 2022 17:45

micahlee reviewed Aug 31, 2022

View reviewed changes

mFelgate force-pushed the Provider-status-filter branch from 1dc8916 to 19a5d3f Compare September 26, 2022 17:47

mFelgate force-pushed the Provider-status-filter branch 2 times, most recently from 041d9d0 to 66dbe15 Compare October 4, 2022 15:53

codeclimate bot reviewed Oct 4, 2022

View reviewed changes

mFelgate force-pushed the Provider-status-filter branch from 66dbe15 to 06a4f77 Compare October 4, 2022 17:00

codeclimate bot reviewed Oct 4, 2022

View reviewed changes

jvanderhoof reviewed Oct 7, 2022

View reviewed changes

mFelgate force-pushed the Provider-status-filter branch from 06a4f77 to 588abed Compare October 7, 2022 18:20

Prevent status webservices from being returned on the providers endpoint

de1e574

mFelgate force-pushed the Provider-status-filter branch from 588abed to de1e574 Compare October 11, 2022 14:34

codeclimate bot reviewed Oct 11, 2022

View reviewed changes

conjur-jenkins force-pushed the master branch from ec608eb to a787ad8 Compare April 30, 2025 17:07

ismarc force-pushed the master branch from a787ad8 to 78664ae Compare June 6, 2025 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent status webservices from being returned on the providers endpoint #2640

Prevent status webservices from being returned on the providers endpoint #2640

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	- Removed Status webservices from the list providers endpoint
	- OIDC Provider endpoint no longer includes duplicates when Status is enabled.

Prevent status webservices from being returned on the providers endpoint #2640

Are you sure you want to change the base?

Prevent status webservices from being returned on the providers endpoint #2640

Uh oh!

Conversation

Uh oh!

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!