miguelgrinberg / Flask-HTTPAuth

Simple extension that provides Basic, Digest and Token HTTP authentication for Flask routes
MIT License
1.27k stars 228 forks source link

Add login_optional decorator #91

Closed bglogic closed 4 years ago

bglogic commented 4 years ago

Adds login_optional decorator described in #90.

miguelgrinberg commented 4 years ago

Considering that this is already supported I'm not sure there is a need to add a decorator that mostly duplicates the code in the original one. How is this better than just using the original login_required and controlling the optional login in the callback?

bglogic commented 4 years ago

Do you mean something like this?

@bp.route('/articles', methods=['GET'])
@login_required(optional=True)
def get_articles():
    ...
miguelgrinberg commented 4 years ago

It's already supported through the callback, which can return True even when the user does not provide credentials. But yes, my main concern is that the code is pretty much an identical copy of the original decorator.

bglogic commented 4 years ago

To implement optional login using existing functionality means contradicting the intent indicated by the method/callback/decorator names in Flask-HTTPAuth's API:

Also required and optional login becomes mutually exclusive or make things more messy to implement.

How about something like this? I'm not sure it will work as is but it shows the idea:

class HTTPAuth(object):
  # ...

  def login_required(self, optional=False):
      def inner_login_required(f):
          @wraps(f)
          def decorated(*args, **kwargs):
              auth = self.get_auth()

              # Flask normally handles OPTIONS requests on its own, but in the
              # case it is configured to forward those to the application, we
              # need to ignore authentication headers and let the request through
              # to avoid unwanted interactions with CORS.
              if request.method != 'OPTIONS':  # pragma: no cover
                  password = self.get_auth_password(auth)
                  is_authenticated = self.authenticate(auth, password)
                  if not optional:
                    if not is_authenticated:
                        # Clear TCP receive buffer of any pending data
                        request.data
                        return self.auth_error_callback()

              return f(*args, **kwargs)
          return decorated
      return inner_login_required

  def login_optional(self, f):
      return self.login_required(optional=True)

  # ...
miguelgrinberg commented 4 years ago

Probably better to do something like this:

class HTTPAuth(object):
    # ...

    def _login(self, optional=False):
        # all the logic here

    def login_required():
        return self._login(optional=False)

    def login_optional():
        return self._login(optional=True)

I expect it isn't going to be as simple to code, but hopefully this serves to show the idea.

bglogic commented 4 years ago

That's definitely better than my suggestion. I'll update the code in this pull request in the coming days.

miguelgrinberg commented 4 years ago

@smmalmansoori note that I just merged a fairly significant change to add support for user roles. This change makes the login_required decorator a bit more complex as it can now work with or without arguments. You will have to adapt this change accordingly.

miguelgrinberg commented 4 years ago

I have been merging a few somewhat related pull requests today, so I also implemented this without code duplication, so I'm closing this PR now. Thanks!