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

Replace 'Login with...' page with auto-redirect script #29

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

aaga
Copy link
Collaborator

@aaga aaga commented Aug 10, 2023

There is also localStorage logic in here to support the "signup intent" feature (two separate buttons). That will be a separate PR on Elk though.

I used localStorage because I can't figure out how to prevent Mastodon from stripping all the URL params when redirecting to /auth/sign_in.

First part of https://mozilla-hub.atlassian.net/browse/SOCIALPLAT-104

@aaga
Copy link
Collaborator Author

aaga commented Aug 10, 2023

Screen cap of new flow:

Screen.Recording.2023-08-10.at.7.11.47.PM.mov

@aaga aaga requested review from bassrock and toufali August 10, 2023 23:17
Copy link
Collaborator

@toufali toufali left a comment

Choose a reason for hiding this comment

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

This works on my machine, and assuming no issues with other clients should suffice!

If you're interested in digging deeper, I imagine there should be a better way to hijack the flow in the controller, before we hit the view?

I looked into it briefly and found that by creating a new method in app/controllers/auth/sessions_controller.rb, I was able to get into the flow before the view renders:

  def new
    redirect_to('https://mozilla.com')
    # HTTP.post('https://mastodon.mozsoc.local/auth/auth/openid_connect')
  end

Getting it to trigger the OIDC flow is of course another matter... we'd need to spend more time on it.

app/views/auth/sessions/new.html.haml Outdated Show resolved Hide resolved

- unless omniauth_only?
= simple_form_for(resource, as: resource_name, url: session_path(resource_name)) do |f|

Choose a reason for hiding this comment

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

Could we keep all this but add a new param instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I realized I can use the sso_redirect param that was defined in mastodon#26083, if I merge that one in

Choose a reason for hiding this comment

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

Yay!

Choose a reason for hiding this comment

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

Could actually merge this back upstream too with that.

@aaga
Copy link
Collaborator Author

aaga commented Aug 11, 2023

@toufali re: hijacking this flow earlier - I've gone down that rabbit hole a few times and unfortunately come up blank each time. My findings are:

  • /auth/auth/openid_connect needs to be navigated to via POST, and I haven't found a way to do that without injecting a DOM script. Every other redirect capability only does GET. The server can make POST requests itself, but it can't instruct the browser to make a POST request via a HTTP 30X response, which (I think..) is what is required.
  • /auth/auth/openid_connect also has CSRF protection. This means (I think..) that it's expecting a CSRF authenticity token, with a corresponding mastodon cookie, to ensure that the navigation isn't via a maliciously forged link. And the only way I can see to set those is by a) "touching" a Mastodon page first for them to be set (which is basically what this solution does) or b) pulling that CSRF stuff into Elk somehow (which, 😬😬😬)

But I gently lay down the gauntlet to be picked up now or in the future!

@aaga aaga requested a review from toufali August 14, 2023 22:53
@aaga aaga closed this Aug 14, 2023
@aaga aaga deleted the skip-login-interstitial branch August 14, 2023 23:05
@aaga aaga reopened this Aug 14, 2023
@aaga
Copy link
Collaborator Author

aaga commented Aug 14, 2023

Addressed comments from above

@toufali I could not for the life of me figure out how to get sso_redirect from InitialStateSerializer into the haml file, so I redefined it in ApplicationController

I was aggressive about minimizing the HTML that is rendered in the redirect case, so that the page loads and redirects as fast as possible.

@aaga aaga merged commit 7107548 into main Aug 15, 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
3 participants