fix: allow for auth with activedirectory again #1061
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Current /login is forced to use local auth. This change resolves this regression in a backwards compatible manner while still supporting alternative auth like OIDC. Auth methods compatible with /login are filtered out from the git-proxy config, if there isn't one /login is essentially disabled to avoid throwing. If there is multiple methods it'll prioritize the first. In the future we can support multiple user & password login methods simultaneously with relevant TODOs added.
loadFromGit should be sufficiently authenticated and if it isn't it's not expected to get interactive input. Inform git not to wait for interactive action. This also resolves an issue in the tests where "should throw error if repository is valid URL but not a git repository" test times out waiting for a username.
If we want to ensure this git repository isn't created it should be pointed at the FINOS org, the parent of this repo currently. In theory test-org (an org that currently exists) could add a repo called test-project.
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
|
cc: @jescalada to double check this doesn't impact OIDC & also may be relevant to #1024 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1061 +/- ##
==========================================
+ Coverage 69.51% 69.60% +0.09%
==========================================
Files 54 54
Lines 2227 2244 +17
Branches 248 248
==========================================
+ Hits 1548 1562 +14
- Misses 648 651 +3
Partials 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
06kellyjac
commented
Jun 25, 2025
Comment on lines
+53
to
+61
| (req, res, next) => { | ||
| const authType = getLoginStrategy(); | ||
| if (authType === null) { | ||
| res.status(403).send('Username and Password based Login is not enabled at this time').end(); | ||
| return; | ||
| } | ||
| console.log('going to auth with', authType); | ||
| return passport.authenticate(authType)(req, res, next); | ||
| }, |
There was a problem hiding this comment.
This is a callback/middleware to ensure getLoginStrategy and more importantly getAuthMethods gets the correct proxy config after it's read the user's configuration.
In reality by the time this particular JS file is evaluated the file is already read but this is the more robust method
06kellyjac
commented
Jun 25, 2025
| res.status(403).send('Username and Password based Login is not enabled at this time').end(); | ||
| return; | ||
| } | ||
| console.log('going to auth with', authType); |
There was a problem hiding this comment.
could remove this log, was mostly there for testing
06kellyjac
commented
Jun 25, 2025
Comment on lines
+55
to
+57
| if (authType === null) { | ||
| res.status(403).send('Username and Password based Login is not enabled at this time').end(); | ||
| return; |
There was a problem hiding this comment.
This should account for cases where only OIDC auth is enabled without just calling into passport and throwing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Draft because my commit signing seems to be playing up but should otherwise be ready for review.After actually posting the PR commit signatures look fine.
Main change - allow for auth with activedirectory again
Current /login is forced to use local auth.
This change resolves this regression in a backwards compatible manner while still supporting alternative auth like OIDC.
Auth methods compatible with /login are filtered out from the git-proxy config, if there isn't one /login is essentially disabled to avoid throwing.
If there is multiple methods it'll prioritize the first.
In the future we can support multiple user & password login methods simultaneously with relevant TODOs added.
other fixes: