Tag oidc unit tests #2706

codihuston · 2023-01-23T22:33:30Z

To run the tests, start conjur dev environment:

cd git/conjur/dev
./start

Exec into the conjur container and run the tests:

docker-compose exec conjur bash

rspec -f d --color --dry-run --tag @type:unit

Desired Outcome

First round of unittags to rspec tests for oidc pkce and v2.

See the above commands for visualizing which tests have been tagged.

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?
- Tests for the authn-oidc strategies were previously not being run due to a typo in the file name _rspec.rb vs _spec.rb
- authn-oidc tests have been tagged as described above
How should the reviewer approach this PR, especially if manual tests are required?
Are there relevant screenshots you can add to the PR description?

Connected Issue/Story

CyberArk internal issue ID: [CNJR-178]

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

codihuston

@adamouamani What do you think? Do tests that leverage a DataObject in conjunction with another module always constitute as an integration test? https://github.com/cyberark/conjur/pull/2706/files#diff-cf99fb4eccb8c5d8aaeb05adb7d5063d743d31d8bd623cc38f878b318620bd60R41

codihuston

@adamouamani Here's another example of a test that uses a mock and a data object. Should it be considered a unit test or an integration test? https://github.com/cyberark/conjur/pull/2706/files#diff-ecc086dac9693ffbe9eba71f440388e8e466250eae64880dadaef7f13f37ddf1R110

codihuston · 2023-02-01T17:46:12Z

Hi @jvanderhoof and @adamouamani,

Currently, we have questions on the following tests (currently tagged with :review => true in this PR). I'll outline some of the ones I have questions about below, and a question(s) for each.

spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb

These tests leverage a Authentication::AuthnOidc::PkceSupportFeature::Strategy
instance that is instantiated with a Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator
and doubles of the Authentication::AuthnOidc::PkceSupportFeature::Client.

Any tests that use doubles/mocks/stubs/etc. has been determined to be a
unit test, however this test uses a combination of doubles and an unmocked
DataObject that appears to be the system under test
(the state being tested in the DataObject).

Since the thing being tested here is the return value of the underlying
data object (through a call to the strategy), do we think this would this qualify it as
an integration test?

codihuston · 2023-02-01T17:46:38Z

Here's another one @jvanderhoof and @adamouamani,

spec/app/domain/authentication/authn-oidc/pkce_support_feature/views/providor_context_spec.rb

Here, we are testing a Authentication::AuthnOidc::PkceSupportFeature::Views::ProviderContext,
which is instantiated with doubled clients, but what is being tested
is the output of the provider_context.call method with unmocked
Authentication::AuthnOidc::PkceSupportFeature::DataObjects.

Since this appears to be testing this method's public interface with an
unmocked input, I'm thinking this would be more integration over it
being a unit test. WDYT?

In the same file, the test for when authenticator discovery endpoint is unreachable appears to be more unit than
integration because of the client's discovery_information being mocked
out, but the call is still using the same unmocked authenticators.

I'm trying to identify if what we should qualify integration tests based
on whether or not the public interfaces are being tested, or the output
itself, or both?

codihuston · 2023-02-01T17:47:04Z

And another, @jvanderhoof, @adamouamani

spec/app/domain/authentication/authn-oidc/pkce_support_feature/client_spec.rb

I didn't mark this one with the review tag, but I see the use of both
an authenticator and a client here. All of these tests look very unit,
since they are testing individual methods, but the fact that the test is
driven with a client that was instantiated with an unmocked authenticator
might be enough to warrant them being integration tests. WDYT?

adamouamani · 2023-02-01T18:19:08Z

@codihuston - you have highlighted a lot of great cases that we can review with @jvanderhoof and document and create a pattern we can apply for other tests!

jvanderhoof · 2023-02-01T20:42:16Z

@codihuston, (cc. @adamouamani), my personal opinion is that the first two examples:

are both appropriate unit tests because they take advantage of dependency injection as alternative to excessive mocking.

The Authentication::AuthnOidc::V2::DataObject is a stateless class that holds common information (specifically, all the information that is stored in Conjur related to an OIDC Authenticator). It's intended as a glorified Struct (this allows us to keep the details of a particular authenticator outside the scope of the particular authentication actions).

