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 SPA Form Based Authentication instructions #36818

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

melloware
Copy link
Contributor

Based on Zulip conversation with @sberyozkin and @michalvavrik

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Nov 1, 2023
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Nov 1, 2023
Quarkus Documentation automation moved this from To do to Reviewer approved Nov 1, 2023
@gastaldi
Copy link
Contributor

gastaldi commented Nov 1, 2023

Mind squashing all commits before this is merged?

@melloware
Copy link
Contributor Author

Squashed!

Copy link

github-actions bot commented Nov 1, 2023

🙈 The PR is closed and the preview is expired.

@gastaldi
Copy link
Contributor

gastaldi commented Nov 1, 2023

Great, the preview link is now broken, looks like it was caused by merging quarkusio/quarkusio.github.io#1826?

const removeCookie = `quarkus-credential=; Max-Age=0;path=/`;
document.cookie = removeCookie;
// redirect to main page
window.location.href = "/";
Copy link
Contributor

Choose a reason for hiding this comment

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

You are documenting logout for SPA with AJAX, I hope this won't confuse users. I think there is many occasions where you want to redirect and it depends on use case whether you want to loose state and use window.location.href or keep state and use routing function provided by TypeScript-based SPA framework (for I think it would be challenging to write SPA with only JS + AJAX).

So in short: I think you intention is rather meta-code than something users will copy & paste and your examples are great in that way, but cookie is going to be removed whether you redirect or not. You could just execute some post-logout actions or whatever.

To logout of the SPA you must destroy the cookie and redirect back to your main page.

That said, I wrote app that did exactly this redirect in past, so I'm well aware this can be a case. I'll leave decision whether to keep it or not on you, personally I don't like to create an impressions that you need to redirect in SPA, there are other ways to reset state while keeping session data in SPA FWs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michalvavrik I updated it to say // perform post logout actions here such as redirecting back to your login page to leave it up to the user what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@michalvavrik
Copy link
Contributor

I wonder if this could close #27389 (as front-end based solution), because it doesn't make sense to me to add logout handler into Quarkus (it's too easy to delete cookies and I'd not want to make impression they need to call back-end for it). What everyone think?

@melloware
Copy link
Contributor Author

@michalvavrik i can change to what you want but all of this I had to figure out piecing together tickets and info just trying to get this working. Deleting the cookie seemed like the only way to logout of the SPA. Do you have an alternative?

@michalvavrik
Copy link
Contributor

@michalvavrik i can change to what you want but all of this I had to figure out piecing together tickets and info just trying to get this working. Deleting the cookie seemed like the only way to logout of the SPA. Do you have an alternative?

Maybe we don't understand each other, I agree with deleting cookie, I just tried to point out that cookie is deleted whether you redirect or not and in SPA (am I wrong? I didn't write FE for about a year), when you do window.location.href you are leaving SPA. I think you can leave it out, that's all.

@melloware
Copy link
Contributor Author

No I am using Quarkus quinoa and if you delete the cookie and redirect it takes me back to my login screen. However I get your point I could do React Router navigate to / and it works also.

Let me update that section to be more clear!

