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

feat: add ability to use openid-connect flow for other providers #29

Closed
wants to merge 1 commit into from

Conversation

max-wittig
Copy link

Use-case for this PR

We're running a private GitLab server, which uses error reporting with sentry.
Currently it is setup in a way that authorized users need to login to sentry with username and password.
This is a hassle.

Instead of using another OAuth2 plugin for sentry, which is specialized to work only with GitLab, I've decided to use OpenID-Connect, which is a layer on top of OAuth2. This enables the Sentry Administrator to chose an authentication provider without using a new plugin for each provider.
More about the OpenID-Connect spec can be found here: http://openid.net/connect/

The change is fully backwards compatibly with the current Google specific implementation.

OpenID-Connect should also not be confused with OpenID, because the user is not free to chose it's provider with OpenID-Connect. The sentry administrator defines the provider that is used, before even starting sentry itself in the sentry.conf.py. No other provider can be chosen by the user of sentry.

This means that the administrator alone is responsible, as to which provider is considered trustworthy. Changing these variables replaces the installation of another plugin for another provider. It's just a way to standarize OAuth2. Has no security implications or anything.

This explanation is related to the discussion in getsentry/sentry#5650

@codecov-io
Copy link

Codecov Report

Merging #29 into master will decrease coverage by 5.55%.
The diff coverage is 40.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   61.63%   56.08%   -5.56%     
==========================================
  Files           8        8              
  Lines         159      189      +30     
==========================================
+ Hits           98      106       +8     
- Misses         61       83      +22
Impacted Files Coverage Δ
setup.py 0% <ø> (ø) ⬆️
sentry_auth_openid_connect/utils.py 60% <ø> (ø)
sentry_auth_openid_connect/constants.py 100% <100%> (ø)
sentry_auth_openid_connect/__init__.py 100% <100%> (ø)
tests/test_provider.py 100% <100%> (ø) ⬆️
sentry_auth_openid_connect/views.py 20.54% <20.54%> (ø)
sentry_auth_openid_connect/provider.py 64% <46.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 578432d...a9f028b. Read the comment docs.

@max-wittig
Copy link
Author

I'm not sure why coverage changed. Most files were just renamed and slightly modified.

@dcramer
Copy link
Member

dcramer commented Jul 17, 2018

Even if we decide to allow this officially (not sure yet), this needs to go into either sentry-plugins or a repo of its own. I’d suggest a repo of its own that way it’s not reliant on us accepting the PR and you can just copy paste the skeleton of this repo.

@max-wittig
Copy link
Author

@dcramer Thanks for your response and your opinion on this feature.

How should we proceed here? Should I create a separate repo and add you as owner so it can be tranfered to the getsentry organization? I think this feature would be interesting for many.

@dcramer
Copy link
Member

dcramer commented Jul 18, 2018

@max-wittig generally if it lives under the getsentry organization we're expected to maintain it, and I'm not willing to sign us up for that in this case. I would suggest simply creating it based on this repo (or sentry-auth-github), and once its functional you can submit a PR to sentry-docs to add it to the list of "third party extensions".

I took a minute to re-read what I had posted in the previous thread, and I think I still agree with that sentiment. The trust model required of our SaaS service wouldn't allow any provider using this model to ever be trusted. While that doesn't mean we couldn't support it as authentication schemes, the 'email is verified' component can never be true for these systems for sentry.io.

I'm closing this out since you'll want to use a new repository anyways.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants