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

SSO #1372

Closed
dcramer opened this issue Jan 5, 2015 · 14 comments
Closed

SSO #1372

dcramer opened this issue Jan 5, 2015 · 14 comments

Comments

@dcramer
Copy link
Member

dcramer commented Jan 5, 2015

Implement organization-based SSO.

We're circulating an internal draft and will publish more details as we make decisions.

Changes to the current authentication system:

  • Auth providers will exist as both identities and user providers
  • django-social-auth will be removed for a 1st party solution
  • Users will have many identities (again, an identity could be what they're logging in with)
  • Users can be "managed" therefore not being able to change various properties (basically only Sentry settings would be configurable)

Basic data modeling:

  • An organization may configure a single authentication provider
  • Each authentication provider relies on an implementation which passes config from the provider instance
  • An implementation is something like Google, GitHub, OneLogin, LDAP
  • An implementation generates a pipeline (probably a set of View classes) which are executed in order
    • Note there's a concern with how to handle errors in the above, as we may not know what an error case is, but that's fixable by executing CurrentPipelineView.handle_next_view (or something similar)

Here's an ugly representation of what some of the code layout might look like:

User =>
    [Identities]

Organization =>
    [AuthProviders] (support many but limit to one)

Identity =>
    {User}
    {AuthProvider}
    ident (username/email/primary key of user)
    data

AuthProvider =>
    {Implementation} [Google, OneLogin, LDAP, etc]
    config
    created_by

    # sync is required for some services to ensure that members who have changed
    # in some way are reflected correctly with some delay
    sync_time
    last_sync


Implementation =>
    (Not stored in the DB)
    get_config_form()

    # we need a way to determine request urls for various things
    get_urls()

    # alternatively we could force them to build a pipeline
    get_auth_pipeline() => [OAuthRequest, OAuthConfirm]


Plugin2 =>
    get_auth_implementations() => [Implementation]  
@dcramer
Copy link
Member Author

dcramer commented Jan 5, 2015

@jaysoffian I'd be curious to hear your feedback since you guys have done a bunch around this and ideally we can support what you're doing without any modifications (beyond potentially a custom auth implementation plugin)

@dcramer
Copy link
Member Author

dcramer commented Jan 5, 2015

Some things we should keep in mind when doing this is other peoples successes failures:

Something important to note is allauth takes a URL based approach, and there might be limitations to what we're trying to do w/ the pipeline.

As an alternative we could do a get_urls hook that just returns views, and we could easily namespace those (see nexus for an example)

@dcramer
Copy link
Member Author

dcramer commented Jan 5, 2015

Some initial thoughts on APIs:

class AuthView(View):
    def dispatch(self):
        return self.redirect_to_step(self.get_next_step())

    def dispatch(self):
        return self.dispatch_step(self.get_next_step())

class OAuth2Redirect(AuthView):
    def dispatch(self, request):
        return self.redirect(...)

class OAuth2Finalize(AuthView):
    def dispatch(self, request):
        if 'error' in shit:
            return # do something

        identity = self.create_identity(...)

        # do something else?
        self.authenticate(identity)

class OAuth2Implementation(Implementation):
    def get_auth_pipeline(self):
        return [OAuth2Redirect, OAuth2Finalize]

class RequestEmail(AuthView):
    def dispatch(self, request):
        form = EmailForm(...)
        if form.is_valid():
            self.bind_session(request, 'email', self.cleaned_data['email'])
            return self.dispatch_step(self.get_next_step())
        return self.render(...)

class SamlRedirect(AuthVIew):
    def dispatch(self, request):
        email = self.fetch_session(request, 'email')

class SAMLImplementation(Implementation):
    def get_auth_pipeline(self):
        return [RequestEmail, SamlRedirect, SamlFinalize]

@dcramer
Copy link
Member Author

dcramer commented Jan 5, 2015

Working branch is sso. Pushed some initial draft API up. Will focus on drafts for the major providers to ensure the API is capable.

@remik
Copy link

remik commented Jan 5, 2015

django-social-auth is renamed to python-social-auth and it has very good support for pipelines.

@dcramer
Copy link
Member Author

dcramer commented Jan 5, 2015

@remik I think we're definitely going with a pipeline approach as it seems flexible enough to support all cases and is easy to implement.

To be clear though our pipelines are per provider whereas social-auth's are for controlled what happens at a global level (afaict)

@dcramer
Copy link
Member Author

dcramer commented Jan 6, 2015

Here's a basic prototype of Google (OAuth2):

https://gist.github.com/dcramer/35dd269c17ddac11274a#file-google_oauth2-py-L65

I'm not sure I'm happy with it yet.

We'll obviously need to make a few minor additions to support refresh tokens, but its a minor issue and IMO due to OAuth2 being so simple we likely wont bring in third party libs since they just add complexity/additional abstractions that can break.

@dcramer
Copy link
Member Author

dcramer commented Jan 6, 2015

My biggest issue right now is how we treat the step flow control, which honestly probably won't even work.

We need a clean way to handle these situations:

  • I need to redirect you (HttpResponse)
  • I need to show you a template (HttpResponse)
  • There is an error (more or less an HttpResponse)
  • Things worked out well, please do whatever you think is right

I don't like returning different data types (in fact, I won't allow that), so the alternative is that we return some kind of tuple (success, [response]), but that's a bit obscure and to me always feels unpythonic. The general alternative to this is using exceptions as flow control (i.e. raise Authenticated) but that's super shitty.

