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 PXE server credential validation endpoint #1017

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lfu
Copy link
Member

@lfu lfu commented Feb 23, 2021

Depends on ManageIQ/manageiq#21013.

#926

@miq-bot add_label enhancement, kasparov/no

POST /api/pxe_servers
{"action" => "verify_credentials",
 "resource" =>
  {:name => "test", :uri => "smb://tmp/foo", :username => "test", :password => "foo"}
}
RESPONSE
{"results" =>
  [{"success" => true,
    "message" => "Credentials sent for verification",
    "task_id" => "13000000000006",
    "task_href" => "http://localhost:3000/api/tasks/13000000000006"}]
}

@lfu
Copy link
Member Author

lfu commented Feb 23, 2021

@miq-bot cross-repo-tests ManageIQ/manageiq#21013

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Feb 23, 2021
@@ -55,6 +55,13 @@ def edit_resource(type, id, data)
end
end

def verify_credentials_resource(_type, _id = nil, data = {})
task_id = PxeServer.verify_depot_settings_queue(User.current_user.userid, data)
Copy link
Member

Choose a reason for hiding this comment

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

@lfu if you're re-verifying credentials (e.g. if you edit an existing resource) we don't require the user to enter a password again but instead load it from vmdb if it doesn't exist (e.g. they only change the username or IP).

We do this by passing in data["id"] = id if id here and handling it on the verify_depot_settings side if an "id" is passed.

I don't know if that's something that we want to do on the UI for PxeServers but I expect it probably is and would keep parity with provider authentication verification.

Copy link
Member

Choose a reason for hiding this comment

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

@lfu (or @Fryguy @bdunne) do you need to pass PxeServers a zone? I don't see one in the table right now but with any "local" resource it might not be reachable from every zone.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea

Copy link
Member Author

Choose a reason for hiding this comment

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

@lfu if you're re-verifying credentials (e.g. if you edit an existing resource) we don't require the user to enter a password again but instead load it from vmdb if it doesn't exist (e.g. they only change the username or IP).

@agrare verify_depot_settings always creates a new instance to verify the settings.

Copy link
Member

Choose a reason for hiding this comment

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

@lfu yes the problem with that is since we don't pull passwords back over the API if the user is editing an existing instance they have to re-type their password even if they don't intend on changing it.

We can handle that in a follow-up if you want but I don't think that's the best user experience.

Copy link
Member

Choose a reason for hiding this comment

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

@agrare Do we have a similar pattern for this with provider validation you can point to? I think this probably is big enough that we should fix for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry ... didn't realize this was an outdated comment. Is this done then?

Copy link
Member

Choose a reason for hiding this comment

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

Mostly asking because I don't see anything that looks up the existing instance on the core side's method - ManageIQ/manageiq#21013

Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy the API now matches the pattern for provider validation, core still doesn't lookup the instance yet not sure if you want that for this PR set or as a follow-up

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 we need the core change then. Otherwise, we have a "broken" API in the interim.

@lfu lfu force-pushed the pxe_verify_credentials_926 branch 2 times, most recently from 9fec3d3 to d4adfff Compare February 24, 2021 21:17
@lfu lfu force-pushed the pxe_verify_credentials_926 branch from d4adfff to f9a04ac Compare March 1, 2021 19:46
@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2021

Checked commit lfu@f9a04ac with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 1 offense detected

app/controllers/api/pxe_servers_controller.rb

@bdunne bdunne closed this May 18, 2021
@bdunne bdunne reopened this May 18, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request May 18, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request May 18, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request May 18, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request May 18, 2021
@chessbyte
Copy link
Member

chessbyte commented Jun 11, 2021

@miq-bot cross-repo-tests /all

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jun 11, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Sep 1, 2021
@kbrock
Copy link
Member

kbrock commented Nov 30, 2021

is this good to go?

Copy link
Member

@Fryguy Fryguy 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 we are still waiting on #1017 (comment). This was mainly being introduced for UI changes, and I'm not sure if there's priority on it. @kavyanekkalapu would know better. Otherwise, we can just leave this open until we get back to that screen.

@miq-bot miq-bot added the stale label Feb 27, 2023
@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock
Copy link
Member

kbrock commented Apr 12, 2023

Looks like ManageIQ/manageiq#21013 has been merged.
Does that mean we can merge this one?

If we need changes, I can rebase/fixup.
If it is not a priority, then that is good too

We were held up on #1017 (comment)

the API now matches the pattern for provider validation, core still doesn't lookup the instance yet not sure if you want that for this PR set or as a follow-up

Ah, never mind. I mixed up the dates and thought that PR was merged after Nov 2021, but it was merged before

@kbrock kbrock removed the stale label Apr 12, 2023
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Apr 12, 2023
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Apr 12, 2023
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Apr 12, 2023
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Apr 12, 2023
@miq-bot miq-bot added the stale label Jul 17, 2023
@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot miq-bot closed this Oct 23, 2023
@miq-bot
Copy link
Member

miq-bot commented Oct 23, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

2 similar comments
@miq-bot
Copy link
Member

miq-bot commented May 13, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Aug 19, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants