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

Add generic authenticator endpoints to spec #115

Merged
merged 4 commits into from
Jan 19, 2021
Merged

Add generic authenticator endpoints to spec #115

merged 4 commits into from
Jan 19, 2021

Conversation

telday
Copy link
Contributor

@telday telday commented Jan 7, 2021 •
edited
Loading

What does this PR do?

Added the most generic forms of the authenticator endpoints. The OIDC and GCP authenticators have slightly differently formatted authenticate endpoints so they are not fully included in this PR. The tests are currently minimal and include many expected failures, this is because we do not have any external authenticators setup. There is an open issue to both add an external authenticator and to update tests to fully use the authenticators.

What ticket does this PR close?

Resolves: #66, #74, #70, #75, #84
Partially addresses: #60, #61

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

@telday telday requested a review from a team as a code owner January 7, 2021 20:17
@telday telday force-pushed the iam-authenticator branch 4 times, most recently from 1d98cb8 to 423f9d4 Compare January 8, 2021 15:46
This was referenced Jan 8, 2021
Closed
Closed
@telday telday force-pushed the iam-authenticator branch 2 times, most recently from 17a2504 to f9da827 Compare January 11, 2021 16:02
john-odonnell
john-odonnell suggested changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

One substantial comment, with a side of small ones:
We might want to consider setting up an external authenticator either in this PR, or before it's merged. There are a few cases where tests expect failure on tests for successful status codes, but without these successful cases we really have no way of confirming the endpoint works.

CHANGELOG.md Outdated
Comment on lines 29 to 34
- Generic authenticator endpoints for authentication/other external authenticator requests.
[cyberark/conjur-openapi-spec#66](https://github.com/cyberark/conjur-openapi-spec/issues/66)
[cyberark/conjur-openapi-spec#74](https://github.com/cyberark/conjur-openapi-spec/issues/74)
[cyberark/conjur-openapi-spec#70](https://github.com/cyberark/conjur-openapi-spec/issues/70)
[cyberark/conjur-openapi-spec#75](https://github.com/cyberark/conjur-openapi-spec/issues/75)
Copy link
Contributor

Choose a reason for hiding this comment

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

There might be a better way to format these multi-issue changes. This MD renders as a one-liner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks slightly better now. Moved one of the changelog entries to another entry. I think unfortunately they all belong with that entry.

spec/openapi.yml Outdated Show resolved Hide resolved
spec/authentication.yaml Show resolved Hide resolved
spec/openapi.yml Outdated Show resolved Hide resolved
spec/authentication.yaml Outdated Show resolved Hide resolved
test/python/test_authn_api.py Outdated Show resolved Hide resolved
izgeri
izgeri reviewed Jan 12, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@telday telday force-pushed the iam-authenticator branch 4 times, most recently from 19433aa to a22aa02 Compare January 14, 2021 20:56
@telday telday force-pushed the iam-authenticator branch 2 times, most recently from e44dc7c to 1c0a738 Compare January 15, 2021 17:54
@john-odonnell
Copy link
Contributor

Might this expected failure be able to pass with the new ldap authn? I can't attach this to a review bc it's not a line you edited but I figured it might be in scope.

john-odonnell
john-odonnell reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

Nothing big, but this is ✨ delightful✨

spec/authentication.yaml Outdated Show resolved Hide resolved
spec/authentication.yaml Outdated Show resolved Hide resolved
spec/authentication.yaml Outdated Show resolved Hide resolved
test/python/test_authn_api.py Show resolved Hide resolved
test/python/test_authn_api.py Show resolved Hide resolved
test/python/api_config.py Outdated Show resolved Hide resolved
@telday
Added authenticator endpoints to spec
ba6ad57 
Allows authentication with external authenticators other than the
built in authn module.
@telday
Added LDAP authenticator to test environment
1da5c39 
Allows for full testing of all authentication endpoints
@telday
Added integration tests for authenticators endpoints
1a4c2c3
john-odonnell
john-odonnell previously approved these changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

LGTM!

@telday telday force-pushed the iam-authenticator branch 2 times, most recently from 5955416 to 42a60e2 Compare January 19, 2021 17:16
@telday
Update CHANGELOG and Jenkinsfile
b2cf344 
Removed cobertera failing on unstable flag until
python tests are integrated into project fully
john-odonnell
john-odonnell approved these changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

👍

@john-odonnell john-odonnell merged commit 5404481 into main Jan 19, 2021
@john-odonnell john-odonnell deleted the iam-authenticator branch January 19, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@izgeri izgeri izgeri left review comments

@john-odonnell john-odonnell john-odonnell approved these changes

@BradleyBoutcher BradleyBoutcher Awaiting requested review from BradleyBoutcher

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the authenticator account endpoint to the spec file
3 participants
@telday @john-odonnell @izgeri