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

Unauthenticated request handling #1907

Merged
merged 14 commits into from
Aug 9, 2021
Merged

Conversation

kjacks
Copy link
Contributor

@kjacks kjacks commented Jul 26, 2021

Require auth for all non-index routes for apps with metadata.auth.auth_required == true.

TODO:

  • add metadata flag to OCCRP Aleph instance metadata settings

`REQUIRE_AUTH` setting is False by default. When enabled it requires
users to be authenticated to access anything other than the base api
endpoints like metadata, statistics, healthz etc
@kjacks
Copy link
Contributor Author

kjacks commented Jul 26, 2021

@sunu I'm having an issue when I try to run this locally with the ALEPH_REQUIRE_AUTH set to true. I'm unable to log in because it seems like it's blocking the login request with a 401

@sunu
Copy link
Contributor

sunu commented Jul 27, 2021

@kjacks Arrh, should be fixed now!

Do you think we can move the message to markdown so that it can be easily customised to be something not OCCRP specific without messing with the code? May be we can also decouple the homepage message from the auth handling part so that it can be used for other messages as well. What do you think?

@sunu sunu requested a review from pudo July 27, 2021 05:32
Copy link
Contributor

@pudo pudo left a comment

Choose a reason for hiding this comment

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

Have you considered whether just improving the rate limiting mechanism and bringing down its limits could be a valid alternative to doing this?

const { location } = ownProps;

return ({
isHomePage: location.pathname === '/',
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to allow the /pages/ stuff, too, no? The content for it is all in /api/2/metadata...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh yes! Thank ya for pointing that out :)



def register_blueprint(app, blueprint, **kw):
if settings.REQUIRE_AUTH:
Copy link
Contributor

Choose a reason for hiding this comment

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

My intuition would be that this wants to live on authz somewhere to make it more coherent with the rest of the system. Maybe like a authz.can_browse_anonymous that gets require'd when no other check is conducted (and is also implicitly checked by can() etc. - analogous to session_write? Doing it at the blueprint layer introduces a whole new dimension to authz that's sort of invisible and unexpected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, in hindsight that's definitely a much cleaner implementation. Thanks @pudo!

@@ -85,6 +85,9 @@
# No authentication. Everyone is admin.
SINGLE_USER = env.to_bool("ALEPH_SINGLE_USER")

# Require authentication. No anonymous access
REQUIRE_AUTH = env.to_bool("ALEPH_REQUIRE_AUTH", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

REQUIRE_LOGGED_IN maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either makes sense

@Rosencrantz
Copy link
Contributor

Have you considered whether just improving the rate limiting mechanism and bringing down its limits could be a valid alternative to doing this?

Yes, it's something that we need to look at. As a first step though, this allows us to shut down potential intrusions quickly. It's a bit of a big hammer and there are certainly finer tools that we should be using but for now this will work for us.

@sunu
Copy link
Contributor

sunu commented Jul 27, 2021

Have you considered whether just improving the rate limiting mechanism and bringing down its limits could be a valid alternative to doing this?

The best I can think of is IP based rate limiting for anonymous requests. The traffic we saw yesterday kept switching between tor exit nodes. So I'm not sure if IP based rate limiting will be effective in that case. Is there any other criteria we can use?

@@ -85,6 +85,9 @@
# No authentication. Everyone is admin.
SINGLE_USER = env.to_bool("ALEPH_SINGLE_USER")

# Require authentication. No anonymous access
REQUIRE_AUTH = env.to_bool("ALEPH_REQUIRE_AUTH", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Either makes sense

@kjacks kjacks changed the title WIP: Unauthenticated request handling Unauthenticated request handling Jul 28, 2021
@Rosencrantz Rosencrantz merged commit 0cb0afe into develop Aug 9, 2021
@kjacks kjacks deleted the kirk/unauthenticated-handling branch January 26, 2022 17:59
tillprochaska added a commit that referenced this pull request Jul 29, 2022
This is another way to display a warning message (only on the home page though). It was introduced in #1907 for a similar purpose (to inform users that anonymous access is temporarily disabled).

As we now have two ways to set an app-wide message banner (via the `APP_BANNER` env variable and by configuring a messages endpoint), I think we can now remove this feature.
tillprochaska added a commit that referenced this pull request Sep 1, 2022
This is another way to display a warning message (only on the home page though). It was introduced in #1907 for a similar purpose (to inform users that anonymous access is temporarily disabled).

As we now have two ways to set an app-wide message banner (via the `APP_BANNER` env variable and by configuring a messages endpoint), I think we can now remove this feature.
Rosencrantz pushed a commit that referenced this pull request Sep 19, 2022
This is another way to display a warning message (only on the home page though). It was introduced in #1907 for a similar purpose (to inform users that anonymous access is temporarily disabled).

As we now have two ways to set an app-wide message banner (via the `APP_BANNER` env variable and by configuring a messages endpoint), I think we can now remove this feature.
tillprochaska added a commit that referenced this pull request Sep 19, 2022
This is another way to display a warning message (only on the home page though). It was introduced in #1907 for a similar purpose (to inform users that anonymous access is temporarily disabled).

As we now have two ways to set an app-wide message banner (via the `APP_BANNER` env variable and by configuring a messages endpoint), I think we can now remove this feature.
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.

4 participants