Another alternative would be to do something like return self.the_kind_of_action(), but that has scoping issues as we need to bind the implementation instance in this area, and we need the outer controller (which realistically is where the the_kind_of_action would come from). Using the previous example we could also pass something like 'provider' or 'implementation' to the view methods. This could work but then the Implementation would need control methods for the auth views (it might be ok).

@dcramer
Copy link
Member Author

dcramer commented Jan 6, 2015

I think this might actually be not completely shitty and reasonable:

class Authview:
    def dispatch(request, provider):
        return HttpResponse()

    def dispatch(request, provider):
        # a step that needs to redirect
        return self.redirect(self.next_step_url())

    def dispatch(request, provider):
        # a random step
        return self.next_step(request, provider)

    def dispatch(request, provider):
        # the last step
        self.bind_session(request, 'data', user_data)
        return self.next_step(request, provider)

    def dispatch(request, provider):
        # a step which errored
        return self.error(request, provider, message)

provider would probably be an instance of Implementation (which we might just rename to Provider)

@jaysoffian
Copy link
Contributor

Sorry for the slow reply. From my perspective, auth is handled by a web server in-front of Sentry. The web-server sends an HTTP header with the username. I have a shim in django to check for that header, create the user if necessary, backfilling certain user attributes from LDAP (real name, email). Within Sentry, I disable all of its user management (create new account, alter username, email, real name).

I probably won't have a chance to experiment with what you've done here before you are able to land it, so I'll just submit patches if need be after the fact. :-)

@pitbulk
Copy link

pitbulk commented Jan 14, 2015

I think the best approach is to adapt Sentry to work with python-social-auth.

Later in order to add SAML support we need to integrate python-saml and create a new SAML Auth provider.

@dcramer
Copy link
Member Author

dcramer commented Jan 14, 2015

@pitbulk we already did that. It's too restrictive/buggy/hard to fix. I even have a branch where we vendor an older django-social-auth so we can fix some core issues, but its just not worth the effort.

Basically we implemented almost all we needed for the base auth framework in <300 loc and its much easier to maintain. It also guarantees functionality with all working cases we have (we have several, and most of them aren't currently supported by social-auth).

As an example we need:

  • To not store pickled data in sessions
  • Saml (onelogin, etc)
  • Heroku, JWT
  • Google, GitHub
  • Full synchronization of user accounts to ensure expiration

I have no desire to build such straight forward APIs on a complex backend that tries to cater to every use case there is. It adds way too much complexity and just makes it harder for people to understand.

It's also important to note that we dont care about 99% of the social auth backends, so reusability is minimal. We're going to be dropping some things like Twitter/Facebook auth as they simply dont make sense for Sentry as a product.

For what its worth, there could be a point we pull the Sentry auth system out of Sentry as it's super generic and thin, but reusability isnt a priority yet.

@pitbulk
Copy link

pitbulk commented Jan 14, 2015

@dcramer, ok I underestand.

In the python-saml library you have 2 examples of how add SAML support to a Django app and a Flask app

The normal case of the python-saml library is to be used 1 SP - 1 IdP, but is not hard to make it work 1SP - multiple IdPs.

If I can help you with the library, let me know. You can open an issue in the repo.

@dcramer
Copy link
Member Author

dcramer commented Feb 25, 2015

v0 of this is in master, closing this out so we can follow up with a new ticket

@dcramer dcramer closed this as completed Feb 25, 2015
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants