Conversation
JJ-8
left a comment
There was a problem hiding this comment.
Your code looks good, except for the most critical part: the actual login. I have also no clue how to test this code so it is difficult for me to actually verify that it is working correctly.
api/migrations/57-external-login.sql
Outdated
| ON CONFLICT ("login") DO UPDATE | ||
| SET password = 'external', role = login_with_extern.role |
There was a problem hiding this comment.
If you login with an external identity provider, you will note be able to also do password login? Why can't it be both? So that you set a password after doing a password reset in CTFNote and then you choose to use OAuth login or through your password and you enter the same account.
For example, we can initially set the password to null to prevent any authentication through password and ON CONFLICT here we just do nothing.
There was a problem hiding this comment.
That is definitively an option (after dropping the NOT NULL constraint).
But in that case we would end up with the opposite situation: I have a local account and want to migrate it to external only. With that option, the best I can do is to choose a long, random password and immediately forget it. Since that is less impacting, I probably will change it that way in the upcoming days.
There was a problem hiding this comment.
I made the change.
I also experimented a bit with adding an option to disable local login, but noticed that it is only loosely related to this PR and not a change of a few lines. As a result, it is better split into a separate PR (if I decide to polish it).
This PR adds the possibility to sign in via an OAuth 2.0 compatible identity provider (#171).
The database function
login_with_externand the utils functiondetermineRoleByMappingare designed to hopefully be reusable by other external login providers added in the future.TODO: