Ouranosinc / Magpie

AuthN/AuthZ services
https://pavics-magpie.readthedocs.io
Apache License 2.0
1 stars 5 forks source link

Magpie not able to use GitHub as external signin provider #172

Closed tlvu closed 1 year ago

tlvu commented 5 years ago

Github Oauth config:

github-oauth-app-config

Magpie was able to initiate connection to Github:

magpie-able-to-talk-to-github

magpie-able-to-talk-to-github-2

But then "Internal Server Error":

magpie-github-error

Relevant Magpie logs (there is no slash in the user name this time):

INFO  [sqlalchemy.engine.base.Engine] BEGIN (implicit)                                                                                                                      
INFO  [sqlalchemy.engine.base.Engine] SELECT users.status AS users_status, users.security_code_date AS users_security_code_date, users.user_password AS users_user_password,
 users.registered_date AS users_registered_date, users.id AS users_id, users.user_name AS users_user_name, users.email AS users_email, users.last_login_date AS users_last_login_date, users.security_code AS users_security_code                                                                                                                       FROM users, external_identities                                                                                                                                       
WHERE external_identities.external_id = %(external_id_1)s AND external_identities.provider_name = %(provider_name_1)s AND users.id = external_identities.local_user_id
 LIMIT %(param_1)s                                                                                                                                                    
INFO  [sqlalchemy.engine.base.Engine] {'provider_name_1': u'github', 'external_id_1': u'tlvu', 'param_1': 1}                                                                
INFO  [sqlalchemy.engine.base.Engine] SELECT groups.description AS groups_description, groups.group_name AS groups_group_name, groups.id AS groups_id, groups.member_count A
S groups_member_count                                                                                                                                                       FROM groups                               
WHERE groups.group_name = %(group_name_1)s
 LIMIT %(param_1)s                                                           
INFO  [sqlalchemy.engine.base.Engine] {'param_1': 1, 'group_name_1': 'users'}                                                                                               
INFO  [sqlalchemy.engine.base.Engine] SELECT anon_1.users_status AS anon_1_users_status, anon_1.users_security_code_date AS anon_1_users_security_code_date, anon_1.users_us
er_password AS anon_1_users_user_password, anon_1.users_registered_date AS anon_1_users_registered_date, anon_1.users_id AS anon_1_users_id, anon_1.users_user_name AS anon_1_users_user_name, anon_1.users_email AS anon_1_users_email, anon_1.users_last_login_date AS anon_1_users_last_login_date, anon_1.users_security_code AS anon_1_users_security_code, groups_1.description AS groups_1_description, groups_1.group_name AS groups_1_group_name, groups_1.id AS groups_1_id, groups_1.member_count AS groups_1_member_count                                                                                                                                                                           FROM (SELECT users.status AS users_status, users.security_code_date AS users_security_code_date, users.user_password AS users_user_password, users.registered_date AS users_
registered_date, users.id AS users_id, users.user_name AS users_user_name, users.email AS users_email, users.last_login_date AS users_last_login_date, users.security_code AS users_security_code                                                                                                                                                       FROM users                                
WHERE lower(users.user_name) = %(lower_1)s                                                                                                                                  
 LIMIT %(param_1)s) AS anon_1 LEFT OUTER JOIN (users_groups AS users_groups_1 JOIN groups AS groups_1 ON groups_1.id = users_groups_1.group_id) ON anon_1.users_id = users_g
roups_1.user_id                                                                                                                                                             INFO  [sqlalchemy.engine.base.Engine] {'lower_1': u'tlvu_github', 'param_1': 1}                                                                                             
INFO  [sqlalchemy.engine.base.Engine] INSERT INTO users (status, security_code_date, user_password, registered_date, user_name, email, last_login_date, security_code) VALUE
S (%(status)s, %(security_code_date)s, %(user_password)s, %(registered_date)s, %(user_name)s, %(email)s, %(last_login_date)s, %(security_code)s) RETURNING users.id         INFO  [sqlalchemy.engine.base.Engine] {'status': 1, 'security_code_date': datetime.datetime(2000, 1, 1, 0, 0), 'user_password': None, 'registered_date': datetime.datetime($
019, 4, 4, 20, 47, 4, 155622), 'last_login_date': datetime.datetime(2019, 4, 4, 20, 47, 4, 155637), 'security_code': u'default', 'user_name': u'tlvu_github', 'email': None$INFO  [sqlalchemy.engine.base.Engine] ROLLBACK 

Using image pavics/magpie:0.7.3.

fmigneault commented 5 years ago

Authorization callback URL has to be <server>/magpie/providers/github/signin

tlvu commented 5 years ago

@fmigneault I did what you said, still not working (using docker image pavics/magpie:0.7.11 now)

magpie-github-error-again

github-oauth-app-config-2

fmigneault commented 5 years ago

@tlvu Seems like it is still broken in version 0.7.11. I have a server with more recent version 0.9.5 and it is working. Is it possible to update it on your side? Only problem is that it could break some requests coming from other app... Otherwise we would need a hotfix for the 0.7.x suite.

tlvu commented 5 years ago

@fmigneault, @dbyrns, I'll be happy to test version 0.9.5. As for what this upgrade would possibly break, I assume you guys would know this much better than I do. I am guessing it would possibly break the front end UI (platform) and twitcher? All the WPS services, thredds, solr, geoserver should not be impacted at all, is this a reasonable assumption? If you decide to do a hotfix, I can help out with testing.

fmigneault commented 5 years ago

@tlvu Yes, mostly frontend/twitcher. On top of my head, the breaking changes include:

tlvu commented 5 years ago

@fmigneault

I upgraded my magpie to 0.9.5 as you suggested:

magpie-version-0 9 5

Github OAuth config still like this https://github.com/Ouranosinc/Magpie/issues/172#issuecomment-480081256.

Integration with Github is still not working, see screenshot:

magpie-0 9 5-github-still-not-working

Is there a config change between 0.7.3 and 0.9.5 that you forgot to let me know?

Is your magpie 0.9.5 deployment a clean deployment without any custom patches/development hacks?

Browser cache is cleared before testing?

fmigneault commented 5 years ago

@tlvu Have you updated the 2 config values GITHUB_CLIENT_ID and GITHUB_SECRET_ID with corresponding values defined in Github?

tlvu commented 5 years ago

@fmigneault Yes, the values for GITHUB_CLIENT_ID and GITHUB_SECRET_ID were already good before I upgraded magpie to 0.9.5. Is there new config I have to set? (recall it's a magpie upgrade from 0.7.3 to 0.9.5)

fmigneault commented 5 years ago

There shouldn't be other configs than these 2 specifically for Github, but something is definitely not configured correctly (either on Github side or Magpie settings) as I can use this login method on a 0.9.5 magpie instance.

image

tlvu commented 5 years ago

@fmigneault Did some more debugging, it looks like the Location response header returned by Github is different (it redirects to localhost, does that matter?). The following screenshot shows that Location response header comparison between magpie 0.9.5 and mapgie 0.7.3. Again, this is a straight docker image version upgrade from magpie 0.9.5 and mapgie 0.7.3, no other config change, no change to Github oauth config either.

Are you sure there are no other new magpie config for the newer version I have to add to fix that different redirect to localhost? My magpie is behind twitcher pavics/twitcher:pavics-0.3.3 that is not upgraded. Is the different redirect somehow related to twitcher being old and magpie too new?

Are you sure your magpie 0.9.5 do not have any "custom patches/development hacks" applied that made it work that you forgot about? Delete the container and recreate it would clear that.

left-magpie-095-right-magpie-073-github-location-resp-header-diff-edited

I am blocked for my task https://github.com/Ouranosinc/pavics-sdi/issues/58. Is there something I can try or should try or I might waste my time and I should rather simply wait for a fix from your side?

fmigneault commented 5 years ago

@tlvu The incorrect redirect has more chance to be caused by the Location header, Twitcher is not used in this part of the process. I think you might need to set one of MAGPIE_URL, MAGPIE_HOST or HOSTNAME since inside the Docker image, the app only knows localhost:2001. Method get_magpie_url is called to set the Location header with it.

tlvu commented 5 years ago

@fmigneault

Adding MAGPIE_URL probably would have worked if I did not use a self-signed SSL certificate. This was what I repeatedly asked previously if there was a new config added/needed for the magpie upgrade from 0.7.3 to 0.9.5.

Getting this now because I use self-signed SSL certificate:

500 Internal Server Error
The server has either erred or is incapable of performing the requested operation.

SSLError(MaxRetryError("HTTPSConnectionPool(host='jupyterhub.ouranos.ca', port=443): Max retries exceeded with url: /magpie/signin (Caused by SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)'),))",),) 

Just for documentation, this is the patch to add the MAGPIE_URL, is it how it should be?:

diff --git a/config/magpie/magpie.env.template b/config/magpie/magpie.env.template
index a5723b9..783113b 100644
--- a/config/magpie/magpie.env.template
+++ b/config/magpie/magpie.env.template
@@ -6,6 +6,7 @@ ADMIN_USER=admin
 ADMIN_PASSWORD=${MAGPIE_ADMIN_PW}
 USER_GROUP=user
 ANONYMOUS_USER=anonymous
+MAGPIE_URL=https://${PAVICS_FQDN}/magpie
 MAGPIE_PORT=2001
 GITHUB_CLIENT_ID=$GITHUB_OAUTH_CLIENTID_MAGPIE
 GITHUB_CLIENT_SECRET=$GITHUB_OAUTH_CLIENTSECRET_MAGPIE
fmigneault commented 5 years ago

@tlvu

Yes, this patch looks right.

As for the SSL error, you have 2 choices, either disable the checks with ssl_verify=False in the corresponding requests (for debugging, but should not be left as is), or verify that the certificate is valid and update/replace it as needed in the server's config.

Sorry if I missed a config along the way. As you might guess, since 0.7.3, over ½ year has passed with ~20 new versions all playing around the configs and settings as well as variations of Magpie deployed across multiple servers and used for different projects. I cannot remember all those variants on top of my head.

huard commented 5 years ago

@fmigneault I think it would help us if you could add some comments when you do Magpie release. Like which ones are production ready and which are not. Something to let us know if it's alright for us to upgrade to this version.

tlvu commented 5 years ago

@fmigneault

For the SSL error, it might be on Github side, not sure, will try your ssl_verify=False.

As for remembering config difference between different releases, you don't have to, that's what source control is there to help. Here's the diff between 0.7.3 and 0.9.5 https://github.com/Ouranosinc/Magpie/compare/0.7.3...0.9.5 (https://github.com/Ouranosinc/Magpie/compare/0.7.3...0.9.5). In this massive diff, the difficulty is knowing which file to look for. I am guessing it's the file env/magpie.env.example and we do see MAGPIE_URL added in there. Thanks for keeping that file up-to-date !

I used to work for a company providing commercial software and we have to upgrade paying customer. We can not tell our customers it's been half a year with many intermediate releases in between, we have no idea what configs we need to add/modify to upgrade our customers to the latest release version. We rely heavily on source control to provide us with the exact answer.

So tools are there to help us. Let's use the tools to the best of their potential.

fmigneault commented 5 years ago

@huard They are all tested and ready for deployment on each tag, but for integration tests we need what @tlvu is working on. I cannot know in advance if a full functionality workflow gets broken in another bird interacting with Magpie. The issues we are facing now is because no updates has been done for so long and we try to jump 2 versions in one time. This isn't really great for continuous integration.

fmigneault commented 5 years ago

@tlvu I know about diff compares, but I assumed you would have done this minimally to setup basic variables, and your questions where more related to the values of these variables. As said above, we have not updated Magpie for 2 versions on Ouranos' side. I cannot even know which variables you have on your server... so hard for me to tell the diff when these changes would be in your PAVICS's config.

dbyrns commented 5 years ago

I just want you to keep in mind that Francis is working hard on other projects right now and he try to help you the most he can in the little spare time he got, it's not fair to compare his support to a commercial support for paying customers. As for the Magpie releases, as you may know, we are actively using Magpie in other projects as well, so yes, tagged versions are tested and used in other production environments. We are just not ready to support the latest version of Magpie in the PAVICS environment.

huard commented 5 years ago

@fmigneault @dbyrns Sorry if you were offended by our questions and comments, that was not our intent. It's going to be painful and somewhat frustrating for us to catch up, but that's on us, not on you.

Would it make sense for us to add a few unit tests in the magpie test suite to exercise PAVICS-required functionalities ? Or you think our efforts would be better spent moving toward continuous integration ?

dbyrns commented 5 years ago

Your contribution to the test suite regarding specific needs is indeed welcomed. The real deal breaker here is that the platform is no more compatible with the master branch... Integration tests should help us in the future when we will try to re-synch them together and in the meantime to test patches to the version used by PAVICS.

fmigneault commented 5 years ago

@huard Issue #74 is tracking some of the unittests that have yet to be complete. I add some gradually on spare time. Of course, any help is appreciated. If functional tests that validates various birds interacting correctly with Magpie can be added, that's even better!

tomLandry commented 5 years ago

I might be wrong, but I feel like tighter integration oto Birdy could lead to quick wins.

huard commented 5 years ago

@tomLandry Please elaborate. Birdy so far has only provided access to WPS services. Unless I'm wrong, the scope here is larger.

tomLandry commented 5 years ago

I transform these feelings into potential specs, and I'll get back to you.

fmigneault commented 1 year ago

Closing as validated in https://github.com/Ouranosinc/Magpie/pull/570.