Make OAuth2 parameters customizable, get OAuth2 to work with salsa.debian.org (gitlab) #3769
Conversation
|
@os-autoinst/tools-team I encouraged phil-hands to create an incomplete draft PR so that we could hopefully provide some useful hints related to actual code or potentially push some work forward ourselves. So, WDYT? |
… later This helps with https://progress.opensuse.org/issues/89020 and the existing PR os-autoinst#3769. Note that this change will not improve support for multiple auth providers. It merely contains the database migration.
… later This helps with https://progress.opensuse.org/issues/89020 and the existing PR os-autoinst#3769. Note that this change will not improve support for multiple auth providers. It merely contains the database migration.
|
Looks like most review comments haven't been addressed yet. (Tell us if you need more information.) |
|
I'm taking over, see https://progress.opensuse.org/issues/90929. I'll keep your commits as-is (besides from rebasing) and will add further changes as separate commits on top of it. |
|
[Hmm ... it seems my reply via mail from Wedneday didn't land here, so I'll paste a slithly edited version in here]
Ah, I thought that I should wait until the thing about adding a database Is that sorted now? If so I'll fix up the code to match and implement the suggestions (or if my lack of action is somehow blocking things, I'll deal with the suggestions straight away). Cheers, Phil. |
|
Yes, the database changes are no in-place. I've already started implementing my own review comments and pushed the WIP state here: https://github.com/Martchus/openQA/tree/oauth However, it is likely easier and more efficient if I just continue with it on Monday myself. It shouldn't take me very long to get this done. If you'd nevertheless like to do the change on yourself, let me know. In this case you might want to pick up the WIP commit from the mentioned branch. Of course it would in any case make sense if you could test whether it actually works using your OAuth server. |
1 similar comment
|
Yes, the database changes are no in-place. I've already started implementing my own review comments and pushed the WIP state here: https://github.com/Martchus/openQA/tree/oauth However, it is likely easier and more efficient if I just continue with it on Monday myself. It shouldn't take me very long to get this done. If you'd nevertheless like to do the change on yourself, let me know. In this case you might want to pick up the WIP commit from the mentioned branch. Of course it would in any case make sense if you could test whether it actually works using your OAuth server. |
Martchus
changed the title
|
I've been pushing my changes here. I have them only tested with GitHub so far. @phil-hands If you like, you can test it with salsa.debian.org. Depending on the coverage report I might need to extend the tests. |
|
The test failure is unrelated, see os-autoinst/os-autoinst#1638 for a fix. |
Codecov Report
@@ Coverage Diff @@
## master #3769 +/- ##
==========================================
+ Coverage 96.68% 96.75% +0.07%
==========================================
Files 368 368
Lines 32638 32683 +45
==========================================
+ Hits 31557 31624 +67
+ Misses 1081 1059 -22
Continue to review full report at Codecov.
|
salsa.debian.org being a gitlab instance, this should also work with gitlab in general if provided with the relevant urls for another instance. As mentioned in the comments below, there must be a better way than the FIXME_... configuration nonsense. Suggestions welcome :-)
Co-authored-by: Martchus <martchus@gmx.net>
* Allow selecting between pre-defined providers 'github' and 'debian_salsa' * Allow configuring custom parameters * Do *not* use attributes within `OAuth2.pm` because we never actually instantiate such an object (`$self` is either the server or a controller object) * Based on WIP state from @phil-hands * See https://progress.opensuse.org/issues/90929
commit 5c4cb4a Merge: 3968afc b124533 Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> AuthorDate: Tue Apr 13 14:42:09 2021 +0000 Commit: GitHub <noreply@github.com> CommitDate: Tue Apr 13 14:42:09 2021 +0000 Merge pull request os-autoinst#3769 from phil-hands/master Make OAuth2 parameters customizable, get OAuth2 to work with salsa.debian.org (gitlab)
In the diff you'll see a load of config hash keys of the form FIXME_oauth2_*
I had rather hoped that I'd be able to put the data contained in them into the $prov_args hash, and thus attach them as extra fields to the provider's defaults, but I'm obviously not quite understanding all the perl5-isms (my perl is quite rusty ;-) )
If someone can have a look at this, and point out how to do this properly, that would be great.
BTW this is a patch arising from my current quest to a) get openqa.debian.net to be more useful, and more widely used, and b) to get an OpenQA package into the Debian package archive at some point soon.
Cheers, Phil.