fix #4106: proprietary oauth #4224

rorour · 2022-03-09T17:43:52Z

This branch is based on 3654-moderate-jupyterhub-user-creations. Please review and merge #4211 first.

e-carlin · 2022-05-30T21:41:48Z

Blocked waiting on flash center to allow our non-tls secured dev endpoint to get a response. Then need to finish https://github.com/radiasoft/sirepo/pull/4224/files#diff-6872e86bbaf9783b9521a2d7bc8efbc06219ec4d6e34d64031682626e3a13ac7R24

e-carlin · 2022-05-31T22:35:23Z

@robnagler this is working. I'd like another review because of the security impact.

One decision we made along the way was to not refresh tokens. Instead just have users log in again once expired. For flash the tokens expire after one hour so this will lead to an annoying ui (need to log in to flash center every hour). Should we implement using refresh tokens or go with just re-login?

robnagler · 2022-05-31T23:05:46Z

Yes implement refresh tokens. It's unfortunately that they didn't set the default to something higher. First find out how many refreshes you get. Also, a refresh only works, I believe, if the user is active. Therefore, even if we implement refreshes, if they close their window and open it again, it might require a relogin

e-carlin · 2022-06-01T17:14:06Z

Sent an email asking for a bump to 24hour expiry. Will implement refresh if the expiry can't be increased.

e-carlin · 2022-06-03T17:35:53Z

Entering invalid credentials and clicking the "deny access" button on the flash login page leads to what looks like an approved authentication response (I would expect the ui to say username/password incorrect or at the very least a response with a payload saying the request was not approved). I've reached out to the flash folks and I'm waiting on a response. This blocks the release.

e-carlin · 2022-06-10T00:18:03Z

@robnagler I think this is ready to merge. Please give it another review.

robnagler

It was easier to edit. I think the code is right. I removed some proprietary information.

etc/run-jupyterhub.sh

robnagler · 2022-06-10T21:21:25Z

sirepo/auth/__init__.py

+    t = sirepo.template.assert_sim_type(sim_type)
+    if t not in sirepo.feature_config.auth_controlled_sim_types():
+        return
+#QUESTION(robnagler) I think this is sufficient, that is, the tests can be reversed


This is the only thing I'm concerned about. LMK what you think.

That doesn't workin in the api_checkAuthJupyterhub case. That API is allow_visitor which means that this check is always True and we return when we should really be prompting to moderate.

Thoughts ccaa8f6 ? I don't love adding another api_perm and I struggled to find a name for it...

sirepo/auth_role_moderation.py

sirepo/auth/__init__.py

…api_perm for it because visitor is a SIM_TYPELESS_PERMS

e-carlin · 2022-06-10T21:45:30Z

setup.py

