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 #1899 warn if oVirt API is HTTPS-only and HTTP URL given #245

Closed
wants to merge 1 commit into from

Conversation

domcleal
Copy link
Contributor

@@ -41,13 +41,28 @@ def clusters
client.clusters
end

# Check if HTTPS is mandatory, since rest_client will fail with a POST
def test_https_required
RestClient.post url, {} if url.match /^http:/
Copy link
Member

Choose a reason for hiding this comment

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

isnt it enough to ask if https is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, though the example URL today already is HTTPS, so clearly the mistake can still be made. IMO the bug lies in the test connection button, since it reports the connection is OK currently, even though it may later fail (rest_client refuses to follow 302s with POSTs).

Copy link
Member

Choose a reason for hiding this comment

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

I think my question was, can't we simply verify that its https in the first place? why do we actually need to post to the server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I checked with #ovirt, we're safe to require an HTTPS URL, so I'll do this as field validation in another PR.

@domcleal domcleal closed this Nov 16, 2012
h0jeZvgoxFepBQ2C pushed a commit to h0jeZvgoxFepBQ2C/foreman that referenced this pull request Jul 26, 2018
cfouant pushed a commit to cfouant/foreman that referenced this pull request Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants