ONYX-13631 - `token-app-property` supports nested claims #2397

sashaCher · 2021-10-27T11:57:03Z

Desired Outcome

token-app-property supports fetching identity from nested claims
Check that value fetched using token-app-property can be a String only

Implemented Changes

ParseClaimPath became a dependency of IdentityFromDecodedTokenProvider.
IdentityFromDecodedTokenProvider changed the way it brings value from decoded token: it uses HASH.dig function instead of fetching by index.

Connected Issue/Story

Follows design #2394
ONYX-13604

Definition of Done

Desired outcome achieved
New code has appropriate UT and System tests

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
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

cucumber/authenticators_jwt/features/authn_jwt_nested_claims.feature

app/domain/authentication/authn_jwt/identity_providers/identity_from_decoded_token_provider.rb

app/domain/authentication/authn_jwt/parse_claim_path.rb

app/domain/authentication/authn_jwt/identity_providers/identity_from_decoded_token_provider.rb

CHANGELOG.md

app/domain/errors.rb

sashaCher · 2021-10-28T14:43:44Z

app/domain/errors.rb

+        code: "CONJ00117E"
+      )
+
+      TokenAppPropertyValueIsNotString = ::Util::TrackableErrorClass.new(


@hilagross @shulifink Please review

Do we have any idea why this was failed?
does the error explain the reason?

Authentication Error: #<Errors::Authentication::AuthnJwt::InvalidTokenAppPropertyValue: CONJ00117E Failed to parse 'token-app-property' value. Error: '#<Errors::Authentication::AuthnJwt::InvalidClaimPath: CONJ00116E Failed to parse claim path: 'account[0]/project/id'. The claim path is in an invalid format. The valid format should meet the following regex: '(?-mix:^[a-zA-Z|$|][a-zA-Z|$||0-9|.](/[a-zA-Z|$|][a-zA-Z|$||0-9|.])*$)'>'>

This is how it will look like, it is clear, thanks @sashaCher

app/domain/errors.rb

shulifink · 2021-10-28T16:01:01Z

app/domain/errors.rb

+      )
+
+      TokenAppPropertyValueIsNotString = ::Util::TrackableErrorClass.new(
+        msg: "{0-claim-path} value in token is an {1-type}. Only string value can be an identity.",


an? will this always start with a vowel? What are the options?

What does "Only string value can be an identity" mean? DO you mean "An identity must be a string"?

tzheleznyak · 2021-10-28T08:17:10Z

cucumber/authenticators_jwt/features/authn_jwt_nested_claims.feature

+    And I am the super-user
+    And I initialize remote JWKS endpoint with file "authn-jwt-general" and alg "RS256"
+    And I successfully set authn-jwt "jwks-uri" variable value to "http://jwks_py:8090/authn-jwt-general/RS256" in service "raw"
+    And I successfully set authn-jwt "token-app-property" variable to value "host"


Remove this from background each test should will have its own value

tzheleznyak · 2021-10-28T08:17:47Z

cucumber/authenticators_jwt/features/authn_jwt_nested_claims.feature

+    And I successfully set authn-jwt "token-app-property" variable to value "host"
+
+  @sanity
+  Scenario: ONYX-????: Token-app-property from nested claim


Move the test to the feature file of tests for identity from decoded token

codeclimate · 2021-10-30T18:04:37Z

app/domain/authentication/authn_jwt/identity_providers/identity_from_decoded_token_provider.rb

+        end
+
+        def id_claim_value_not_empty
+          return unless id_claim_value.nil? || id_claim_value.empty?


Authentication::AuthnJwt::IdentityProviders#id_claim_value_not_empty performs a nil-check

codeclimate · 2021-10-30T18:04:38Z

app/domain/authentication/authn_jwt/parse_claim_path.rb

-          claim.nil? || !claim.match?(NESTED_CLAIM_NAME_REGEX)
+      def call(claim:, parts_separator: PATH_DELIMITER)
+        raise Errors::Authentication::AuthnJwt::InvalidClaimPath.new(claim, PURE_NESTED_CLAIM_NAME_REGEX) if
+          claim.nil? || !claim.match?(PURE_NESTED_CLAIM_NAME_REGEX)


Authentication::AuthnJwt::ParseClaimPath#call performs a nil-check

app/domain/authentication/authn_jwt/identity_providers/identity_from_decoded_token_provider.rb

app/domain/authentication/authn_jwt/parse_claim_path.rb

nessiLahav

Put some comments

cucumber/authenticators_jwt/features/authn_jwt_fetch_identity_decoded_token.feature

tzheleznyak

LGTM

codeclimate · 2021-10-31T10:00:14Z

Code Climate has analyzed commit 4848eaa and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	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 90.8% (0.0% change).

View more on Code Climate.

nessiLahav

LGTM

sashaCher requested a review from a team October 27, 2021 11:57

sashaCher marked this pull request as ready for review October 28, 2021 07:40

sashaCher requested a review from a team as a code owner October 28, 2021 07:40

sashaCher force-pushed the nested-token-app-property branch from 0026a2c to 64e5962 Compare October 28, 2021 07:42

sashaCher commented Oct 28, 2021

View reviewed changes

cucumber/authenticators_jwt/features/authn_jwt_nested_claims.feature Outdated Show resolved Hide resolved

codeclimate bot reviewed Oct 28, 2021

View reviewed changes

sashaCher commented Oct 28, 2021

View reviewed changes

app/domain/errors.rb Outdated Show resolved Hide resolved

sashaCher commented Oct 28, 2021

View reviewed changes

shulifink reviewed Oct 28, 2021

View reviewed changes

tzheleznyak reviewed Oct 29, 2021

View reviewed changes

sashaCher added 12 commits October 30, 2021 20:36

ONYX-13631 - token-app-property supports nested claims

f32cacc

token-app-property input validation

8f44592

Raise error when token-app-property points to an array in token

7355967

token-app-property nested changelog entity

ed7f2bc

token-app-property nested integration sanity

a2d53b6

Add hash usecase to identity type check

663f9d6

minor updates

9d13212

add token-app-property points on array cucumber test

fe517b3

more cucumber tests

b99fb98

Modify ONYX-8825 to cover ONYX-13706

8683761

Remove array support from claim path

bdc18de

CodeClimate remarks and error handling

1e46462

sashaCher force-pushed the nested-token-app-property branch from f72be20 to 2c5e06f Compare October 30, 2021 17:37

More code climate

43546ff

sashaCher force-pushed the nested-token-app-property branch from 2c5e06f to 43546ff Compare October 30, 2021 17:39

codeclimate bot reviewed Oct 30, 2021

View reviewed changes

nessiLahav reviewed Oct 31, 2021

View reviewed changes

app/domain/authentication/authn_jwt/identity_providers/identity_from_decoded_token_provider.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Oct 31, 2021

View reviewed changes

app/domain/authentication/authn_jwt/parse_claim_path.rb Show resolved Hide resolved

nessiLahav suggested changes Oct 31, 2021

View reviewed changes

nessiLahav reviewed Oct 31, 2021

View reviewed changes

cucumber/authenticators_jwt/features/authn_jwt_fetch_identity_decoded_token.feature Show resolved Hide resolved

sashaCher added 3 commits October 31, 2021 10:33

Call call explicitly

6a84b8f

Funtion parsed_claim_path

39e7b5a

Log parsing procedure

4848eaa

tzheleznyak approved these changes Oct 31, 2021

View reviewed changes

nessiLahav approved these changes Oct 31, 2021

View reviewed changes

sashaCher merged commit ee7d340 into master Oct 31, 2021

sashaCher deleted the nested-token-app-property branch October 31, 2021 10:26

jtuttle mentioned this pull request Nov 12, 2021

Bump version for v1.14.1+suite.1 release cyberark/conjur-oss-suite-release#237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ONYX-13631 - `token-app-property` supports nested claims #2397

ONYX-13631 - `token-app-property` supports nested claims #2397

sashaCher commented Oct 27, 2021 •

edited

Loading

sashaCher Oct 28, 2021

hilagross Oct 31, 2021

hilagross Oct 31, 2021

shulifink Oct 28, 2021

tzheleznyak Oct 28, 2021

tzheleznyak Oct 28, 2021

sashaCher Oct 30, 2021

codeclimate bot Oct 30, 2021

codeclimate bot Oct 30, 2021

nessiLahav left a comment

tzheleznyak left a comment

codeclimate bot commented Oct 31, 2021

nessiLahav left a comment

ONYX-13631 - token-app-property supports nested claims #2397

ONYX-13631 - token-app-property supports nested claims #2397

Conversation

sashaCher commented Oct 27, 2021 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

sashaCher Oct 28, 2021

Choose a reason for hiding this comment

hilagross Oct 31, 2021

Choose a reason for hiding this comment

hilagross Oct 31, 2021

Choose a reason for hiding this comment

shulifink Oct 28, 2021

Choose a reason for hiding this comment

tzheleznyak Oct 28, 2021

Choose a reason for hiding this comment

tzheleznyak Oct 28, 2021

Choose a reason for hiding this comment

sashaCher Oct 30, 2021

Choose a reason for hiding this comment

codeclimate bot Oct 30, 2021

Choose a reason for hiding this comment

codeclimate bot Oct 30, 2021

Choose a reason for hiding this comment

nessiLahav left a comment

Choose a reason for hiding this comment

tzheleznyak left a comment

Choose a reason for hiding this comment

codeclimate bot commented Oct 31, 2021

nessiLahav left a comment

Choose a reason for hiding this comment

ONYX-13631 - `token-app-property` supports nested claims #2397

ONYX-13631 - `token-app-property` supports nested claims #2397

sashaCher commented Oct 27, 2021 •

edited

Loading