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

Fixes #6211,BZ1102521 - Use hidden user's remote id to ping pulp_auth #4265

Merged
merged 1 commit into from Jun 17, 2014
Merged

Fixes #6211,BZ1102521 - Use hidden user's remote id to ping pulp_auth #4265

merged 1 commit into from Jun 17, 2014

Conversation

daviddavis
Copy link
Contributor

No description provided.

@ehelms
Copy link
Member

ehelms commented Jun 13, 2014

I am OK with this for now, but we should use the actual status check (http://pulp-dev-guide.readthedocs.org/en/latest/integration/rest-api/status.html) to do this. Could you make an issue to address this since I imagine it requires a Runcible and Katello update?

@daviddavis
Copy link
Contributor Author

@ehelms it doesn't look like that route uses authentication though which would defeat the purpose of the auth check. From your link:

An unauthenticated resource that shows the current status of the pulp server.

@daviddavis
Copy link
Contributor Author

We can have the regular pulp ping check (pulp_without_oauth) use the status route:

#4267

@ehelms
Copy link
Member

ehelms commented Jun 13, 2014

Ahhh, why do we check auth and unauth status?
On Jun 13, 2014 11:57 AM, "David Davis" notifications@github.com wrote:

@ehelms https://github.com/ehelms that doesn't look like an auth route
though.


Reply to this email directly or view it on GitHub
#4265 (comment).

@jlsherrill
Copy link
Member

@ehelms So we can more easily check if there is an auth error versus there being an issue with the pulp server

@ehelms
Copy link
Member

ehelms commented Jun 13, 2014

Line 25 says ping should be called with the admin user and we also ping
candlepin auth as the admin. Should we be consistent here instead of using
the hidden user?
On Jun 13, 2014 1:35 PM, "Justin Sherrill" notifications@github.com wrote:

@ehelms https://github.com/ehelms So we can more easily check if there
is an auth error versus there being an issue with the pulp server


Reply to this email directly or view it on GitHub
#4265 (comment).

@daviddavis
Copy link
Contributor Author

we also ping candlepin auth as the admin

Can you point me to that? I tried looking for the code but couldn't find it.

@ehelms
Copy link
Member

ehelms commented Jun 14, 2014

@daviddavis We only have a single user in Candlepin, the admin user unlike Pulp.

@daviddavis
Copy link
Contributor Author

So then it's inconsistent? In which case, that point is moot right?

Regarding the comment on line 25, this should use the admin user if the admin user is logged in (hence the if).

@ehelms
Copy link
Member

ehelms commented Jun 14, 2014

https://github.com/daviddavis/katello/blob/temp/20140613091921/app/models/katello/ping.rb#L25 -- seems misleading then. Should an unauthenticated user be able to check Pulp status with auth?

@daviddavis
Copy link
Contributor Author

I don't know. I was just trying to fix the exception and I hadn't thought about it.

@ehelms
Copy link
Member

ehelms commented Jun 14, 2014

@daviddavis that is why I am wondering if it should just check if User.current exists and only return that status if so.

@daviddavis
Copy link
Contributor Author

@ehelms it looks like when you call the v2 ping api, User.current NEVER exists even if you pass in credentials. The fix probably needs to be made in the controller so that it sets the user and checks the user's permissions.

The question then would be: which permissions do we allow for ping? Any authenticated user can ping? Or (according to line 25) only admins?

@ehelms
Copy link
Member

ehelms commented Jun 14, 2014

+1
On Jun 14, 2014 9:27 AM, "David Davis" notifications@github.com wrote:

So I'll fix the controller to require a user (any user) and have the code
check for a logged in user before it does its pulp_auth ping. If there is a
user, it proceeds. If not, it skips the pulp_auth check. Sounds good?


Reply to this email directly or view it on GitHub
#4265 (comment).

It looks like the code didn't properly set User.current so Katello.pulp_server
was nil. This code checks for a user in the controller. Also, it handles the
case of no user by skipping the pulp_auth check which needs remote_id from a
user.
@@ -18,7 +18,7 @@ class Api::V2::PingController < Api::V2::ApiController
end

skip_before_filter :authorize
skip_before_filter :require_user, :only => [:server_status]
before_filter :require_login, :only => [:index]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't ask me why but require_login doesn't actually require a login or user. It just sets User.current if credentials were passed. I tested it:

❯ curl "http://localhost:3000/katello/api/v2/ping"
{"status":"FAIL","services":{"elasticsearch":{"status":"ok","duration_ms":"13"},"katello_jobs":{"status":"FAIL","message":"katello-jobs service not running"},"candlepin":{"status":"ok","duration_ms":"564"},"candlepin_auth":{"status":"ok","duration_ms":"82"},"pulp":{"status":"ok","duration_ms":"21"},"pulp_auth":{}}}

@ehelms
Copy link
Member

ehelms commented Jun 16, 2014

APJ

daviddavis pushed a commit that referenced this pull request Jun 17, 2014
Fixes #6211,BZ1102521 - Use hidden user's remote id to ping pulp_auth
@daviddavis daviddavis merged commit 9510426 into Katello:master Jun 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants