grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

New feature: configurable login methods #16

Closed vladimir-mencl-eresearch closed 8 years ago

vladimir-mencl-eresearch commented 8 years ago

Hi Zenon,

I hope this isn't coming too soon after closing #15 :-)

I've already had this code ready and was waiting on the Django 1.8 upgrade to complete before sending this in...

This is the promised follow-up to #9 - making it easy to configure the login methods to be available at /manage without having to modify anything outside local_settings.py.

How is this looking to you? Would you be happy to have this included in the main code base? Any suggestions for change?

I look forward to hearing from you.

Cheers, Vlad

zmousm commented 8 years ago

Hi Vladimir,

it looks good! I want to take a closer look with a colleague and then we need to think how to fit local login in the registration workflow (whether we just have to add a form or need more profound changes).

Give us a few days to test this before we come back to it.

vladimir-mencl-eresearch commented 8 years ago

Hi Zenon,

Thanks for the feedback!

Regarding the local login: I've been so far creating the accounts manually from the admin interface (/admin/). (And was wondering whether to add that somewhere into the documentation).

Are you thinking about self-service registration? (With admin approval?) Any pointers for that? (How to link to the registration form?)

Cheers, Vlad

zmousm commented 8 years ago

Hi Vladimir,

yes I am thinking about self-service registration, with moderation, which is sort of what we do for other authentication methods as well. However some tweaking would be involved as we have previously redirected parts of the self-service activation workflow from django-registration towards admin moderation, but now we'd need both (user activation and then admin moderation). GRNET has dealt with this sort of issue in other apps that rely exclusively on local login (for example ganetimgr), so it may be an easy fix after all. If not, we may proceed without self-registration for now and deal with it later.

We need a few days to look into this and also to have the chance to properly test this patch.

vladimir-mencl-eresearch commented 8 years ago

Hi Zenon,

Thanks for these pointers!

I've now looked at the documentation for django-registration ... and that looked easy to use.

However, after your prompt, I also looked at how the DjNRO code base overrides this functionality - and implementing user account self-registration in a way that does not clash with how RegistrationProfile is used in the existing approval workflow might be rather tricky - and at this point, beyond the scope of this PR focused on configurable login methods (where local login is just one option).

If you are fine with it, I'd leave it with manually created accounts for now (and perhaps add that to documentation somewhere?)

I look forward to receiving your feedback on this PR.

Cheers, Vlad

vladimir-mencl-eresearch commented 8 years ago

Hi Zenon,

Just wondering whether you've had a chance to look into this PR.

I'll be going on leave for three weeks Friday night - so it would be great if we could finish this PR this week.

Cheers, Vlad

zmousm commented 8 years ago

Hi Vladimir,

sorry for the delay. We also had some holidays and traveling that added some further delay. I will make sure we move forward with testing and merging this in the next couple of days and certainly before this Friday.

vladimir-mencl-eresearch commented 8 years ago

Thanks for getting back to me - I look forward to hearing from you. (And in the meantime, I've also pushed #17 for a minor branding fix to remove hard-coded references to GRNET).

zmousm commented 8 years ago

Hi Vladimir,

besides the line note, I am ready to merge this (without registration for local login).

However I am a bit skeptical about bulk enabling by default all such login methods. Not so much about PSA providers (such as Yahoo), as for

As for the django-registration issues, after discussion with @sergafts, we can handle them in such a way:

Let me note however that in the longer term I am inclined to say we should probably move towards registration by invitation, rather than admin moderation.

vladimir-mencl-eresearch commented 8 years ago

Hi Zenon,

The entries that have enabled: False are equivalent to being commented out - so are just configuration snippets the user may decide to enable. And I have intentionally left the local login with enabled: False.

I did leave Shibboleth enabled to match the current defaults on the manage login page. Happy to change to enabled: False if you prefer so.

Registration: I wasn't sure about how much functionality of the registration flow was overridden in the use for account activation - sounds good to have accounts created via invites if someone takes it on.

Please let me know whether happy to merge this PR (or change Shibboleth to False?)

Cheers, Vlad

zmousm commented 8 years ago

Alright, merging. I will disable shib backend by default afterwards.