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

Use cookie authentication against the API, drop the token renewal #5164

Merged
merged 3 commits into from
Jan 22, 2019

Conversation

skateman
Copy link
Member

If the API allows cookie authentication, there is no need for the API token requesting and renewal processes. After a successfull login the API requests should be implicitly authorized until the cookie is available. This allows us to delete a few hundred lines of JS code. However, we have to add a new header for those API requests to distinguish them from regular requests if the session expires. This way we can prevent displaying a HTTP basic auth dialog if the cookie times out.

@miq-bot assign @himdel

Depends on: ManageIQ/manageiq-api#543

@himdel
Copy link
Contributor

himdel commented Jan 15, 2019

You might also want to remove the API.logout call from app/views/layouts/login.html.haml

EDIT: done 👍

@skateman skateman force-pushed the api-cookie branch 2 times, most recently from dea3cf6 to 32c619a Compare January 16, 2019 13:25
@jvlcek
Copy link
Member

jvlcek commented Jan 17, 2019

I worked with @skateman to test these changes. The testing done was with SAML via Keycloak, External Auth and SSO (kerberos) via IPA. I believe this is a good test sampling. LGTM JoeV

@skateman skateman changed the title [WIP] Use cookie authentication against the API, drop the token renewal Use cookie authentication against the API, drop the token renewal Jan 17, 2019
@miq-bot miq-bot removed the wip label Jan 17, 2019
@skateman
Copy link
Member Author

@miq-bot add_label pending core, hammer/no
@miq-bot add_reviewer @martinpovolny
@miq-bot add_reviewer @mzazrivec

@skateman
Copy link
Member Author

I'm moving the decision about the origin verification from the API PR to here. @martinpovolny the API verifies the Origin header if it is set, the question is: should we append this header to every API request or just let the browser decide about it based on the request method?

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2019

api and appliance PRs merged

@skateman skateman force-pushed the api-cookie branch 3 times, most recently from 92f7a7a to 3c8c841 Compare January 17, 2019 21:38
@skateman
Copy link
Member Author

@himdel I added back the window.localStorage.userFeatures code in a hacky way so we won't forget about it. I will do a refactoring of this and the WS token retrieval in a followup PR.

@skateman
Copy link
Member Author

@miq-bot rm_label pending core

package.json Show resolved Hide resolved
@himdel
Copy link
Contributor

himdel commented Jan 21, 2019

There... is a slight chance this may be exposing an API bug...

I'm getting

URL http://localhost:3000/api/notifications?expand=resources&attributes=details&sort_by=id&sort_order=desc&limit=100

Status 500 Internal Server Error

Content-Type application/json; charset=utf-8

Data {"error":{"kind":"internal_server_error","message":"Circular dependency detected while autoloading constant ContainerProject","klass":"RuntimeError"}}

right after logging in when testing the PR.

(api.log)

EDIT: added a note to ManageIQ/manageiq-api#544

@skateman
Copy link
Member Author

@himdel this bug was already present even before this PR and @Hyperkid123 suffered because of that.

@miq-bot
Copy link
Member

miq-bot commented Jan 22, 2019

Checked commits skateman/manageiq-ui-classic@8def1c0~...f161abd with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@himdel himdel added this to the Sprint 104 Ending Feb 04, 2019 milestone Jan 22, 2019
@himdel himdel merged commit a3bbe07 into ManageIQ:master Jan 22, 2019
@himdel
Copy link
Contributor

himdel commented Jan 22, 2019

Looks like the login fails sensibly (on failure), and also works (on success) :), merged.

@himdel
Copy link
Contributor

himdel commented Jan 28, 2019

If we ever backport this, need to add #5203 so that the API always gets the cookie :).

EDIT: and #6065 + #6325

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.

5 participants