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

Raise proper error in case user not defined with authn #1591

Merged
merged 2 commits into from
Jun 8, 2020
Merged

Raise proper error in case user not defined with authn #1591

merged 2 commits into from
Jun 8, 2020

Conversation

orenbm
Copy link
Member

@orenbm orenbm commented Jun 4, 2020 •
edited
Loading

Connected to #1593

In case a user sends an authn request with the authn authenticator,
and the user is not defined in Conjur, the log message will show:
Authentication Error: #<NoMethodError: undefined method valid_origin?' for nil:NilClass>`.

This is not informative and doesn't tell the user the real story. This commit will change
the message to:
Authentication Error: #<Errors::Authentication::Security::RoleNotFound: CONJ00007E ‘cucumber:user:non-existing-user’ not found>

@orenbm orenbm requested a review from aloncarmel111 June 4, 2020 12:19
@orenbm orenbm force-pushed the raise-authn-proper-error branch 2 times, most recently from 4a5a659 to 98d7df3 Compare June 4, 2020 12:20
aloncarmel111
aloncarmel111 previously approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@aloncarmel111 aloncarmel111 left a comment

Choose a reason for hiding this comment

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

LGTM

izgeri
izgeri suggested changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@izgeri izgeri left a comment

Choose a reason for hiding this comment

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

I think this should have a test - would have been a good time to use TDD, and add a test that was broken and verify your changes fixed it

I also left some comments on how the issue was raised

It appears to be consistent with how we are already raising errors on this page, so I'm not concerned this is doing anything different re: security / visibility of error type on a failed authentication request (cc @andytinkham)

app/domain/authentication/validate_origin.rb Outdated
return @role if @role

@role = @role_cls.by_login(@username, account: @account)
raise Errors::Authentication::Security::RoleNotFound, role_id unless @role
Copy link
Contributor

Choose a reason for hiding this comment

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

should this just be

Suggested change
raise Errors::Authentication::Security::RoleNotFound, role_id unless @role
raise Err::Security::RoleNotFound, role_id unless @role

since Err ||= Errors::Authentication above?

also, does Security::RoleNotFound need to be added to the comment at the top of the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, nice catch

@orenbm orenbm force-pushed the raise-authn-proper-error branch from 0f5b7a8 to 1e58035 Compare June 4, 2020 14:51
izgeri
izgeri previously approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@izgeri izgeri left a comment

Choose a reason for hiding this comment

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

LGTM! Nice catch @orenbm

jvanderhoof
jvanderhoof previously approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@jvanderhoof jvanderhoof left a comment

Choose a reason for hiding this comment

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

Once there is a bug report, and we've linked this PR to that issue, LGTMed!

CHANGELOG.md Outdated
@@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## Unreleased
### Fixed
- Raise proper error of an authn request with a non-existing user to the `authn`
authenticator ((cyberark/conjur#1591)[https://github.com/cyberark/conjur/pull/1591])
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a bug as well? All PRs should have corresponding cases/issue/story/bug report.

Copy link
Member Author

Choose a reason for hiding this comment

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

i created the bug and add it to the PR description. Do we want it also in the Changelog? I think that the PR link is enough.

@orenbm orenbm self-assigned this Jun 4, 2020
andytinkham
andytinkham suggested changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@andytinkham andytinkham left a comment

Choose a reason for hiding this comment

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

Before we merge this, I asked a question in Slack that needs to be resolved from a security standpoint.

@orenbm
Copy link
Member Author

orenbm commented Jun 7, 2020

Before we merge this, I asked a question in Slack that needs to be resolved from a security standpoint.

Thanks, I'll look there. Please approve the PR once it is resolved.

@orenbm orenbm mentioned this pull request Jun 7, 2020
Merged
orenbm added 2 commits June 8, 2020 10:17
@orenbm
Raise proper error in case user not defined with authn
7372776 
In case a user sends an authn request with the `authn` authenticator,
and the user is not defined in Conjur, the log message will show:
`Authentication Error: #<NoMethodError: undefined method `valid_origin?' for nil:NilClass>`.

This is not informative and doesn't tell the user the real story. This commit will change
the message to:
`Authentication Error: #<Errors::Authentication::Security::RoleNotFound: CONJ00007E ‘cucumber:user:non-existing-user’ not found>`
@orenbm
Test that a non-existing user authn request writes error to log
1c96dd8
@orenbm orenbm dismissed stale reviews from jvanderhoof and izgeri via 1c96dd8 June 8, 2020 07:17
@orenbm orenbm force-pushed the raise-authn-proper-error branch from 1e58035 to 1c96dd8 Compare June 8, 2020 07:17
@codeclimate
Copy link

codeclimate bot commented Jun 8, 2020

Code Climate has analyzed commit 1c96dd8 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 85.6% (0.0% change).

View more on Code Climate.

andytinkham
andytinkham approved these changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@andytinkham andytinkham left a comment

Choose a reason for hiding this comment

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

Answers on Slack addressed my concern. Thanks!

@orenbm orenbm merged commit 320b627 into master Jun 8, 2020
@orenbm orenbm deleted the raise-authn-proper-error branch June 8, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andytinkham andytinkham andytinkham approved these changes

@jvanderhoof jvanderhoof jvanderhoof left review comments

@izgeri izgeri izgeri left review comments

@aloncarmel111 aloncarmel111 aloncarmel111 left review comments

Assignees

@orenbm orenbm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@orenbm @jvanderhoof @andytinkham @izgeri @aloncarmel111