Skip to content
This repository has been archived by the owner on Jan 4, 2023. It is now read-only.

feat: enable oidc authz code flow #317

Merged
merged 3 commits into from
Apr 15, 2021
Merged

feat: enable oidc authz code flow #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e054233
feat(auth): enable oidc authz code flow
vmttn Apr 12, 2021
2254216
feat(docker): Add oidc config
vmttn Apr 12, 2021
782baf4
chore(oidc): source id token expiration time
vmttn Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .template.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,19 @@ POSTGRES_USER=river
POSTGRES_PASSWORD=river
POSTGRES_HOST=localhost
POSTGRES_PORT=5432

###
### Mozilla OpenID Connect
###

OIDC_RP_CLIENT_ID=
OIDC_RP_CLIENT_SECRET=
OIDC_RP_EXTRA_SCOPES=
OIDC_RP_SIGN_ALGO=
# OIDC_OP_JWKS_ENDPOINT=
LOGIN_REDIRECT_URL=
LOGIN_REDIRECT_URL_FAILURE=
LOGOUT_REDIRECT_URL=
OIDC_OP_AUTHORIZATION_ENDPOINT=
OIDC_OP_TOKEN_ENDPOINT=
OIDC_OP_USER_ENDPOINT=
33 changes: 33 additions & 0 deletions django/river/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"rest_framework",
"corsheaders",
"django_filters",
"mozilla_django_oidc",
# 1st parties
"core",
"extractor",
Expand All @@ -65,6 +66,8 @@
"django.middleware.common.CommonMiddleware",
"django.middleware.csrf.CsrfViewMiddleware",
"django.contrib.auth.middleware.AuthenticationMiddleware",
# Redirect requests to silently re-authenticated:
"mozilla_django_oidc.middleware.SessionRefresh",
Copy link
Member

Choose a reason for hiding this comment

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

Did you read the code ? From memory it performs a redirect or in our case (when using an SPA) returns a 403 along with a refresh url in the body. It's not silent. That's why I stored the refresh token and implemented a custom middleware based on this one. There's a couple of issues and PR related to this in the mozilla-django-oidc repo.

Copy link
Contributor Author

@vmttn vmttn Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Are you're suggesting there is a bug with SPAs ? Point me towards the issues you're referring to.

I did read the code.

It seems to me that what you're describing is not related to mozilla-django-oidc. Rather an issue with the provider implementation when receiving prompt=none. It either can be the expected outcome (the end-user do need to reauthenticate) or an issue with the provider implementation.

If you want to review this PR, you should also check arkhn/o-provider.

