search

chef-boneyard / opscode-pushy-server

Chef Push Jobs Server
https://docs.chef.io/push_jobs.html
Apache License 2.0
16 stars 10 forks source link

Support multi-key auth #111

Closed stevendanna closed 9 years ago

stevendanna commented 9 years ago

@chef/lob @manderson26 I still have to test against pushy-pedant (and maybe add a test there) but I wanted to get this out for review.

This is the "just make it work with the current code" version of this fix. I think we could probably extract some of this out into a common library for use in other projects (pushy_http_common is probably a good starting point for a generic erchef_client and most of the code here: https://github.com/chef/opscode-pushy-server/pull/111/files#diff-6c2407c0c1779021963948964504cee0R177 could probably be moved into a chef_authn function that provides a nice API).

Overall I think we do eventually need a better endpoint than principals (principals is fine, it just doesn't really do what we need), but I'd prefer to put some thought into that rather than push it through quickly.

stevendanna commented 9 years ago

Plan of record:

stevendanna commented 9 years ago

I've been looking at reporting and while it uses the principals endpoint for the same purpose, it's HTTP client implementation is a bit different as it includes a caching layer.

sdelano commented 9 years ago

After all that review, there's nothing major here. This looks good. Nice!

markan commented 9 years ago

:+1: Looks good, there's some tech debt here I need to clean up later, but that shouldn't be a blocker.

stevendanna commented 9 years ago

@sdelano @manderson26 Alrighty, I addressed the code review comments. I also backed out some of the changes I made where I converted functions to return error tuples rather than throw them. The changes were unnecessary cruft from an initial refactoring idea and they were not playing nicely with dialyzer.

I turned off dialyzer's -Wunderspec. After spending too much time trying to get it to work, I got it down to the following warnings:

pushy_principal.erl:43: Type specification pushy_principal:request_principals(OrgName::binary(),Requestor::binary()) -> [#pushy_principal{}] is a supertype of the success typing: pushy_principal:request_principals(binary(),binary()) -> [#pushy_principal{requestor_id::'undefined' | binary(),requestor_type::'client' | 'undefined' | 'user',requestor_key::'undefined' | binary()},...]
pushy_principal.erl:56: Type specification pushy_principal:parse_principal_response(#chef_api_response{} | {'error',any()}) -> [#pushy_principal{}] | {'error',any()} is a supertype of the success typing: pushy_principal:parse_principal_response({'error',_} | #chef_api_response{response_api_version::'undefined' | integer(),response_code::'undefined' | string(),response_body::'undefined' | {maybe_improper_list()},response_headers::'undefined' | [any()]}) -> [#pushy_principal{requestor_id::'undefined' | binary(),requestor_type::'client' | 'undefined' | 'user',requestor_key::'undefined' | binary()},...] | {'error',_}

I think I could probably resolve this by putting 'undefined' in the type specs for the record definition and maybe a few other places; but since we have this off in our other erlang projects I figured I would just turn off the warnings here too.

stevendanna commented 9 years ago

CI failed for this on RHEL7 but the failure looks unrelated: http://wilson.ci.chef.co/job/opscode-push-jobs-server-test/342/

@manderson26 any thoughts there? Otherwise this is ready to merge.