@@ -12,7 +12,7 @@
    description='accelerator code gui',
    install_requires=[
        'Flask==2.0.3',
-        'SQLAlchemy',
+        'SQLAlchemy>=1.4',


Should we put a test in for why we have to pin versions? Someday we'll want to upgrade and it would be helpful to know why we pinned it in the first place

etc/run-jupyterhub.sh

e-carlin · 2022-06-13T17:26:38Z

sirepo/auth/__init__.py

+    t = sirepo.template.assert_sim_type(sim_type)
+    if t not in sirepo.feature_config.auth_controlled_sim_types():
+        return
+#QUESTION(robnagler) I think this is sufficient, that is, the tests can be reversed


That doesn't workin in the api_checkAuthJupyterhub case. That API is allow_visitor which means that this check is always True and we return when we should really be prompting to moderate.

Thoughts ccaa8f6 ? I don't love adding another api_perm and I struggled to find a name for it...

sirepo/auth_role_moderation.py

sirepo/auth/__init__.py

sirepo/sim_api/jupyterhublogin.py

robnagler · 2022-06-13T22:34:05Z

sirepo/sim_api/jupyterhublogin.py

@@ -33,15 +34,15 @@


 class API(sirepo.api.Base):
-    @sirepo.api_perm.allow_visitor
+    @sirepo.api_perm.manual_permission_check


What I think is going on is that this function is doing too much. It should be @sirepo.api_perm.require_user which will throw an exception, and then the code in sirepo.jupyterhub would do the parsing of the response. It's all inside the sirepo repo. Then you wouldn't have to catch anything in this module. All the exceptions would cause a Response to go back to the sirepo.jupyterhub.authenticate. The fall through case (creating the juptyerhub user) would also do the right thing.

How will sirepo.jupyterhub build the urls? We have access to self.sirepo_uri but all the methods like uri_for_api and local_route access the flask object directly which we can't do because we are outside the flask context.

The schema is available, but not quite. It's what's necessary to map URIs from APIs. For now, these mappings could be hardwired in sirepo.jupyterhub, since we need to get this PR out.

I don't like adding the new API permission. Another approach is to use self.call_api and have a "hidden" api that api_checkAuthJupyterhub calls and wraps in a proper response. One things we could do to fix the SRException mess is to include a header in the response (X-Sirepo-SRException) that contains a serialized version of the exception so the HTML doesn't need to be parsed (which isn't terrible, and could be better encapsulated).

Anyway, I prefer the first approach and to get to work pulling apart the request dependent (external) from the static stuff (schema and args).

Thoughts on adding sirepo_uri? Then we can re-use the code without adding another place where we handle creation of uris

sirepo/auth_role_moderation.py

sirepo/feature_config.py

…esponse

robnagler

See question about sirepo_uri.

sirepo/jupyterhub.py

robnagler · 2022-06-14T22:44:42Z

sirepo/jupyterhub.py

+
+        def _maybe_html_redirect(response):
+            m = re.search(r'window.location = "(.*)"', pkcompat.from_bytes(response.content))
+            m and self._redirect(handler, f'{self.sirepo_uri}{m.group(1)}')


Does there need to be a sirepo_uri? Doesn't the redirect just work on / without http://....?

You're right, it isn't needed. I removed it from everywhere and made the uri building code external=False and to just use '/' in that case (not call into flask) 579a421

robnagler · 2022-06-16T23:36:13Z

@e-carlin I'll let you merge

rorour requested review from robnagler and e-carlin March 9, 2022 17:43

e-carlin mentioned this pull request Mar 9, 2022

Remove merge GitHub into Sirepo Jupyter #3734

Closed

rorour force-pushed the 4106-proprietary-oauth branch from 87ea297 to 632cefb Compare March 9, 2022 22:45

rorour changed the base branch from master to 3654-moderate-jupyterhub-user-creations April 11, 2022 16:38

rorour force-pushed the 4106-proprietary-oauth branch from 632cefb to c0eebbb Compare April 11, 2022 17:43

e-carlin force-pushed the 4106-proprietary-oauth branch 5 times, most recently from 6653108 to 9ad66cf Compare April 12, 2022 14:51

e-carlin force-pushed the 4106-proprietary-oauth branch from 9ad66cf to 20574b9 Compare April 21, 2022 15:35

e-carlin changed the base branch from 3654-moderate-jupyterhub-user-creations to master April 21, 2022 15:38

e-carlin changed the base branch from master to 3654-moderate-jupyterhub-user-creations April 28, 2022 21:04

e-carlin changed the base branch from 3654-moderate-jupyterhub-user-creations to master April 28, 2022 21:04

e-carlin force-pushed the 4106-proprietary-oauth branch from 20574b9 to f3b2cd9 Compare April 28, 2022 21:34

e-carlin changed the base branch from master to 3654-moderate-jupyterhub-user-creations April 28, 2022 21:35

e-carlin changed the base branch from 3654-moderate-jupyterhub-user-creations to master April 28, 2022 21:36

e-carlin force-pushed the 4106-proprietary-oauth branch from f3b2cd9 to ebcd13a Compare April 29, 2022 20:06

e-carlin force-pushed the 4106-proprietary-oauth branch 3 times, most recently from e084efd to 7b63eaa Compare May 30, 2022 21:40

e-carlin force-pushed the 4106-proprietary-oauth branch 2 times, most recently from 135a288 to 60bd851 Compare May 31, 2022 22:32

e-carlin force-pushed the 4106-proprietary-oauth branch from 60bd851 to e96f1e3 Compare June 6, 2022 15:07

e-carlin force-pushed the 4106-proprietary-oauth branch 5 times, most recently from aafc872 to 16129a9 Compare June 10, 2022 00:10

fix #4106: proprietary oauth

62f2668

e-carlin force-pushed the 4106-proprietary-oauth branch from 16129a9 to 62f2668 Compare June 10, 2022 14:59

review comments

572387f

robnagler requested changes Jun 10, 2022

View reviewed changes

e-carlin added 4 commits June 13, 2022 15:35

cleanup

ca2fffe

auth for jupyterhub needs to be checked manually, we need a specific …

ccaa8f6

…api_perm for it because visitor is a SIM_TYPELESS_PERMS

raise to match raise_authorize_redirect and raise_forbidden

4980a3b

check_ better matches our other apis

655f4de

e-carlin reviewed Jun 13, 2022

View reviewed changes

e-carlin requested a review from robnagler June 13, 2022 17:50

robnagler requested changes Jun 13, 2022

View reviewed changes

e-carlin added 3 commits June 13, 2022 22:47

urls are not proprietary; cleanup

7d2f7e0

use "normal" permission checks and have sirepo.jupyterhub parse the r…

890681e

…esponse

cleanup

b824893

e-carlin requested a review from robnagler June 14, 2022 21:37

robnagler reviewed Jun 14, 2022

View reviewed changes

sirepo_uri is not needed - can do local redirect

579a421

e-carlin requested a review from robnagler June 15, 2022 15:19

robnagler approved these changes Jun 16, 2022

View reviewed changes

Merge branch 'master' into 4106-proprietary-oauth

11ab9af

e-carlin merged commit d0b2235 into master Jun 20, 2022

e-carlin deleted the 4106-proprietary-oauth branch June 20, 2022 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #4106: proprietary oauth #4224

fix #4106: proprietary oauth #4224

rorour commented Mar 9, 2022

e-carlin commented May 30, 2022

e-carlin commented May 31, 2022

robnagler commented May 31, 2022

e-carlin commented Jun 1, 2022

e-carlin commented Jun 3, 2022

e-carlin commented Jun 10, 2022

robnagler left a comment

robnagler Jun 10, 2022

e-carlin Jun 13, 2022

e-carlin Jun 10, 2022

e-carlin Jun 13, 2022

robnagler Jun 13, 2022

e-carlin Jun 13, 2022

robnagler Jun 13, 2022

e-carlin Jun 14, 2022

e-carlin Jun 14, 2022

robnagler left a comment

robnagler Jun 14, 2022

e-carlin Jun 15, 2022

robnagler commented Jun 16, 2022

fix #4106: proprietary oauth #4224

fix #4106: proprietary oauth #4224

Conversation

rorour commented Mar 9, 2022

e-carlin commented May 30, 2022

e-carlin commented May 31, 2022

robnagler commented May 31, 2022

e-carlin commented Jun 1, 2022

e-carlin commented Jun 3, 2022

e-carlin commented Jun 10, 2022

robnagler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robnagler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robnagler commented Jun 16, 2022