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

RAS Authentication Updates #1864

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

RAS Authentication Updates #1864

wants to merge 45 commits into from

Conversation

utku-ozturk
Copy link
Member

@utku-ozturk utku-ozturk commented Oct 31, 2023

Notes:

Related PRs:

  1. RAS Authentication Updates utils#291
  2. RAS Authentication Updates snovault#273
  3. RAS Authentication Updates shared-portal-components#238

@utku-ozturk utku-ozturk marked this pull request as ready for review November 17, 2023 21:45
Comment on lines +157 to +162
if 'kty' not in jwk:
raise ValueError("JWK must have a 'kty' field")
kty = jwk['kty']

if kty == 'RSA':
if 'n' not in jwk or 'e' not in jwk:
Copy link
Member

Choose a reason for hiding this comment

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

Might be useful to give an example here, maybe move this helper to snovault as well so it can be re-used in smaht-portal eventually

Copy link
Member Author

Choose a reason for hiding this comment

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

@willronchetti I added more description, sample into docstring and TODO for possible move to snovault.

src/encoded/__init__.py Outdated Show resolved Hide resolved
else:
# RAS
scope = 'openid profile email ga4gh_passport_v1'
allowed_conn = ['google-oauth2']
Copy link
Member

Choose a reason for hiding this comment

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

This I think is unused in RAS right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, scope is still used, but allowed_conn is unnecessary. Btw, simplified it for clarity: ce2a53a

pytestmark = [pytest.mark.setone, pytest.mark.working]


class TestRedisSession:
Copy link
Member

Choose a reason for hiding this comment

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

I should add some more to test here

generate_user,
NamespacedAuthenticationPolicy,
session_properties
import base64
Copy link
Member

Choose a reason for hiding this comment

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

What changes in this file are actually necessary vs. what is handled in snovault? I think most of this should be removed right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@willronchetti this commit was already in redis_311. let me check it, if it became stale I'd remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

@willronchetti cleared the duplicate (and unnecessary) stuff and redeployed it into mastertest/hotseat. They're still running well.

pyproject.toml Outdated
@@ -97,7 +97,7 @@ simplejson = "^3.17.0"
SPARQLWrapper = "^1.8.5"
SQLAlchemy = "1.4.41" # latest stable version we can take - Will Jan 13 2023
structlog = ">=19.2.0,<20"
submit4dn = "0.9.7"
submit4dn = "0.9.7" # XXX: this version is likely not 3.11 compliant but will allow locking for now
Copy link
Member

Choose a reason for hiding this comment

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

Does fourfront even use submit4dn? Maybe this dependency should be removed or if it is used checked and updated as we are now on v4.0.0 in pypi.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@willronchetti @aschroed rebuilding FF without submit4dn worked. I have updated mastertest and hotseat.

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.

3 participants