-
-
Notifications
You must be signed in to change notification settings - Fork 604
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
Modular auth backend #1162
Modular auth backend #1162
Conversation
Introduce abstract Auth class, BasicAuth providing HTTP Authentication and its dummy subclass WoTTAuth. Move assetdir initialization to a separate function run by Flask.
Let Auth classes specify their config.
Rewrite auth settings update procedure.
Add NoAuth backend ("Disabled").
29b9fae
to
4e93a37
Compare
|
||
class BasicAuth(Auth): | ||
name = 'Basic' | ||
id = 'auth_basic' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, it is not considered a good practice to have anything named by builtins.
def template(self): | ||
return 'auth_basic.html', {'user': self.settings['user']} | ||
|
||
def authenticate(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not exactly doing what is written in the docstring.
Also, seems terms authorize and authenticated are used a bit confusing
http://cdn.differencebetween.net/wp-content/uploads/2017/10/Difference-between-Authentication-and-Authorization.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your statement about terminology. I’ll come up with something better.
However the function does exactly what the docstring says: initiates authentication. Okay, the browser initiates it as a result of getting 401. But if we will have another auth backend which can generate its own login page that’s what this function will return. And that will truly initiate authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But looking at the authorize function above it seems like authenticate is used to always return fail response.
if not self.is_authorized:
return self.authenticate()
self.auth_backends_list = [NoAuth(), BasicAuth(self), WoTTAuth(self)] | ||
self.auth_backends = {} | ||
for b in self.auth_backends_list: | ||
c = b.config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor : b and c are not readable names.
Closed in favor of #1165 |
Closes #1148