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

Set cookie token on omniauth success to avoid having to store the scraped query param token in the client #1539

Closed
wants to merge 1 commit into from

Conversation

theblang
Copy link
Contributor

@theblang theblang commented Jun 24, 2022

In #1453 we added support for sending and receiving the auth token as a cookie. We're currently working on a de-angularized version of ng-token-auth with cookie support, which means we don't need to store the token in client storage anymore. But I realized that the OAuth flow still requires client storage because the token is scraped from query params, temporarily stored in client storage, then sent to the validate_token call, where a cookie is created. I realized that we could instead just create the cookie immediately in the same place that we're creating the query params in order to avoid having to leak the token in client storage just for that small initial period.

@theblang
Copy link
Contributor Author

theblang commented Jun 24, 2022

Hey @MaicolBen ! Quick question, I started to write a test for this, but when following the instructions I hit the following error:

Rails couldn't infer whether you are using multiple databases from your database.yml and can't generate the tasks for the non-primary databases. If you'd like to use this feature, please simplify your ERB.

I think this was also mentioned in #1456 . Any ideas of how I should proceed?

@theblang
Copy link
Contributor Author

theblang commented Jul 7, 2022

Going to close this PR in favor of opening a new one whose source isn't my fork's master. Need to make a similar additional change for the reset password flow.

@theblang theblang closed this Jul 7, 2022
@theblang
Copy link
Contributor Author

theblang commented Jul 8, 2022

See #1542 instead

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.

1 participant