Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Integrate signing key settings and settings fetcher to main flow #2456

Merged
merged 1 commit into from
Jan 12, 2022
Merged

Integrate signing key settings and settings fetcher to main flow #2456

merged 1 commit into from
Jan 12, 2022

Conversation

sashaCher
Copy link
Contributor

@sashaCher sashaCher commented Jan 4, 2022 •
edited
Loading

Desired Outcome

The PR is a part of creating abstraction layer for variables in the area of signing keys fetching and settings.

Implemented Changes

This PR integrates classes are created in the PR #2455 into the main flows of authentication and status of JWT authenticator.

Connected Issue/Story

ONYX-15595

Definition of Done

  • The desired outcome is achieved
  • There are new UTs are covered the change
  • All existing integration tests are passed

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

@sashaCher sashaCher requested a review from a team January 4, 2022 19:42
@sashaCher sashaCher force-pushed the authn-jwt-integrate-signing-key-settings branch 3 times, most recently from c20a80e to cb8da19 Compare January 6, 2022 13:34
@sashaCher sashaCher force-pushed the authn-jwt-integrate-signing-key-settings branch 2 times, most recently from 06047d7 to 5f12b3d Compare January 11, 2022 21:17
@sashaCher sashaCher force-pushed the authn-jwt-integrate-signing-key-settings branch from 5f12b3d to 5200f67 Compare January 12, 2022 11:30
@sashaCher sashaCher force-pushed the authn-jwt-integrate-signing-key-settings branch from 5200f67 to 27b9a59 Compare January 12, 2022 12:57
Base automatically changed from authn-jwt-s1 to master January 12, 2022 13:40
@sashaCher sashaCher force-pushed the authn-jwt-integrate-signing-key-settings branch from 27b9a59 to 1435996 Compare January 12, 2022 13:42
@sashaCher sashaCher marked this pull request as ready for review January 12, 2022 13:42
@sashaCher sashaCher requested a review from a team as a code owner January 12, 2022 13:42
codeclimate[bot]
codeclimate bot reviewed Jan 12, 2022
View reviewed changes
app/domain/authentication/authn_jwt/signing_key/create_signing_key_provider.rb
if provider_uri_resource_exists? and !jwks_uri_has_resource_exists?
fetch_provider_uri_signing_key
elsif jwks_uri_has_resource_exists? and !provider_uri_resource_exists?
case signing_key_settings.type
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authentication::AuthnJwt::SigningKey#create_signing_key_provider calls 'signing_key_settings.type' 2 times

app/domain/authentication/authn_jwt/signing_key/create_signing_key_provider.rb
create_signing_key_provider
end

private

def fetch_signing_key_settings
@signing_key_settings ||= @fetch_signing_key_settings.call(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memoized variable @signing_key_settings does not match method name fetch_signing_key_settings. Use @fetch_signing_key_settings instead.

app/domain/authentication/authn_jwt/signing_key/create_signing_key_provider.rb
raise Errors::Authentication::AuthnJwt::InvalidUriConfiguration.new(
PROVIDER_URI_RESOURCE_NAME,
JWKS_URI_RESOURCE_NAME
raise Errors::Authentication::AuthnJwt::InvalidSigningKeyType.new(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide an exception class and message as arguments to raise.

tzheleznyak
tzheleznyak previously approved these changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@tzheleznyak tzheleznyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sashaCher sashaCher force-pushed the authn-jwt-integrate-signing-key-settings branch from 1435996 to 6075461 Compare January 12, 2022 19:47
tzheleznyak
tzheleznyak approved these changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@tzheleznyak tzheleznyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codeclimate
Copy link

codeclimate bot commented Jan 12, 2022

Code Climate has analyzed commit 6075461 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Style 2

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 91.1% (0.0% change).

View more on Code Climate.

@sashaCher sashaCher merged commit 3817dca into master Jan 12, 2022
@sashaCher sashaCher deleted the authn-jwt-integrate-signing-key-settings branch January 12, 2022 20:49
@jtuttle jtuttle mentioned this pull request Jul 12, 2022
Merged
@gl-johnson gl-johnson mentioned this pull request Aug 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@tzheleznyak tzheleznyak tzheleznyak approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sashaCher @tzheleznyak