I did have an issue with django-oauth-toolkit, but was able to move past with the following monkeypatch (I can explain what a monkeypatch is if you don't know the pattern):
https://github.com/arkhn/o-provider/blob/master/django/oauth/__init__.py

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry my comment wasn't very clear. The request to the provider works flawlessly. But the issue is when using an SPA like our React app. When we will perform a redirect, it means we need to store the client-side state before the redirect and restore the state afterwards.

For instance, if the user is filling a form. When submitting this form, he will receive a 403 (because he reached the 15 minutes you set in the settings) and then the app will redirect him to the redirect_url which will redirect him back to the app. To make it silent, we need to store the user state before the redirect in some ways in the app and rollback this state after the redirect. In our example, it means restoring the form inputs and submitting again. It's a bit cumbersome.

What we can do alternatively is using the refresh token. No redirect is needed in this case. That's what I implemented on my other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay ! This is the kind of insightful feedback I was expecting from you !

  1. The SessionRefresh middleware is only used for GET requests (https://github.com/mozilla/mozilla-django-oidc/blob/HEAD/mozilla_django_oidc/middleware.py#L114). Therefore I don't think there should not be a redirection when submitting a form. No ?
  2. Still. Maybe there is a dropdown on the form that makes an ajax request on click. We could fetch the values when the form is first rendered or use what's in the store.
  3. 15 min refresh is a very strict setting. I think we can increase that value significantly, hence preventing the risk of 2 from happening.

Copy link
Member

@tevariou tevariou Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Totally forgot about that! you're right!
  2. The form example is a bit off. Let's take another one. Take a component with some local state within it, It could have a modal which will fetch some data to display that open if we click on a button.. when the user comes back the state will be reinitialised. And we can't put everything in the redux store. And for persisting the state in the store, we would need something like redux-persist. Or we could use an hidden iframe!
  3. Why not! But I may have missed something but it looks way more convenient to me to just use the refresh token. I looked a bit into it (not much though haha so I may be wrong) and the case where it's not recommended is for SPA without a backend since you can't store the refresh token safely in a browser.

Copy link
Contributor Author

@vmttn vmttn Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. silent reauth is the go-to (in the spec and almost all resources I've crossed)
    a. mozilla infosec does not recommend using the refresh token at all (and they just implement the authz code flow).
    b. auth0 recommends using refresh token (with rotation) (ref), but they do so in the context of a SPA without a backend (ref). They previously recommended silent reauth but changed their recommendation because :

Recent developments in browser privacy technology, such as Intelligent Tracking Prevention (ITP) prevent access to the Auth0 session cookie, thereby requiring users to reauthenticate.
[...]
Refresh token rotation offers a remediation to end-user sessions being lost due to side-effects of browser privacy mechanisms. Because refresh token rotation does not rely on access to the Auth0 session cookie, it is not affected by ITP or similar mechanisms.

In short, Auth0 is a centralized session management system and ITP has forced them to recommend refresh token (with rotation) to their users.

c. Refresh token are dangerous because they never expire. Using them securely implies using rotation among other things (cf here and here). Without these, poorly implemented 3rd party clients could leak refresh token that never expire.

  1. Do you have a concrete example where it could be a problem with pyrog ? I understand there is a world in which it could happen, but the way I see it :
    a. meaningful data, that should not be lost (e.g. user work), should be progressively (but coherently, as units of user work) saved with the backend. Otherwise it might be a UX problem.
    b. for other non meaningful data, we should not bother
    c. given 3. the chances of the issue could be low
    d. at worst, we have the quick fallback: https://mozilla-django-oidc.readthedocs.io/en/stable/xhr.html

To sum up 2., (severity of the problem occuring) * (probability of the problem occuring) is low and to me it does not justify to preemptively implement something.

Maybe you disagree. For now, let's just see how it goes. If it become truly an issue, I'd be happy to review your implementation.

Copy link
Member

@tevariou tevariou Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

a. I didn't talk about losing any meaningful data. I'm just saying it won't be silent for the user if we have any local state in our components and that's not like it's not a common thing. For a concrete example, take the drawer in my last PR on Pyrog. If I don't use redux-persist (which stores the state in local storage), that will just close it without warning (there's three steps, each one potentially fetches something). And yes I think we should bother because it just breaks the UX.

c. is not true. You can set an expiry on refresh token as you do on access token.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this debate needs arbitration lol. I think both of you are more knowledgeable than me on this topic but the way I see it:

(severity of the problem occuring) * (probability of the problem occuring) is low

is true if the token expiration time is long enough. I think users will be bothered by this at some point, and this is when we'll think of something to patch it (like using refresh token for instance). Central authentication is already a hell of a mess, so let's take it step by step 😉.
@tevariou let's not take any action in the webapp to prevent this and see how problematic it becomes for users.

Copy link
Contributor Author

@vmttn vmttn Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@tevariou

  • Silent reauth only means no prompt for credentials. It's not unvisible reauth ;)
  • If the UX is your concern, we can add a loader with "reauth..." before redirecting. Or we could open the redirect_url in an iframe to preserve the app state or idk use a popup.

c. is not true. You can set an expiry on refresh token as you do on access token.

  • Granted. I rephrase : the refresh token CAN expire, depending on the provider implementation and configuration. We won't always have control on this.

"django.contrib.messages.middleware.MessageMiddleware",
"django.middleware.clickjacking.XFrameOptionsMiddleware",
]
Expand Down Expand Up @@ -114,6 +117,7 @@
# https://docs.djangoproject.com/en/3.1/topics/auth/customizing/#specifying-authentication-backends

AUTHENTICATION_BACKENDS = [
"mozilla_django_oidc.auth.OIDCAuthenticationBackend",
simonvadee marked this conversation as resolved.
Show resolved Hide resolved
"django.contrib.auth.backends.ModelBackend",
]

Expand Down Expand Up @@ -247,3 +251,32 @@
# Prometheus

EXPORTER_PORT = os.environ.get("EXPORTER_PORT", 8001)

# Mozilla OpenID Connect
# https://mozilla-django-oidc.readthedocs.io/en/stable/settings.html

OIDC_STORE_ID_TOKEN = True
OIDC_STORE_ACCESS_TOKEN = True
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why storing the access token which is short lived ? And what about the refresh token ?

Copy link
Contributor Author

@vmttn vmttn Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if/why you think there is an issue.

The access token has an expiration time (expires_in field in a successful auth response). It can be an hour, it can be 12h. It's not a one shot token. Given the requests/response pattern of web frameworks (the framework will forget you in between requests) and the fact that we don't want to send the access token in the front (authz code flow), we need to store the access token in the session (i.e. in DB).

Maybe you're asking me : why even bother storing the access token ?

  1. asking a new access token for each requests will add latency and resources consumption. The provider will become more of a single point of failure.
  2. IIRC access tokens have an expiration time bc it's otherwise hard to revoke them. Applications that still need an even longer access can use a refresh token. The refresh token mechanism forces a token bearer to check in with the provider, so that the provider can deny them access. Do you think our apps need to store the token longer than 12h ?
  3. access token are not one-shot tokens, according to the spec. It goes against the spirit of the spec.
  4. maybe thats why mozilla-django-oidc doesnt support storing refresh token.

Copy link
Member

@tevariou tevariou Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. I don't understand this. It's not what's happening with mozilla oidc if you don't store the access token. It won't try to get one at each request according to the code I've read. Or you can point me to where the access token is actually used after the session is created because I don't see it.
  2. I agree with your point but I also think a refresh token can be useful in other ways (see my other comment)
  3. Agreed
  4. As they say in their readme, they keep it as minimal as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding 1., you're also right. I was just saying that : not storing the access token implies that we'll need to make sure to fetch a new one each time a view requires access to a resource server. This has indeed nothing to do with mozilla-oidc. If many views need access to external resources, then we'd better take advantage of storing the access token and reuse it.

# Silently re-authenticated after following time:
OIDC_RENEW_ID_TOKEN_EXPIRY_SECONDS = int(os.environ.get("OIDC_RENEW_ID_TOKEN_EXPIRY_SECONDS", 12 * 60 * 60))

# Relying party
OIDC_RP_CLIENT_ID = os.environ.get("OIDC_RP_CLIENT_ID")
OIDC_RP_CLIENT_SECRET = os.environ.get("OIDC_RP_CLIENT_SECRET")
OIDC_RP_EXTRA_SCOPES = os.environ.get("OIDC_RP_EXTRA_SCOPES", "").replace(",", " ").split(" ")
OIDC_RP_SCOPES = " ".join(["openid", *OIDC_RP_EXTRA_SCOPES])
OIDC_RP_SIGN_ALGO = os.environ.get("OIDC_RP_SIGN_ALGO")

LOGIN_REDIRECT_URL = os.environ.get("LOGIN_REDIRECT_URL")
LOGIN_REDIRECT_URL_FAILURE = os.environ.get("LOGIN_REDIRECT_URL_FAILURE")
LOGOUT_REDIRECT_URL = os.environ.get("LOGOUT_REDIRECT_URL")

# Provider
OIDC_OP_AUTHORIZATION_ENDPOINT = os.environ.get("OIDC_OP_AUTHORIZATION_ENDPOINT")
OIDC_OP_TOKEN_ENDPOINT = os.environ.get("OIDC_OP_TOKEN_ENDPOINT")
OIDC_OP_USER_ENDPOINT = os.environ.get("OIDC_OP_USER_ENDPOINT")

if OIDC_RP_SIGN_ALGO == "RS256":
OIDC_OP_JWKS_ENDPOINT = os.environ.get("OIDC_OP_JWKS_ENDPOINT")
elif OIDC_RP_SIGN_ALGO == "HS256":
pass
1 change: 1 addition & 0 deletions django/river/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
path("", include("pagai.urls")),
path("", include("pyrog.urls")),
path("", include("users.urls")),
path("oidc/", include("mozilla_django_oidc.urls")),
]

if settings.ADMIN_ENABLED:
Expand Down
13 changes: 13 additions & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,19 @@ services:
- DJANGO_SUPERUSER_USERNAME=admin
- [email protected]
- DJANGO_SUPERUSER_PASSWORD=admin
- OIDC_RP_CLIENT_ID=
- OIDC_RP_CLIENT_SECRET=
- OIDC_RP_EXTRA_SCOPES=email
- OIDC_RP_SIGN_ALGO=HS256
- LOGIN_REDIRECT_URL=http://localhost:3000/
- LOGIN_REDIRECT_URL_FAILURE=http://localhost:3000/error/
- LOGOUT_REDIRECT_URL=http://localhost:3000/
# LPT: Always use 127.0.0.1 (not localhost) in public URLs redirecting to the provider
# This is to prevent cookies in the browser from being shared and overridden by
# different services running on the host.
- OIDC_OP_AUTHORIZATION_ENDPOINT=http://127.0.0.1:8001/o/authorize/
- OIDC_OP_TOKEN_ENDPOINT=http://127.0.0.1:8001/o/token/
- OIDC_OP_USER_ENDPOINT=http://127.0.0.1:8001/o/userinfo/

zookeeper:
image: zookeeper:3.4.10
Expand Down
1 change: 1 addition & 0 deletions requirements/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ fhir.resources @ git+https://github.com/arkhn/[email protected]#egg=fhir.reso
fluent-logger==0.9.6
hiredis==1.1.0
jsonschema==3.0.2
mozilla-django-oidc==1.2.4
msgpack==0.6.2
prometheus-client==0.8.0
pyodbc==4.0.30
Expand Down