Add audit message to whoami command

[amosmintzcyberark]

What does this PR do?

adding audit for whoami REST command.

What ticket does this PR close?

Resolves #2052

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

[egvili]

Looks very good!
I left some comments.
2 more general comments:

1. The ticket to use for the PR should be Git ticket. You can use Rspec Unit Tests are Present for Existing Audit Events #1987
2. Please pay attention to Code Climate comments and fix wherever possible

[egvili]

Do we have to define role as class member (with @)? Maybe it can be a simple variable?

[egvili]

Please ask from @shulifink to review this text

[netacoral]

Regarding @egvili 's comment:

1. The ticket to use for the PR should be Git ticket. You can use Rspec Unit Tests are Present for Existing Audit Events #1987

There's a dedicated ticket for this: #2052

[amosmintzcyberark]

Please ask from @shulifink to review this text
she already did.

[orenbm]

i wouldn't write WhoAmI but whoami as this is the command. Also - checked its identity seems better.
In summary, i think we should write #{@role_id.role_id} checked its identity using the whoami command

[amosmintzcyberark]

This text was already reviewed and changed by Chuli, so, I'm not sure about changing it again.

[amosmintzcyberark]

Hi, Regarding the #1987 ticket, do I need to create a new one ? (because it’s close) or use it ? From: egvili <[email protected]> Reply-To: cyberark/conjur <[email protected]> Date: Sunday, 7 March 2021 at 17:27 To: cyberark/conjur <[email protected]> Cc: Amos Mintz <[email protected]>, Assign <[email protected]> Subject: Re: [cyberark/conjur] Onyx 7549 whoami audit (#2061) @egvili commented on this pull request. Looks very good! I left some comments. 2 more general comments: 1. The ticket to use for the PR should be Git ticket. You can use #1987<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/issues/1987__;!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWQJxfMl8$> 2. Please pay attention to Code Climate comments and fix wherever possible

________________________________ In app/controllers/status_controller.rb<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/pull/2061*discussion_r589044618__;Iw!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWA5qpUAA$>:

client_ip: request.ip,

user_agent: request.user_agent, account: token_user.account, username: token_user.login, token_issued_at: Time.at(token_user.token.claims["iat"]) } end + + def audit_success + @ROLE = ::Role.by_login(token_user.login, account: token_user.account) Do we have to define role as class member (with @)? Maybe it can be a simple variable?

________________________________ In app/models/audit/event/whoami.rb<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/pull/2061*discussion_r589044950__;Iw!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNW1KbjbHE$>:

@@ -0,0 +1,80 @@

+module Audit + module Event + class Whoami + + #ACCOUNT_PLACEHOLDER = "ACCOUNT_MISSING" + #USERNAME_PLACEHOLDER = "USERNAME_MISSING" + + # Note: "username" may refer to a user or host. + def initialize( + client_ip:, + role_id:, + success: + ) + @client_ip = client_ip + @role_id = role_id Given its content, maybe consider naming the variable role instead of role_id

________________________________ In app/models/audit/event/whoami.rb<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/pull/2061*discussion_r589045237__;Iw!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWyaFyxTk$>:

+ # feels like premature optimization, so we ignore reek here.

+ # :reek:UtilityFunction + def progname + Event.progname + end + + def severity + attempted_action.severity + end + + def to_s + message + end + + def message + "#{@role_id.role_id} checked identity using WhoAmI" Please ask from @shulifink<https://urldefense.com/v3/__https:/github.com/shulifink__;!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWC96HRLk$> to review this text

________________________________ In app/models/audit/event/whoami.rb<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/pull/2061*discussion_r589045495__;Iw!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWS3yqkPw$>:

+

+ def ==(other) + @comparable_evt == other + end + + # action_sd means "action structured data" + def action_sd + attempted_action.action_sd + end + + private + + def attempted_action + @attempted_action ||= AttemptedAction.new( + success: @success, + operation: 'check' Is the operation 'check' agreed/requested by @netacoral<https://urldefense.com/v3/__https:/github.com/netacoral__;!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWBV1wooM$> ? (just making sure)

________________________________ In spec/models/audit/event/whoami_spec.rb<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/pull/2061*discussion_r589045638__;Iw!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNW4AuyR_E$>:

@@ -0,0 +1,37 @@

+require 'spec_helper' + +describe Audit::Event::Whoami do + + let(:my_role_id) { 'rspec:user:my_user' } + let(:role) { double('Role', role_id: my_role_id) } Not critical but maybe inline my_role_id as it's used only once — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/cyberark/conjur/pull/2061*pullrequestreview-605856157__;Iw!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNW8HXEQPU$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/ASTGYNR64ZIUKICTTCKXYW3TCOLOLANCNFSM4YX5OJMQ__;!!Pe07N362zA!hpzcS_J6MPhowR0Zf_6qnXNF1Rlkl4Xd9AKjhls4W4PXWc3TmWszXcsHotNWd9rLYx8$>.

[orenbm]

left some comments

[codeclimate]

Add empty line after guard clause.

[orenbm]

this

[codeclimate]

Audit::Event::Whoami#role_id performs a nil-check

[orenbm]

left some comments

[orenbm]

@shulifink please review

[egvili]

Shouldn't this change be placed in Appliance repo CHANGELOG.md? @netacoral

[shulifink]

Suggested change

	- `/whoami` API endpoint now produce audit events.
	- `/whoami` API endpoint now produces audit events.

[codeclimate]

Code Climate has analyzed commit f71c65d and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 94.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.3% (0.0% change).

View more on Code Climate.