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

Add default-off rule that denies login for maintenance #175

Closed
wants to merge 1 commit into from

Conversation

gdestuynder
Copy link
Contributor

@gdestuynder gdestuynder commented Feb 15, 2018

Add default-off rule that denies login for maintenance. Order is 999.
Only use this when required as it forbids ALL logins.

NOTE: This introduce a new message 'maintenancemode' which needs to also
be present on the sso dashboard (it works without, but looks prettier
with)

Fixes #174

Add default-off rule that denies login for maintenance. Order is 999.
Only use this when required as it forbids ALL logins.

NOTE: This introduce a new message 'maintenancemode' which needs to also
be present on the sso dashboard (it works without, but looks prettier
with)
Copy link
Contributor

@gene1wood gene1wood left a comment

Choose a reason for hiding this comment

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

If this enabled = false will it not actually be in play?

@gdestuynder
Copy link
Contributor Author

gdestuynder commented Feb 22, 2018

correct, enabled = true is required to enable (or flip it in the dashboard)
otherwise, it will always deny so that'd be a problem

so when needed, you flip it on, do your stuff, flip it off (or will be flipped off by CI after rules have been merged, and thus it does not need to be active anyway)

@gene1wood gene1wood changed the title Fix #174 Add default-off rule that denies login for maintenance Feb 22, 2018
@gene1wood
Copy link
Contributor

What do you think about moving more to how firewall rules work where a positive indicator is needed to pass through the rules. So for example, right now where we check to see if the client_id is one that we know the auth0 rules need to do authorization checks for (because the RP doesn't have the capability), what if in that client_id check, when we find that the client_id does not need our help, we set a "yes, this user should be allowed to login flag which gets us out of the rule processing flow. This way, just like with firewall rules, we can leave the 999 rule enabled and then when we make a mistake in the rules (related to their order, a bug in a rule, etc) the default behavior is not to allow the user to login, but instead to bar the user from logging in.

@gdestuynder
Copy link
Contributor Author

while this sounds good in theory, we're limited to how auth0 is implemented and I would be careful when implementing this as there is no state guaranteed for volatile data between rules.
ie, auth0 is not currently made for this - and the ways I can think of to implement might break unknowingly with updates - which would fail safe (forbid login) but also be terrible from an availability stand point

f.e.:

  • we can try to create context.login_allowed and hope this keeps working
  • Another possibility is to create app_metadata.login_allowed and delete it at the 999 rule, and similarly hope this keep working (there chances this shows in the user profile but maybe that's fine)

The advantage of the current rule is that its not state dependent basically.

I propose we just ask Auth0 what they think of this and what their recommendation is

@gdestuynder
Copy link
Contributor Author

gdestuynder commented Mar 28, 2018

Bundling this in #183

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.

2 participants