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

[docs] Clarify OAuth flow documentation #2472

Merged
merged 3 commits into from
Aug 4, 2023
Merged

[docs] Clarify OAuth flow documentation #2472

merged 3 commits into from
Aug 4, 2023

Conversation

Seltyk
Copy link
Contributor

@Seltyk Seltyk commented Jul 20, 2023

Description

  • add note about using AUGUR_DEV envvar to allow localhost clients (i.e. to disable SSL enforcement)
  • remove Augur View and docs terminology disparity
  • fix "acquired" typo

Notes for Reviewers

In my opinion, the SSL issue should be met with a backend change. Assuming Augur is running with HTTPS support, enforcing SSL into Augur is a good idea, but enforcing it out of Augur appears to be why localhost clients don't work. The redirect back to the user's client application at http://127.0.0.1 fails SSL enforcement silently, responding with (non-existent) temporary authorization code "undefined". Should I make a GitHub issue for this?

Signed commits

  • Yes, I signed my commits.

- add note about AUGUR_DEV envvar and localhost
- remove Augur View and docs terminology disparity
- fix "acquired" typo

Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
@Seltyk Seltyk requested a review from sgoggins as a code owner July 20, 2023 16:16
@Ulincsys
Copy link
Contributor

The doc updates look good to me. I'm not sure I understand what the SSL issue is you've mentioned? I'm able to authenticate with a client locally from an instance of augur served behind HTTPS.

@Seltyk
Copy link
Contributor Author

Seltyk commented Jul 21, 2023

The Jinja template authorization.j2 gives Flask the redirect target to handle temporary auth code generation to then be handed back to Jinja. From what I can tell, if that target lacks SSL (because, say, the redirect is to http://localho.st), the endpoint rejects immediately and Jinja is left with a {"status": "SSL required"} object when it expects an object with a code field. No code field means data.code is undefined.

It's possible this is my experience alone, but I'm puzzled that it happens at all.

@Ulincsys
Copy link
Contributor

The application's redirect URL is not considered as part of the request to generate a temporary auth code. As long as the user agent makes the request to the backend with https as the scheme, then the requirement for SSL should be met (assuming that the certs have been setup on the host).

If the instance of Augur you're querying is hosted behind a server like Nginx or Apache, you'll additionally need to make sure you have the following parameter set in your sites-enabled configuration under the location which targets the backend:

proxy_set_header        X-Forwarded-Proto           $scheme;

@Seltyk
Copy link
Contributor Author

Seltyk commented Jul 24, 2023

Further testing reveals I'm actually getting a 426 from /user/authorize when I click the "authorize" button, which is not resolved by the nginx config you mentioned. AUGUR_DEV=1 does fix the issue, however, which strongly implies there are some SSL shenanigans happening with my frontend. I will tweak my PR when the solution is found.

Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
@Seltyk
Copy link
Contributor Author

Seltyk commented Jul 31, 2023

4 days of confusion down to one little typo. Fixed in the latest commit. This doesn't solve the SSL thing mentioned above, but it solves another problem that had been lurking in my code the whole darn time.

@Ulincsys
Copy link
Contributor

Ulincsys commented Aug 2, 2023

If you have some time on Wednesday or Thursday @Seltyk I think it might help to meet via Zoom to resolve the SSL issue.

@Seltyk
Copy link
Contributor Author

Seltyk commented Aug 2, 2023

I'm available until 16:00 EDT today and until 18:00 EDT tomorrow. GitHub says you're in CDT, so that should be easy to work out.

@Ulincsys
Copy link
Contributor

Ulincsys commented Aug 3, 2023

Let's plan for 3pm CDT Thursday. My work email should be visible on my profile page, if you'd like to reach out there we can arrange the meeting link.

@Ulincsys Ulincsys merged commit da537ce into chaoss:dev Aug 4, 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.

2 participants