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 JWT authenticator and status endpoints to OpenAPI #193

Merged
merged 4 commits into from
Jun 15, 2021
Merged

Add JWT authenticator and status endpoints to OpenAPI #193

merged 4 commits into from
Jun 15, 2021

Conversation

Avi-Eyal
Copy link
Contributor

What does this PR do?

  • This PR adds the new JWT authenticator endpoints to Conjur OpenAPI

Checklists

Change log

  • Change log update will be added in the future in different PR(according to PM's decision)

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

@Avi-Eyal Avi-Eyal requested review from izgeri and telday May 30, 2021 17:10
@Avi-Eyal Avi-Eyal requested a review from a team as a code owner May 30, 2021 17:10
telday
telday suggested changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@telday telday left a comment

Choose a reason for hiding this comment

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

Looks like a good start. I think adding in some tests for the new authenticator would be nice if possible

spec/authentication.yml Outdated
schema:
$ref: 'openapi.yml#/components/schemas/AccountName'

- $ref: openapi.yml#/components/parameters/ServiceID
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- $ref: openapi.yml#/components/parameters/ServiceID
- $ref: 'openapi.yml#/components/parameters/ServiceID'

spec/authentication.yml Show resolved Hide resolved
spec/status.yml Outdated
@@ -206,6 +206,46 @@ components:
security:
- conjurAuth: []

JWTAuthenticatorStatus:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this endpoint might already be covered by the existing ServiceAuthenticatorStatus endpoint definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point, but dont we need to specify anyway to represent the separate JWT status route in conjur repository?

Copy link
Contributor

Choose a reason for hiding this comment

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

The route isn't separate though, it is already fully described by the existing ServiceAuthenticatorStatus endpoint. We have separate endpoints for each authenticator in the authentication.yml file because they cannot all be accurately described with a single one.

spec/openapi.yml Outdated
@@ -235,6 +238,9 @@ paths:
'/whoami':
$ref: 'status.yml#/components/paths/WhoAmI'

'/authn-jwt/{service_id}/{account}/status':
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'/authn-jwt/{service_id}/{account}/status':

docker-compose.yml Outdated
@@ -12,7 +12,7 @@ services:
CONJUR_DATA_KEY: 'OyXV68Mip14xj33huGaQKewmmS+gKtDlp6ECZ2iATpU='
DATABASE_URL: postgres://postgres@pg/postgres
RAILS_ENV: development
CONJUR_AUTHENTICATORS: authn-ldap/test,authn-oidc/test,authn
CONJUR_AUTHENTICATORS: authn-ldap/test,authn-oidc/test,authn-jwt/test,authn
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only necessary if you are going to stand up an instance of the JWT authenticator and run integration tests against it in a client. It would be nice to see this if possible. We have been avoiding setting up e2e tests but if we could stand up a local instance in the docker environment it would be worthwhile to write some tests for the python client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont entirely understand what you mean by "going to stand up an instance", is this something that exists for other authenticators right now?
should I add tests for this new authenticator in the OpenApi?
As far as I know, as a team we want to do the minimum regarding the OpenApi at the moment

@Avi-Eyal Avi-Eyal requested a review from telday June 3, 2021 12:47
telday
telday suggested changes Jun 3, 2021
View reviewed changes
spec/authentication.yml Outdated
@@ -106,6 +140,13 @@ components:
schema:
type: string

JWTToken:
Copy link
Contributor

Choose a reason for hiding this comment

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

This response doesn't seem to be used anywhere. Is it meant to be referenced in responses of the new endpoint.

spec/status.yml Outdated
@@ -206,6 +206,46 @@ components:
security:
- conjurAuth: []

JWTAuthenticatorStatus:
Copy link
Contributor

Choose a reason for hiding this comment

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

The route isn't separate though, it is already fully described by the existing ServiceAuthenticatorStatus endpoint. We have separate endpoints for each authenticator in the authentication.yml file because they cannot all be accurately described with a single one.

@Avi-Eyal Avi-Eyal requested a review from telday June 6, 2021 12:34
telday
telday suggested changes Jun 7, 2021
View reviewed changes
Copy link
Contributor

@telday telday left a comment

Choose a reason for hiding this comment

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

LGTM Just need a few style tweaks and adjustments to get the CI tests to pass

telday
telday suggested changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@telday telday left a comment

Choose a reason for hiding this comment

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

Just need to merge in the changes to Conjur and wait until the conjur:edge image is updated, then CI checks here should go green and it can be pulled in.

@Avi-Eyal Avi-Eyal force-pushed the add-jwt-authenticator-endpoints branch 4 times, most recently from a232bc1 to 12dd24b Compare June 15, 2021 15:55
@Avi-Eyal Avi-Eyal force-pushed the add-jwt-authenticator-endpoints branch from 12dd24b to 6a1b769 Compare June 15, 2021 17:09
telday
telday approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@telday telday left a comment

Choose a reason for hiding this comment

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

LGTM

@Avi-Eyal Avi-Eyal merged commit be1f44a into main Jun 15, 2021
@Avi-Eyal Avi-Eyal deleted the add-jwt-authenticator-endpoints branch June 16, 2021 08:32
@jtuttle jtuttle mentioned this pull request Sep 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@telday telday telday approved these changes

@izgeri izgeri Awaiting requested review from izgeri

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Avi-Eyal @telday