We don't need to mock the data object, because it's essentially a mock already (but it's the real object). For this reason, I'd consider the first two examples appropriate to call unit tests on.

jvanderhoof · 2023-02-01T20:50:14Z

@codihuston, (cc. @adamouamani). I do want to state that mocking must be understood as a code smell, albeit one which is necessary in any software which takes in external data (essentially all of them). Mocking is highly problematic as it requires two definitions of a single interface: one for the class being mocked, the second, the mock of the class. Mocks are necessary when we deal with outside data (loading from database, API requests, user input, etc.). Dependency inject simplifies the process of mocking (which is why we push this pattern), but any mocking is a poor substitute for the actual dependent object.

jvanderhoof · 2023-02-01T20:54:50Z

@codihuston, (cc. @adamouamani), I'd also classify the third item (spec/app/domain/authentication/authn-oidc/pkce_support_feature/client_spec.rb) as being appropriate as a unit test give the same argument as above.

I do think it's very important to understand the risk in the mocked OIDC client in this test. We're using VCR to mock the OIDC client wrapped in this class. The danger in our mock is that if the upstream OIDC API changes, our tests will continue to pass. This is why it's important to have a few E2E tests over all this. Unit tests can only identify that when given a particular set of pre-defined inputs, the method returns an expected output.

jvanderhoof · 2023-02-01T20:55:47Z

@codihuston, @adamouamani, I really should have started with the fact that I really like this approach! It's simple, natively part of RSpec, and gives more context to a particular test. Well done!!!!

adamouamani · 2023-02-01T22:13:08Z

@codihuston @jvanderhoof This is a fantastic start! I'll update the confluence page with approach, feedback from Jason and examples to help people determine test type when evaluating additional modules in the future.

https://ca-il-confluence.il.cyber-ark.com/display/rndp/Test+Level+Review+Patterns

codihuston · 2023-02-01T22:24:02Z

@adamouamani Awesome!

We've only come out of this effort with identifying unit tests--should we consider merging this PR as is, or should we shelf it for the time being until we tag and review some true integration tests? If so, let me know and I'll mark this ready for review / undraft it.

Note: no changes have been added in the pipeline to leverage this tag (I believe we weren't planning on doing this anytime soon).

adamouamani · 2023-02-02T16:17:34Z

@codihuston I would merge this and finish any remaining updates to docs on confluence if anything that I missed. We can then create a follow-up story to continue this effort for another section/module with more integration tests examples

codihuston · 2023-02-02T19:01:47Z

Hey @jvanderhoof , this PR contains a commit that adds tests that were being ignored by rspec due to some files not following the *_spec.rb naming scheme (see commit details).

It looks like one of the oidc pkce strategy tests were failing when expecting a specific error code to be thrown, but it turns out a different error code was being thrown instead. I updated the test to account for that, but can you confirm that this is the intended behavior please? Thanks!

Failures:

  1) Authentication::AuthnOidc::PkceSupportFeature::Strategy #callback when required parameters are missing when code_verifier is missing raises a `MissingRequestParam` error
     Failure/Error:
       expect { strategy.callback(nonce: 'nonce', code: 'code') }
         .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam)
     
       expected Errors::Authentication::RequestBody::MissingRequestParam, got #<Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty: CONJ00013E Claim '' not found or empty in ID token. This claim is defined in the id-token-user-property variable.> with backtrace:
         # ./app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb:30:in `callback'
         # ./spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb:73:in `block (6 levels) in <top (required)>'
         # ./spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb:73:in `block (5 levels) in <top (required)>'
         # ./spec/spec_helper.rb:65:in `block (3 levels) in <top (required)>'
         # ./spec/spec_helper.rb:65:in `block (2 levels) in <top (required)>'
         # ./spec/spec_helper.rb:57:in `block (3 levels) in <top (required)>'
         # ./spec/spec_helper.rb:56:in `block (2 levels) in <top (required)>'
     # ./spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb:73:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:65:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:65:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:57:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:56:in `block (2 levels) in <top (required)>'

jvanderhoof · 2023-02-03T15:21:28Z

Hey @jvanderhoof , this PR contains a commit that adds tests that were being ignored by rspec due to some files not following the *_spec.rb naming scheme (see commit details).

It looks like one of the oidc pkce strategy tests were failing when expecting a specific error code to be thrown, but it turns out a different error code was being thrown instead. I updated the test to account for that, but can you confirm that this is the intended behavior please? Thanks!

Failures:

  1) Authentication::AuthnOidc::PkceSupportFeature::Strategy #callback when required parameters are missing when code_verifier is missing raises a `MissingRequestParam` error
     Failure/Error:
       expect { strategy.callback(nonce: 'nonce', code: 'code') }
         .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam)
     
       expected Errors::Authentication::RequestBody::MissingRequestParam, got #<Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty: CONJ00013E Claim '' not found or empty in ID token. This claim is defined in the id-token-user-property variable.> with backtrace:
         # ./app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb:30:in `callback'
         # ./spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb:73:in `block (6 levels) in <top (required)>'
         # ./spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb:73:in `block (5 levels) in <top (required)>'
         # ./spec/spec_helper.rb:65:in `block (3 levels) in <top (required)>'
         # ./spec/spec_helper.rb:65:in `block (2 levels) in <top (required)>'
         # ./spec/spec_helper.rb:57:in `block (3 levels) in <top (required)>'
         # ./spec/spec_helper.rb:56:in `block (2 levels) in <top (required)>'
     # ./spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_spec.rb:73:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:65:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:65:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:57:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:56:in `block (2 levels) in <top (required)>'

