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 Generic OAuth2 Provider Support #18

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ethantmcgee
Copy link

No description provided.

@ethantmcgee
Copy link
Author

This pull request adds support for generic OAuth2 providers like Fusion Auth (https://fusionauth.io). I've also added a sample docker-compose.yml file to help with deploying the project more easily.

@ethantmcgee
Copy link
Author

ethantmcgee commented Aug 14, 2022

I also fixed an issue where the OTP / OTC codes did not work in mobile safari (and added integrity hashes / nonces for security)

@m-barthelemy
Copy link
Owner

Thanks for the PR!
I'll review it but it might take me a few weeks in order to get the time to do it + test the result.

Copy link
Owner

@m-barthelemy m-barthelemy left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the pull request!
Adding support for a generic Auth2 provider is great and I'd love to merge this.

I have added a few comments and questions, let me know what you think.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
services/oauth2_provider.go Show resolved Hide resolved
templates/enter2fa.html Outdated Show resolved Hide resolved
Comment on lines 6 to 12
<link href="/assets/material-icons.css" rel="stylesheet" integrity="sha384-Tn+eHgvLDlHfZ/Bd0HmrFRKnaocbJJECUsEAjjg2gey5liDBv1trMEyh2l7XC2C+">
<link rel="stylesheet" type="text/css" href="/assets/materialize-0.97.5.min.css" integrity="sha384-1ji7hb9jc+M2e4aPgCIK93lON9Hxx38Vo/3oNk9vrJsU8JbrdFdLs+VmVE1YNiuM">
<link rel="stylesheet" type="text/css" href="/assets/font-awesome.min-4.7.0.css" integrity="sha384-wvfXpqpZZVQGK6TAh5PVlGOfQNHSoD2xbE+QkPxCAFlNEevoEH3Sl0sibVcOQVnN">
<link rel="stylesheet" type="text/css" href="/assets/css.css" integrity="sha384-TeXUgdWM9e/rDL9CedCXsh4Z0a6aZJUiyD7Vfz3S+H1REIOyQEXhpneyhHZswE3a">
<script type="text/javascript" src="/assets/jquery-3.5.1.min.js" integrity="sha384-ZvpUoO/+PpLXR1lu4jmpXWu80pZlYUAfxl5NsBMWOEPSjUn/6Z/hRTt8+pR6L4N2"></script>
<script type="text/javascript" src="/assets/materialize.min-0.97.5.js" integrity="sha384-jnt1QI5LA9Z8CEqFV7YvpkT/kvVzzSDZbit0VjFaNiz/XtzoN8OA7z/RI/cbzs95"></script>
<script type="text/javascript" src="/assets/script.js" integrity="sha384-lCnwfMpb5VT9dtF36tAIDhczx0j9zjzzpFY7qIxdwU+0VPRYT7xtOTsVIVkcwbhm"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

I like the idea of having these integrity checksums! I'm not too familiar with that, is there a way these could be generated automatically, for example during the build or during the stage when the assets are embedded into the binary? That would make further updates to the assets easier.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure, but it should be do-able. Mobile browsers complain quit a bit without those is the reason they were added.

Copy link
Author

Choose a reason for hiding this comment

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

Okay I have added a script for the integrity checksums. And mobile safari seems happy. I'm not sure how you want to integrate the script into the build pipeline

templates/register2faOtp.html Show resolved Hide resolved
templates/enter2fa.html Outdated Show resolved Hide resolved
templates/enter2fa.html Outdated Show resolved Hide resolved
templates/register2faOtp.html Outdated Show resolved Hide resolved
@ethantmcgee
Copy link
Author

@m-barthelemy I have reviewed all your comments and made the changes you requested. I've tested this using my local setup and all seems good on both desktop and mobile. There is one comment left unresolved. Please let me know how you want to proceed.

@m-barthelemy
Copy link
Owner

@m-barthelemy I have reviewed all your comments and made the changes you requested. I've tested this using my local setup and all seems good on both desktop and mobile. There is one comment left unresolved. Please let me know how you want to proceed.

Thanks a lot!!
Give me while to go through your work again.

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