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

fix(auth): Fix repeat auth calls on logout #321

Merged
merged 13 commits into from
Oct 18, 2021

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Oct 8, 2021

Fixes #320

@jan-law jan-law requested a review from andrewazores October 8, 2021 17:27
@jan-law jan-law marked this pull request as draft October 8, 2021 21:50
@jan-law jan-law force-pushed the fix-repeated-auth-calls branch from 30f1471 to 24ea4f3 Compare October 13, 2021 19:47
@jan-law jan-law marked this pull request as ready for review October 13, 2021 20:07
@jan-law jan-law force-pushed the fix-repeated-auth-calls branch from f4c79c4 to 727b3fc Compare October 15, 2021 16:35
@jan-law jan-law requested review from andrewazores and removed request for andrewazores October 18, 2021 15:14
@andrewazores
Copy link
Member

Just testing this out quickly, it seems to work very nicely when using the BasicAuthManager on the backend. However, when I use the standard development NoopAuthManager, the behaviour is a little funny. It displays the Bearer auth login page, and if I leave the token field blank and submit the form, I see that the client makes the POST /api/v2.1/auth request and receives a 200 response, but it stays on the login page. However, if I enter any non-empty token string, then the "auth" request succeeds again, and the login page is dismissed and I'm able to enter into and navigate the web-client application as usual.

@jan-law
Copy link
Contributor Author

jan-law commented Oct 18, 2021

but it stays on the login page

The latest fixup should let you login with an empty token when using the NoopAuthManager.

It displays the Bearer auth login page

When querying the auth method by calling /auth for the first time, I changed checkAuth('', 'Basic') to checkAuth('', '') in order to display the Basic Login page with their saved credentials until they press Login (instead of the above query automatically logging in the user). Since the authMethod was saved as '', the Login component now displays the Bearer form due to the line authMethod === 'Basic' ? <BasicAuthForm /> : <BearerAuthForm />. Would you rather the web client display the Basic form when using NoopAuthManager?

@andrewazores
Copy link
Member

I don't think it really matters what's displayed with the NoopAuthManager, so long as just clicking Submit lets the user in without having to enter some arbitrary faked credentials. Even better would be if NoopAuthManager led to the Login form being automatically dismissed since the default no-credentials case should count as valid authentication there, but that's just a nice-to-have. We aren't expecting anyone to use NoopAuthManager other than developers at this point anyway, and unless we decide to package up and support a "local installation" of Cryostat (either as a bare JVM process or as a local Podman container), I don't see that changing.

I changed checkAuth('', 'Basic') to checkAuth('', '') in order to display the Basic Login page with their saved credentials until they press Login (instead of the above query automatically logging in the user)

How would this interact/behave with saved credentials for Bearer tokens? It's best to avoid making any assumptions or having some preferential treatment for one login form over the others, I think.

@jan-law
Copy link
Contributor Author

jan-law commented Oct 18, 2021

checkAuth('', '') was an attempt to keep the behaviour consistent between the Basic and Bearer methods while following your recommendation for the login state machine.

The Login.tsx component could repopulate itself from sessionStorage/localStorage, but it would require the user to click the Submit button to set the Login.service state machine back in motion.

If you check out the main branch while using BasicAuthManager, check Remember Me, login, then refresh the page, the login screen is briefly displayed with your saved credentials, then the original method query checkAuth('', 'Basic') will automatically log the user in. For the OpenShiftAuthManager however, the method query checkAuth('', 'Basic') does not log the user in and the user stays on the login page with the repopulated token. If I use checkAuth('', '') as the method query, users will remain on the Login page with the repopulated fields - regardless of whether they are using the Basic or Bearer form - until they click Login.

@andrewazores
Copy link
Member

Makes sense, thanks.

Tangent:
The NoopAuthManager reports its authenticate scheme (X-WWW-Authenticate response header) as Basic - we could change this on the backend to be some other value to differentiate it from Basic auth. It could respond with None, for example, which is also what the Apache HTTP server responds with in a similar scenario. I think we could then implement a NoopAuth frontend component that just immediately calls its onSubmit prop on load. What do you think?

@andrewazores andrewazores merged commit 80cf1f3 into cryostatio:main Oct 18, 2021
@jan-law
Copy link
Contributor Author

jan-law commented Oct 18, 2021

I think we could then implement a NoopAuth frontend component that just immediately calls its onSubmit prop on load. What do you think?

I like the idea of the NoopAuthManager sending None as an authentication scheme. Since this new NoopAuth frontend component would likely only be used by developers, what happens when you rebuild an image with the new web client assets? Does that just mean the component never gets used in a real scenario?

@jan-law jan-law deleted the fix-repeated-auth-calls branch October 18, 2021 19:35
@andrewazores
Copy link
Member

Yea, unless we do some build-time hacks to strip it out to save a few bytes of storage space, the component would end up included in the shipped Cryostat image, but we would just never expect anyone to actually configure their Cryostat instance to trigger it to show up.

It's not that dissimilar to the downstream productized builds with the Operator only using Bearer auth in practice, but still containing the Basic auth form and code for handling Basic credentials.

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

Successfully merging this pull request may close these issues.

Do not retry login after logging out
3 participants