Good catch. I just found and fixed this issue here: #2713.

@type

You can display the tagged the tests with the following command(s). Drop the `--dry-run` to actually run the tests: ```bash rspec -f d --color --dry-run --tag @type:unit ```

codeclimate · 2023-02-06T18:26:54Z

Code Climate has analyzed commit f621ed6 and detected 0 issues on this pull request.

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.0% (-1.5% change).

View more on Code Climate.

codihuston · 2023-02-06T18:48:59Z

Thanks Jason! I dropped those commits from this PR--it only contains the labeling effort then. Appreciate it!

adamouamani

LGTM! Thanks @codihuston

codihuston changed the title ~~Label oidc-pkce unit tests~~ Label oidc pkce unit tests Jan 23, 2023

codihuston requested a review from adamouamani January 23, 2023 22:33

codihuston commented Jan 23, 2023

View reviewed changes

codihuston force-pushed the test-tagging branch from 8bd5d9b to f607512 Compare January 26, 2023 23:00

codihuston force-pushed the test-tagging branch from f607512 to 2326937 Compare February 1, 2023 22:15

codihuston force-pushed the test-tagging branch from 2326937 to 5db1fbd Compare February 2, 2023 18:57

codihuston changed the title ~~Label oidc pkce unit tests~~ Add missing oidc strategy rspec tests and label oidc pkce unit tests Feb 2, 2023

codihuston changed the title ~~Add missing oidc strategy rspec tests and label oidc pkce unit tests~~ Add missing oidc strategy rspec tests and label oidc unit tests Feb 2, 2023

Add unit test tags to oidc tests

f621ed6

You can display the tagged the tests with the following command(s). Drop the `--dry-run` to actually run the tests: ```bash rspec -f d --color --dry-run --tag @type:unit ```

codihuston force-pushed the test-tagging branch from 5db1fbd to f621ed6 Compare February 6, 2023 17:47

codihuston marked this pull request as ready for review February 6, 2023 17:47

codihuston requested a review from a team as a code owner February 6, 2023 17:47

codihuston changed the title ~~Add missing oidc strategy rspec tests and label oidc unit tests~~ Tag oidc unit tests Feb 6, 2023

adamouamani approved these changes Feb 6, 2023

View reviewed changes

codihuston merged commit 38d7ecc into master Feb 6, 2023

codihuston deleted the test-tagging branch February 6, 2023 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag oidc unit tests #2706

Tag oidc unit tests #2706

codihuston commented Jan 23, 2023 •

edited

Loading

codihuston left a comment •

edited

Loading

codihuston left a comment •

edited

Loading

codihuston commented Feb 1, 2023

codihuston commented Feb 1, 2023

codihuston commented Feb 1, 2023

adamouamani commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

adamouamani commented Feb 1, 2023

codihuston commented Feb 1, 2023

adamouamani commented Feb 2, 2023 •

edited

Loading

codihuston commented Feb 2, 2023 •

edited

Loading

jvanderhoof commented Feb 3, 2023

codeclimate bot commented Feb 6, 2023

codihuston commented Feb 6, 2023

adamouamani left a comment

Tag oidc unit tests #2706

Tag oidc unit tests #2706

Conversation

codihuston commented Jan 23, 2023 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codihuston left a comment • edited Loading

Choose a reason for hiding this comment

codihuston left a comment • edited Loading

Choose a reason for hiding this comment

codihuston commented Feb 1, 2023

codihuston commented Feb 1, 2023

codihuston commented Feb 1, 2023

adamouamani commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

jvanderhoof commented Feb 1, 2023

adamouamani commented Feb 1, 2023

codihuston commented Feb 1, 2023

adamouamani commented Feb 2, 2023 • edited Loading

codihuston commented Feb 2, 2023 • edited Loading

jvanderhoof commented Feb 3, 2023

codeclimate bot commented Feb 6, 2023

codihuston commented Feb 6, 2023

adamouamani left a comment

Choose a reason for hiding this comment

codihuston commented Jan 23, 2023 •

edited

Loading

codihuston left a comment •

edited

Loading

codihuston left a comment •

edited

Loading

adamouamani commented Feb 2, 2023 •

edited

Loading

codihuston commented Feb 2, 2023 •

edited

Loading