Authenticator classes reflection rules and `authn-jwt` #2365

sashaCher · 2021-08-19T13:12:45Z

What does this PR do?

There's an issue that new authn-jwt authenticator does not appear in installed authenticators list.

The PR contains a document analyzes root causes of the issue and proposes number solutions for it.

The PR also contains chosen solution implementation.

What ticket does this PR close?

ONYX-11396

Checklists

Change log

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, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

API Changes

The OpenAPI spec has been updated to meet new API changes (or an issue has been opened), or
The changes in this PR do not affect the Conjur API

cucumber/authenticators_config/features/step_definitions/available_authn_steps.rb

codeclimate · 2021-08-19T13:14:47Z

design/authenticators/authn_class_rules.md

+
+#### Functional Tests
+
+[//]: # "Fill in the table below to depict the tests that should run to validate your solution"


Line length

design/authenticators/authn_class_rules.md

codeclimate · 2021-08-19T13:14:48Z

design/authenticators/authn_class_rules.md

+[//]: # "2. Design documents should not be updated after implementation"
+[//]: # "3. Design decisions should be made before writing this document, and as such this document should not include options / choices"
+
+


Multiple consecutive blank lines

nessiLahav

LGTM

eladkug · 2021-08-24T11:51:31Z

cucumber/authenticators_config/features/available_authn.feature

@@ -1,7 +1,19 @@
 Feature: The list of available authentication providers is discoverable
  through the API.

-  Background:
+  Scenario: Verify installed authenticators
+    When I retrieve the list of authenticators


Given a running master
If we have the test in XRAY we need to add it here as well

@eladkug You've mismatched repos. There's no master in OSS so there's no any given.

Will add a test to XRAY.

@sashaCher How are we using XRAY? Are we duplicating the cucumber tests somewhere?

Are we duplicating the cucumber tests somewhere

Briefly. See ONYX-12104 for example in Jira and in the PR

@jonahx for now we using XRAY for manual testing only and the cucumber represent the test.
In the future we plan to update the feature from our repo

jonahx · 2021-08-25T06:24:49Z

design/authenticators/authn_class_rules.md

+### 3. Rethink this area from scratch and to create a new contract meat all known needs
+
+From my perspective this approach does not worth ROI at the moment and
+should be taken as a part of core-authenticators separation if occur.


I agree though I do think this would be a good thing to do. I wrote the original interface with a lot less context than we have now and would probably do things differently today.

jonahx · 2021-08-25T06:27:59Z

This proposal looks reasonable to me.

In fact, if I were doing this over, I would probably remove all the reflection stuff from the code. I haven't thought much about what I'd do instead -- there are a few options -- but I think using reflection to inspect the classes at runtime is just too fancy for my taste now. That said, I understand if you want to keep the scope small, which it is now.

nessiLahav

LGTM

codeclimate · 2021-08-25T15:56:43Z

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ### Fixed
 - Fix bug of cache not working in authn jwt. [cyberark/conjur#2353](https://github.com/cyberark/conjur/pull/2353)
+- Fix bug `authn-jwt` now appears in `installed` authenticators list of `authenticators` endpoint output. [cyberark/conjur#2365](https://github.com/cyberark/conjur/pull/2365)


Line length

codeclimate · 2021-08-25T15:56:43Z

design/authenticators/authn_class_rules.md

+The general authentication flow does not suite needs of all existing authenticators
+from both flow and authentication parameters view.
+Today it's often solved by adding degenerate `valid?` method to an `Authenticator` class.
+It makes code less readable and can cause [bugs](https://github.com/cyberark/conjur/pull/2348/commits/7db01a6ab8a19f33c157c519ff903b11a392a8aa).  


Trailing spaces

codeclimate · 2021-08-25T15:56:43Z

design/authenticators/authn_class_rules.md

+  with the same class definition `MyClass` for example ruby will "choose" the class from `myclass.rb` file.
+  In our use-case all `Authenticator` classes are in `authenticator.rb` files and it's
+  highly unlikely that we somehow will break this convention. In any case it will behave
+  the same with or without `valid?` method rule. 


Trailing spaces

codeclimate · 2021-08-25T15:56:43Z

design/authenticators/authn_class_rules.md

+- Authenticator module must have a class named `Authenticator`
+- `Authenticator` class must have `valid?` method
+
+All rules together define authenticator appearance in `installed` authenticators of the `authenticators` rest call.


Line length

codeclimate · 2021-08-25T15:56:44Z

design/authenticators/authn_class_rules.md

+
+The general authentication flow does not suite needs of all existing authenticators
+from both flow and authentication parameters view.
+Today it's often solved by adding degenerate `valid?` method to an `Authenticator` class.


Line length

codeclimate · 2021-08-25T19:36:30Z

design/authenticators/authn_class_rules.md

+The general authentication flow does not suite needs of all existing authenticators
+from both flow and authentication parameters view.
+Today it's often solved by adding degenerate `valid?` method to an `Authenticator` class.
+It makes code less readable and can cause [bugs](https://github.com/cyberark/conjur/pull/2348/commits/7db01a6ab8a19f33c157c519ff903b11a392a8aa).  


Line length

codeclimate · 2021-08-25T19:36:30Z

design/authenticators/authn_class_rules.md

+## Issue Description
+
+A recently introduced `authn-jwt` authenticator does not appear in `installed` authenticators
+of the `authenticators` rest call. It has its own authentication flow hence it has no `valid?` method.


Line length

codeclimate · 2021-08-25T19:36:30Z

design/authenticators/authn_class_rules.md

+- makes contract weaker?
+
+  Actually the reflection rules are looking for the first level class in the module.
+  Ruby prioritizes classes with respective file name, means if there are multiple files


Line length

codeclimate · 2021-08-25T19:36:31Z

design/authenticators/authn_class_rules.md

+
+  Actually the reflection rules are looking for the first level class in the module.
+  Ruby prioritizes classes with respective file name, means if there are multiple files
+  with the same class definition `MyClass` for example ruby will "choose" the class from `myclass.rb` file.


Line length

codeclimate · 2021-08-25T19:36:31Z

design/authenticators/authn_class_rules.md

+  Actually the reflection rules are looking for the first level class in the module.
+  Ruby prioritizes classes with respective file name, means if there are multiple files
+  with the same class definition `MyClass` for example ruby will "choose" the class from `myclass.rb` file.
+  In our use-case all `Authenticator` classes are in `authenticator.rb` files and it's


Line length

codeclimate · 2021-08-25T19:36:32Z

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

Here's the issue category breakdown:

Category	Count
Style	12

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

View more on Code Climate.

nessiLahav

LGTM

sashaCher requested a review from a team August 19, 2021 13:12

codeclimate bot reviewed Aug 19, 2021

View reviewed changes

nessiLahav previously approved these changes Aug 22, 2021

View reviewed changes

sashaCher dismissed nessiLahav’s stale review via 4fe0ac7 August 24, 2021 09:30

sashaCher force-pushed the authn-jwt-verify branch 2 times, most recently from 565b660 to 5cd4e3b Compare August 24, 2021 09:35

eladkug reviewed Aug 24, 2021

View reviewed changes

sashaCher force-pushed the authn-jwt-verify branch from 5cd4e3b to 83ea064 Compare August 24, 2021 13:18

jonahx reviewed Aug 25, 2021

View reviewed changes

sashaCher marked this pull request as ready for review August 25, 2021 07:08

sashaCher requested a review from a team as a code owner August 25, 2021 07:08

sashaCher force-pushed the authn-jwt-verify branch 2 times, most recently from fc8e9ee to 126fd28 Compare August 25, 2021 10:34

nessiLahav previously approved these changes Aug 25, 2021

View reviewed changes

sashaCher dismissed nessiLahav’s stale review via efe274b August 25, 2021 14:13

sashaCher force-pushed the authn-jwt-verify branch 3 times, most recently from f1497c1 to 51e01f2 Compare August 25, 2021 15:00

codeclimate bot reviewed Aug 25, 2021

View reviewed changes

sashaCher force-pushed the authn-jwt-verify branch from 51e01f2 to a90ff6a Compare August 25, 2021 15:58

sashaCher added 4 commits August 25, 2021 22:11

Authentication class rules change proposal

664a22d

Remove valid? method rule from Authenticator class rules

cc38f92

Test installed authenticators

befd2ed

Add changelog message

6379224

sashaCher force-pushed the authn-jwt-verify branch from a90ff6a to 6379224 Compare August 25, 2021 19:11

codeclimate bot reviewed Aug 25, 2021

View reviewed changes

nessiLahav approved these changes Aug 26, 2021

View reviewed changes

sashaCher merged commit a4efc51 into master Aug 26, 2021

sashaCher deleted the authn-jwt-verify branch August 26, 2021 06:55

jtuttle mentioned this pull request Sep 14, 2021

Update suite.yml for v1.13.1+suite.1 release cyberark/conjur-oss-suite-release#233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authenticator classes reflection rules and `authn-jwt` #2365

Authenticator classes reflection rules and `authn-jwt` #2365

sashaCher commented Aug 19, 2021 •

edited

Loading

codeclimate bot Aug 19, 2021

codeclimate bot Aug 19, 2021

nessiLahav left a comment

eladkug Aug 24, 2021

sashaCher Aug 24, 2021

sashaCher Aug 24, 2021

jonahx Aug 25, 2021

sashaCher Aug 25, 2021

eladkug Sep 2, 2021

jonahx Aug 25, 2021 •

edited

Loading

jonahx commented Aug 25, 2021 •

edited

Loading

nessiLahav left a comment

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot Aug 25, 2021

codeclimate bot commented Aug 25, 2021

nessiLahav left a comment


		#### Functional Tests

		[//]: # "Fill in the table below to depict the tests that should run to validate your solution"

		[//]: # "2. Design documents should not be updated after implementation"
		[//]: # "3. Design decisions should be made before writing this document, and as such this document should not include options / choices"

Authenticator classes reflection rules and authn-jwt #2365

Authenticator classes reflection rules and authn-jwt #2365

Conversation

sashaCher commented Aug 19, 2021 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

API Changes

codeclimate bot Aug 19, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 19, 2021

Choose a reason for hiding this comment

nessiLahav left a comment

Choose a reason for hiding this comment

eladkug Aug 24, 2021

Choose a reason for hiding this comment

sashaCher Aug 24, 2021

Choose a reason for hiding this comment

sashaCher Aug 24, 2021

Choose a reason for hiding this comment

jonahx Aug 25, 2021

Choose a reason for hiding this comment

sashaCher Aug 25, 2021

Choose a reason for hiding this comment

eladkug Sep 2, 2021

Choose a reason for hiding this comment

jonahx Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

jonahx commented Aug 25, 2021 • edited Loading

nessiLahav left a comment

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 25, 2021

Choose a reason for hiding this comment

codeclimate bot commented Aug 25, 2021

nessiLahav left a comment

Choose a reason for hiding this comment

Authenticator classes reflection rules and `authn-jwt` #2365

Authenticator classes reflection rules and `authn-jwt` #2365

sashaCher commented Aug 19, 2021 •

edited

Loading

jonahx Aug 25, 2021 •

edited

Loading

jonahx commented Aug 25, 2021 •

edited

Loading