-
Notifications
You must be signed in to change notification settings - Fork 142
Prevent status webservices from being returned on the providers endpoint #2640
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,11 +13,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||||
|
|
||||||
| ### Added | ||||||
| - List resources request (`GET /resources`) now produce audit events. | ||||||
| ([cyberark/conjur#2652](https://github.com/cyberark/conjur/pull/2652) | ||||||
| [cyberark/conjur#2652](https://github.com/cyberark/conjur/pull/2652) | ||||||
|
|
||||||
| ### Changed | ||||||
| - AWS Access Key Rotation now preserves only one key | ||||||
|
|
||||||
| ### Fixed | ||||||
| - Removed Status webservices from the list providers endpoint | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend something like:
Suggested change
|
||||||
| [cyberark/conjur#2640](https://github.com/cyberark/conjur/pull/2640) | ||||||
|
|
||||||
| ## [1.18.4] - 2022-09-11 | ||||||
|
|
||||||
| ### Added | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,27 @@ | ||
| module DB | ||
| module Repository | ||
| class AuthenticatorRepository | ||
| def initialize(data_object:, resource_repository: ::Resource, logger: Rails.logger) | ||
| 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:) | ||
| @resource_repository.where( | ||
| enabled_authenticator_types = @enabled_authenticators.select { |authenticator| authenticator.match("#{type}") } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer |
||
| .map { |authenticator| "#{account}:webservice:conjur/#{authenticator}" } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use 2 (not 8) spaces for indenting an expression in an assignment spanning multiple lines. |
||
| @resource_repository.where( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent indentation detected. |
||
| 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| | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space after closing |
||
| load_authenticator(account: account, id: webservice.id.split(':').last, type: type) | ||
| end.compact | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ module Authentication | |
| class InstalledAuthenticators | ||
|
|
||
| AUTHN_RESOURCE_PREFIX = "conjur/authn-" | ||
| AUTHN_STATUS_FILTER = %r{conjur/(authn(?:-[^/]+)?(?:/[^/]+)?)$} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Freeze mutable objects assigned to constants. |
||
|
|
||
| class << self | ||
| def authenticators(env, authentication_module: ::Authentication) | ||
|
|
@@ -28,7 +29,7 @@ def configured_authenticators | |
| .where(identifier.like("#{AUTHN_RESOURCE_PREFIX}%")) | ||
| .where(kind => "webservice") | ||
| .select_map(identifier) | ||
| .map { |id| id[%r{^conjur/(authn(?:-[^/]+)?(?:/[^/]+)?)$}, 1] } # filter out nested status webservice | ||
| .map { |id| id[AUTHN_STATUS_FILTER, 1] } # filter out nested status webservice | ||
| .compact | ||
| .push(::Authentication::Common.default_authenticator_name) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,17 @@ | |
| let(:repo) do | ||
| DB::Repository::AuthenticatorRepository.new( | ||
| resource_repository: resource_repository, | ||
| data_object: Authentication::AuthnOidc::V2::DataObjects::Authenticator | ||
| data_object: Authentication::AuthnOidc::V2::DataObjects::Authenticator, | ||
| enabled_authenticators: enabled_authenticators | ||
| ) | ||
| end | ||
|
|
||
| let (:enabled_authenticators) { | ||
| %w[authn-oidc/foo-abc123 | ||
| authn-oidc/baz-abc123 | ||
| authn-oidc/bar-abc123] | ||
| } | ||
|
|
||
| let(:arguments) { %i[provider_uri client_id client_secret claim_mapping nonce state] } | ||
|
|
||
| describe('#find_all') do | ||
|
|
@@ -89,6 +96,21 @@ | |
| ::Role['rspec:policy:conjur/authn-oidc/baz-abc123'].destroy | ||
| end | ||
| end | ||
|
|
||
| context 'when webservices status are presents' do | ||
| before(:each) do | ||
| ::Resource.create( | ||
| resource_id: "rspec:webservice:conjur/authn-oidc/foo-abc123/status", | ||
| owner_id: "rspec:policy:conjur/authn-oidc/foo-abc123" | ||
| ) | ||
| end | ||
|
|
||
| it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec').length).to eq(2) } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| after(:each) do | ||
| ::Resource['rspec:webservice:conjur/authn-oidc/foo-abc123/status'].destroy | ||
| end | ||
| end | ||
| end | ||
|
|
||
| after(:each) do | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lists should be surrounded by blank lines