Google Sign-On with new libraries #5777
Conversation
templates/login/login_core.html.twig
Outdated
| const auth_response = googleUser.getAuthResponse(); | ||
| const id_token = auth_response.id_token; | ||
|
|
||
| // const auth_response = googleUser.getAuthResponse(); |
There was a problem hiding this comment.
ok to remove this instead of commenting
There was a problem hiding this comment.
@stephenwaite I made changes like you said. Could you review
commit cd882f8 ?
What is the next step?
Thanks
|
hi @Saparbek-Nagashibekov , left some things to fix up on a quick review, haven't tested this though |
templates/login/login_core.html.twig
Outdated
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| {% if displayGoogleSignin %} | ||
| <meta name="google-signin-client_id" content="{{ googleSigninClientID|attr }}"> | ||
| {% endif %} | ||
| <script src="https://accounts.google.com/gsi/client" async defer></script> | ||
| <div id="g_id_onload" | ||
| data-client_id="{{ googleSigninClientID }}" |
There was a problem hiding this comment.
need to html escape this via attr:
data-client_id="{{ googleSigninClientID|attr }}"
There was a problem hiding this comment.
Hi, how to do that ? could you give your suggested change? I will add it. Thanks
There was a problem hiding this comment.
There was a problem hiding this comment.
i should of been more clear. you pipe it through the attr to html escape it there (see line below):
data-client_id="{{ googleSigninClientID|attr }}"
There was a problem hiding this comment.
What does it mean "html escape this via attr" how can I do that? Could you give me sample. thanks
There was a problem hiding this comment.
here is what the line should look like (see where the |attr is in below line :) ):
data-client_id="{{ googleSigninClientID|attr }}"
There was a problem hiding this comment.
Hi Brady,
made changes. please check commit 1d831fc
Thanks
templates/login/login_core.html.twig
Outdated
| <div id="g_id_onload" | ||
| data-client_id="{{ googleSigninClientID }}" | ||
| data-callback="onSignInSuccess" data-auto_prompt="false"> | ||
| </div> |
There was a problem hiding this comment.
shouldn't this block of code be within the above if conditional in line 4?
(ie. only shows if displayGoogleSignin)
templates/login/login_core.html.twig
Outdated
| @@ -1,10 +1,15 @@ | |||
| {% set srOnly = (showLabels == true) ? "" : "sr-only" %} | |||
There was a problem hiding this comment.
above line looks like an unrelated change. If so, would add it back.
|
hi @Saparbek-Nagashibekov , thanks for addressing this bug! Just noted a couple issues. |
|
hi @Saparbek-Nagashibekov , Code looks good. Just let me know this is testing well for you and then will bring it in. thanks! |
|
this is testing well I think a separate issue is that the login process is not checking the edit: or maybe am misunderstanding the flow since it should work without filling in the 2nd edit: it's MFA that interrupts the flow so MFA cannot be activated for google sign in |
|
@kchapple or @growlingflea |
|
ran another test with another email and it successfully finds the user with the google email and signs in without having to enter username/password |
|
Code looks good. Audited code on the server side of things and basically impossible to login unless authenticated (the passed token is confirmed with the stored client_id and google sign in email on the server end), so if the login is working on testing then this is good to bring in. Will bring this in and also bring into rel-700 branch. thanks @Saparbek-Nagashibekov ! |
|
also, thanks @stephenwaite for testing!
|
|
|
Fixes #5776
Short description of what this resolves:
OpenEMR Google Sign-On with new libraries
Changes proposed in this pull request:
Added Google new library changes