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.

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

Add authn-local support to the API #131

Merged
merged 4 commits into from
Dec 18, 2017
Merged

Add authn-local support to the API #131

merged 4 commits into from
Dec 18, 2017

Conversation

kgilpin
Copy link
Contributor

@kgilpin kgilpin commented Dec 14, 2017

The authn-local service is now available in both v4 and v5.

This pull request adds Conjur::API.authenticate_local, which uses authn-local according to the Conjur.configuration.version setting.

Build results : https://jenkins.conjur.net/job/cyberark--conjur-api-ruby/job/authn_local/

@ghost ghost assigned kgilpin Dec 14, 2017
@ghost ghost added the in progress label Dec 14, 2017
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.

One minor comment; otherwise LGTM. Let me know if you want to revise or leave as-is.

self.token_born = gettime
def gettime
monotonic_time
end
Copy link
Contributor

Choose a reason for hiding this comment

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

If the gettime method is the same in both APIKeyAuthenticator and LocalAuthenticator, why not include it in the TokenExpiration module like you did with update_token_born? Why even have a method for this - you could just replace calls to gettime with monotonic_time

@kgilpin
Copy link
Contributor Author

kgilpin commented Dec 18, 2017

@izgeri back in your court...

@izgeri
Copy link
Contributor

izgeri commented Dec 18, 2017

LGTM

@izgeri izgeri merged commit ea4d1b4 into master Dec 18, 2017
@ghost ghost removed the in progress label Dec 18, 2017
@izgeri izgeri deleted the authn_local branch December 18, 2017 19:21
micahlee pushed a commit that referenced this pull request Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants