-
Notifications
You must be signed in to change notification settings - Fork 0
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
mrc-5092 Proof of concept: login to packit from index page #85
Draft
EmmaLRussell
wants to merge
12
commits into
master
Choose a base branch
from
mrc-5092
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains 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
EmmaLRussell
commented
Mar 4, 2024
Comment on lines
+36
to
+37
# TODO: need this on CI? | ||
sleep 30 |
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.
Hack to get working locally for me as db was timing out, shouldn't really be needed!
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.
The changes in this PR, and the related Packit PR are a proof of concept to show how we can support Montagu auth in Packit. These branches should not be merged but will form the basis of further tickets to do a full implementation.
Unlike our approach in OrderlyWeb, where Montagu auth was a special case, and where OrderlyWeb interacted directly with Montagu API to deal with Auth, here we support Basic Auth in Packit, which knows nothing about Montagu. Instead, it's Montagu which will work with Packit's auth requirements. We'll migrate Montagu users into the Packit DB as Packit users, and make the add user admin feature in Montagu create a corresponding new user in Packit. So here the bulk of the work is implementing Basic auth in Packit and integrating Packit into the Montagu proxy, ensuring that login requests to Packit are routed via the Montagu login page, which authenticates with both Montagu and Packit.
I'm sure there are some wrinkles to iron out here - you may have a weird experience with multiple tabs or back button. As @richfitz pointed out, we should also make sure Montagu and Packit tokens are given the same expiry duration....
The changes in these PRs cover the following aspects:
1. SUPPORTING BASIC AUTH IN PACKIT
I've reinstated Lekan's old Basic auth, somewhat changed for the refactored security classes. This uses a JSON POST of credentials, but we should put credentials in the Authorization header when we implement this for real. There isn't actually a user database as yet, just a single hardcoded test user. You can see the implementation in the backend changes in Packit, which includes some refactoring of the user principal classes to support both Basic and OAuth2 users.
2. ADDING PACKIT TO MONTAGU-PROXY DEMO
Packit containers added to Docker compose and added some config wrangling in related scripts.
3. ROUTING PACKIT REQUESTS IN MONTAGU PROXY
nginx.montagu.conf
updated to route requests under/packit
to the Packit front end container, and under/packit/api
to the Packit back end container:Also needed to make a change in Packit, so that the front end router can route to the
/packit
path when required rather than always directly under the domain route. To do this, I added anappRoute
config value, which is passed to the front end as part ofauthConfig
, and made a customPackitBrowserRouter
component which sets thebasename
inBrowserRouter
toappRoute
.4. LOGGING INTO PACKIT WHEN LOG IN TO MONTAGU
The
login
method inmontagu-login.js
, called from the montagu index page, now also logs into Packit, via a newpackit-auth.js
module, which gets the Packit token from the api'sauth/login/basic
endpoint and saves it to local storage, as expected by Packit.5. REDIRECTING FROM PACKIT LOGIN PAGE TO MONTAGU LOGIN PAGE
nginx.montagu.conf
updated to redirect requests to the packit login page to the index page, where the user can login. Sets theredirectTo
qs param, so that the user is automatically redirected to Packit after login with Montagu.What isn't implemented yet is redirecting to any initially requested page within Packit e.g. a Packit group page.
6. REDIRECTING FROM PACKIT PROTECTED ROUTES TO MONTAGU LOGIN PAGE
Usually, the user wouldn't browse direct to
/packit/login
to login, but instead would be directed there by Packit itself when they attempt to access a protected route (e.g the Packit home page). We want users to be redirected to the Montagu page in these cases too, but this required a change in Packit. so that rather than using front-end only react router navigating, it uses ordinary window.location navigation, which round trips to the server and gives the proxy the opportunity to do the redirect. This is implemented inProtectedRoute.tsx
and inAccountHeaderDropdown.tsx
(to cover the case where the user explicitly logs out and is directed to the login page).7. LOGGING OUT FROM MONTAGU PAGE
When the user logs out from the Montagu index page, it also 'logs out' of Packit by calling
deleteUser
inpackit-auth.js
to delete the token from local storage.8. LOGGING OUT FROM PACKIT
When the user logs out explicity from Packit, as well as being directed to the Montagu index page, they should also be logged out of Montagu, so we need some way of telling Montagu that it needs to log out too. So, when Packit's
AccountHeaderDropdown.tsx
redirects with the new window navigation on logout, it also includes aloggingOut=1
query string. This is not used within Packit (which already knows to delete the token), but is picked up by the Montagu index page (the nginx rewrite configuration passes the parameter through) - if theloggingOut
param is present, the index page will log out of Montagu on page load.TESTING
To test locally, run
./scripts/dev.sh
, browse to https://localhost and push through the dire warnings to get to the Montagu index page. I haven't added a button to take you directly to Packit, but once you've logged in (as [email protected]) you should be able to browse to https://localhost/packit to see the Packit front page, logged in as the test user. (You'll also be able to see some packets if you run up the proxy in a folder at the same level as apackit
folder with an initialised outpack demo).You should be able to do all the actions described above e.g. get redirected to Montagu when log out from Packit, get redirected to Montagu if browse to https://localhost/packit when not logged in etc.