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

Giving a choice between SLO and Starchart logout #366

Merged
merged 14 commits into from
Mar 22, 2023

Conversation

sfrunza13
Copy link
Contributor

Closes #362

There should be some discussion around this in the coming days, I would like to maybe make this an alert the way we have an alert for delete record but I don't know how to do this across all components.

For now I have made logout.tsx a page with some UI, a couple of buttons to select which type of logout to perform and placed it under the index folder to get the header on it and have it look more a part of the app.

I added back the bits of SLO logic that I had before, this creates a request to the IDP to logout and then receives a response in the query string on the callback.

image

@sfrunza13 sfrunza13 self-assigned this Mar 17, 2023
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Most people aren't going to care about the distinction you're drawing here, and I want to de-emphasize it.

When I sign-out, it should sign me out of Starchart automatically. You can give a message saying, "You've been signed out of My.Custom.Domain. If you'd also like to sign out of Seneca, click here" or something.

This way, people can safely ignore what you're doing here vs. having 2 big call to action buttons that are going to cause confusion.

app/routes/__index/logout.tsx Outdated Show resolved Hide resolved
@sfrunza13
Copy link
Contributor Author

sfrunza13 commented Mar 18, 2023

Most people aren't going to care about the distinction you're drawing here, and I want to de-emphasize it.

When I sign-out, it should sign me out of Starchart automatically. You can give a message saying, "You've been signed out of My.Custom.Domain. If you'd also like to sign out of Seneca, click here" or something.

This way, people can safely ignore what you're doing here vs. having 2 big call to action buttons that are going to cause confusion.

I have been wondering how to keep the user after destroying the session, how do I know who to SLO?

@sfrunza13
Copy link
Contributor Author

sfrunza13 commented Mar 18, 2023

This also means that if this SLO prompt will be with the rest of the index stuff I will have to change the header to useOptionalUser cause the signout will happen before the prompt for SLO. Which is still a problem since I don't know what user to SLO. I have a feeling I am thinking about this wrong and am missing an obvious solution.

@humphd
Copy link
Contributor

humphd commented Mar 18, 2023

I see your issue. Let's give the user a choice:

  1. Sign out of My.Custom.Domain
  2. Sign out of My.Custom.Doman and all Seneca services

Make 1. the red call-to-action button, and have 2. be gray/lighter, so you don't naturally use it.

@sfrunza13
Copy link
Contributor Author

I see your issue. Let's give the user a choice:

  1. Sign out of My.Custom.Domain
  2. Sign out of My.Custom.Doman and all Seneca services

Make 1. the red call-to-action button, and have 2. be gray/lighter, so you don't naturally use it.

I have an idea, it might be awful I can't really tell but I am going to commit it so you can see.

@humphd
Copy link
Contributor

humphd commented Mar 18, 2023

I have an idea, it might be awful I can't really tell but I am going to commit it so you can see.

Test it on some Seneca friends, see if they can make sense of it. That's who we are building this for.

@sfrunza13
Copy link
Contributor Author

sfrunza13 commented Mar 18, 2023

Is this an ok use of global var? do I need to use global here?

image

app/components/header.tsx Outdated Show resolved Hide resolved
app/components/header.tsx Outdated Show resolved Hide resolved
app/routes/__index/logout.tsx Outdated Show resolved Hide resolved
app/routes/__index/logout.tsx Outdated Show resolved Hide resolved
@sfrunza13 sfrunza13 requested a review from humphd March 18, 2023 20:34
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I'd love to go through this with you on a call this week to understand some of your choices. I'm not sure I understand all of this.

@sfrunza13
Copy link
Contributor Author

I would be happy to impart what I understand about my own choices : ' )

@Ririio Ririio added this to the Milestone 0.7 milestone Mar 19, 2023
@sfrunza13 sfrunza13 requested a review from humphd March 21, 2023 17:36
@SerpentBytes SerpentBytes self-requested a review March 21, 2023 19:19
SerpentBytes
SerpentBytes previously approved these changes Mar 21, 2023
Copy link
Contributor

@SerpentBytes SerpentBytes left a comment

Choose a reason for hiding this comment

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

Nice. Works on my machine.

app/components/header.tsx Outdated Show resolved Hide resolved
app/routes/logout.tsx Outdated Show resolved Hide resolved
app/routes/logout.tsx Outdated Show resolved Hide resolved
app/routes/logout.tsx Outdated Show resolved Hide resolved
app/session.server.ts Outdated Show resolved Hide resolved
app/routes/logout.tsx Outdated Show resolved Hide resolved
app/routes/logout.tsx Outdated Show resolved Hide resolved
app/routes/logout.tsx Outdated Show resolved Hide resolved
@sfrunza13 sfrunza13 requested a review from humphd March 22, 2023 14:44
humphd
humphd previously approved these changes Mar 22, 2023
.eslintrc.js Outdated Show resolved Hide resolved
app/routes/logout.tsx Show resolved Hide resolved
@humphd humphd requested review from Ririio and Eakam1007 March 22, 2023 15:22
Copy link
Contributor

@Ririio Ririio left a comment

Choose a reason for hiding this comment

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

Runs as intended on my system

@sfrunza13 sfrunza13 merged commit cdbfef3 into DevelopingSpace:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding SLO prompt
4 participants