----
const logout= () => {
// delete the credential cookie essentially killing the session
const removeCookie = `quarkus-credential=; Max-Age=0;path=/`;
Copy link
Member

Choose a reason for hiding this comment

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

Does it work out of the box, with the httpOnly flag set to true by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sberyozkin out of the box it looks like httpOnly is set to false by default:
image

Copy link
Member

Choose a reason for hiding this comment

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

@melloware thanks, https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/FormAuthConfig.java#L127 indeed have a default false value. It should be set to true by default, but it is another issue.

So IMHO it has to be noted that this solution only works with the HttpOnly disabled, and I wonder if it is a good idea to recommend a logout solution which depends on the cookie being accessible to JavaScript.

Should a logout handler solution be recommended ?

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 points. I guess I should mention that it has to be httponly false and also provide an example server side controller logout?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be set to true by default

+1

I wonder if it is a good idea to recommend a logout solution which depends on the cookie being accessible to JavaScript.

Here @melloware tries to document logout for SPA. SPA FWs actively prevents XSS (sanitize) unless you explicitly override it, in which is you are aware of what you are doing. All points you raised are absolutely true, but what is recommended here is not dangerous for typical SPA.

That is why I suggested this new code examples should be rather treated as meta code, because JS + AJAX doesn't provide OOTB protection nor the typical way you handle things (how you store state, route, ...) with SPA.

Good points. I guess I should mention that it has to be httponly false and also provide an example server side controller logout?

+1

I'd also mention why - XSS and make it clear when it is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This thread is on logout, same is true for login documented here because you won't be able to use this cookie for BE requests if you can't access it.

Copy link
Contributor

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

thank you

@melloware
Copy link
Contributor Author

Added docs for HttpOnly setting for cookie mentioning false if you want client side logout and true for server side logout.

Included Client, RestEasy Classic, and Reactive examples of logging out

Copy link
Contributor

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

To logout of the SPA from the server the cookie can be set to quarkus.http.auth.form.http-only-cookie=true but and use this example code to destroy the cookie.

I don't believe we agree on basic principle of SPA, if you can't access credentials cookie, then you can't make authenticated HTTP requests to back-end, which means you need to leave SPA (for example submit form, click on hyperlink, ...), which means that is not SPA by definition.

I'm sorry @melloware this PR takes too long, you are putting a good work in, thank you. I'd suggest to strongly highlight perils of HttpOnly set to false and that is it.

@michalvavrik
Copy link
Contributor

Hmm, there is one else thing and I should probably try it before I write more - did anyone tried what is effect of HttpOnly when using HTTP client on frontend? I'd expect there is none, so it should be fine as long as everything is done via the client. Thoughts @melloware ?

@melloware
Copy link
Contributor Author

@michalvavrik I am testing all of this with a React SPA app right now And what I have verified.

  1. HttpOnly true everything in my SPA works fine you are allowed to use to cookie to make requests the cookie is just not modifiable by JavaScript so trying the removeCookie trick nothing happens. you HAVE to use server side logout.

  2. HttpOnly false the cookie can be manipulated by JavaScript and thus the client side logout trick works.

@michalvavrik
Copy link
Contributor

@michalvavrik I am testing all of this with a React SPA app right now And what I have verified.

  1. HttpOnly true everything in my SPA works fine you are allowed to use to cookie to make requests the cookie is just not modifiable by JavaScript so trying the removeCookie trick nothing happens. you HAVE to use server side logout.

Can you share the app (unless it is too much of a problem)? If you create HTTP client on front end, it will only have headers and cookies you set it, therefore you are the owner of logic what to do with HttpOnly. And you don't need to delete cookies from browser as they are not stored in a browser. I'd like to learn why it works differently than that, thank you. I trust you, but it is really interesting.

@melloware
Copy link
Contributor Author

Let me see if I can put together a simple reproducer for you.


@POST
public Response logout() {
if (identity.getIdentity().isAnonymous()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Authenticated would do a same job if you insist on 401. Personally I don't know what is expected behavior.

@michalvavrik
Copy link
Contributor

Let me see if I can put together a simple reproducer for you.

Thanks! As long as it is not too much of a job.

@michalvavrik
Copy link
Contributor

@melloware never mind I remembered how it works, the browser is in control. Okay, I am fine with changes. I'll let @sberyozkin to do rest, thanks again!

@sberyozkin sberyozkin self-requested a review November 4, 2023 22:14
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks, we can follow up with a dedicated issue to set http only to true by default

@sberyozkin sberyozkin merged commit fded7c0 into quarkusio:main Nov 4, 2023
5 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Nov 4, 2023
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Nov 4, 2023
@melloware melloware deleted the patch-1 branch November 4, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation
Development

Successfully merging this pull request may close these issues.

None yet

4 participants