fix: make oauth work when baseUrl is set (#1673)

preserve query param when redirecting when baseUrl is set
grafana · Nov 7, 2022 · 6cc1a2a · 6cc1a2a
1 parent 175bf10
commit 6cc1a2a
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 23 deletions.
diff --git a/.github/workflows/cypress-tests.yml b/.github/workflows/cypress-tests.yml
@@ -50,6 +50,11 @@ jobs:
           # keep the server quiet
           PYROSCOPE_LOG_LEVEL: error
           ENABLED_SPIES: none
+      - uses: actions/upload-artifact@v2
+        if: always()
+        with:
+          name: cypress-screenshots
+          path: cypress/screenshots
 
   cypress-tests-auth:
     runs-on: ubuntu-latest
@@ -96,6 +101,11 @@ jobs:
           # keep the server quiet
           PYROSCOPE_LOG_LEVEL: error
           ENABLED_SPIES: none
+      - uses: actions/upload-artifact@v2
+        if: always()
+        with:
+          name: cypress-screenshots
+          path: cypress/screenshots
 
   cypress-tests-base-url:
     runs-on: ubuntu-latest
@@ -142,3 +152,64 @@ jobs:
           # keep the server quiet
           PYROSCOPE_BASE_URL: 'http://localhost:8080/pyroscope'
           PYROSCOPE_LOG_LEVEL: error
+      - uses: actions/upload-artifact@v2
+        if: always()
+        with:
+          name: cypress-screenshots
+          path: cypress/screenshots
+
+  # Test auth when baseUrl is set
+  # We run the same tests from auth
+  # But with a different CYPRESS_BASE_URL
+  cypress-tests-base-url-auth:
+    runs-on: ubuntu-latest
+    env:
+      ENABLED_SPIES: none
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+      - uses: actions/setup-go@v2
+        with:
+          go-version: '^1.19.0'
+      - uses: actions/setup-node@v2
+        with:
+          node-version: '16.18'
+      - name: Cache go mod directories
+        uses: actions/cache@v2
+        with:
+          path: ~/go/pkg/mod
+          key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
+          restore-keys: |
+            ${{ runner.os }}-go-
+      - name: Get yarn cache directory path
+        id: yarn-cache-dir-path
+        run: echo "::set-output name=dir::$(yarn cache dir)"
+
+      - uses: actions/cache@v2
+        id: yarn-cache # use this to check for `cache-hit` (`steps.yarn-cache.outputs.cache-hit != 'true'`)
+        with:
+          path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
+          key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-yarn
+      - run: yarn install --frozen-lockfile
+      - run: make -j e2e-build
+      - name: run nginx with /pyroscope
+        run: docker-compose -f cypress/base-url/base-url-docker-compose.yml up -d
+      - name: Cypress run
+        uses: cypress-io/github-action@v2
+        with:
+          wait-on: http://localhost:8080/pyroscope
+          start: |
+            node ./scripts/oauth-mock/oauth-mock.js
+            make server SERVERPARAMS=--config=scripts/oauth-mock/pyroscope-config-base-url.yml
+          config-file: cypress/integration/auth/cypress.json
+        env:
+          PYROSCOPE_BASE_URL: 'http://localhost:8080/pyroscope'
+          PYROSCOPE_LOG_LEVEL: error
+          CYPRESS_BASE_URL: 'http://localhost:8080/pyroscope'
+      - uses: actions/upload-artifact@v2
+        if: always()
+        with:
+          name: cypress-screenshots
+          path: cypress/screenshots
diff --git a/cypress/integration/auth/cypress.json b/cypress/integration/auth/cypress.json
@@ -4,6 +4,6 @@
   "integrationFolder": "cypress/integration/auth",
   "testFiles": "*.ts",
   "retries": {
-    "runMode": 5
+    "runMode": 1
   }
 }
diff --git a/cypress/integration/auth/oauth.ts b/cypress/integration/auth/oauth.ts
@@ -10,12 +10,18 @@ describe('oauth with mock enabled', () => {
     cy.get('#github-link').should('not.exist');
 
     cy.get('#gitlab-link').click();
-    cy.url().should('contain', '/?query=');
-    // Wait before data load
-    cy.waitForFlamegraphToRender();
-    // cy.get('.spinner-container.loading').should('be.visible');
-    // cy.get('.spinner-container.loading').should('exist');
-    cy.get('.spinner-container').should('exist');
+
+    // When accessing /login directly we should be redirected to the root
+    cy.location().should((loc) => {
+      const removeTrailingSlash = (url: string) => url.replace(/\/+$/, '');
+
+      const basePath = new URL(Cypress.config().baseUrl).pathname;
+
+      expect(removeTrailingSlash(loc.pathname)).to.eq(
+        removeTrailingSlash(basePath)
+      );
+    });
+
     cy.intercept('/api/user');
 
     cy.findByTestId('sidebar-settings').click();
@@ -25,15 +31,4 @@ describe('oauth with mock enabled', () => {
     cy.get('li.pro-menu-item').contains('Sign out').click({ force: true });
     cy.url().should('contain', '/login');
   });
-
-  it('should correctly display forbidden page', () => {
-    cy.visit('/login');
-
-    cy.get('#gitlab-link').should('be.visible');
-
-    cy.get('#gitlab-link').click();
-    cy.url().should('contain', '/forbidden');
-    cy.visit('/logout');
-    cy.url().should('contain', '/login');
-  });
 });
diff --git a/pkg/server/controller.go b/pkg/server/controller.go
@@ -546,10 +546,10 @@ func (ctrl *Controller) redirectPreservingBaseURL(w http.ResponseWriter, r *http
 			// TODO: technically this should never happen because NewController would return an error
 			logrus.Error("base URL is invalid, some redirects might not work as expected")
 		} else {
-			u.Path = filepath.Join(u.Path, urlStr)
-			urlStr = u.String()
+			urlStr = filepath.Join(u.Path, urlStr)
 		}
 	}
+
 	http.Redirect(w, r, urlStr, status)
 }
 

diff --git a/scripts/oauth-mock/oauth-mock.js b/scripts/oauth-mock/oauth-mock.js
@@ -20,8 +20,7 @@ async function main() {
     res.status(200).json([{ path: 'allowed-group-example' }]);
   });
 
-  server.service.once('beforeUserinfo', (userInfoResponse, req) => {
-    console.log('beforeUserinfo');
+  server.service.addListener('beforeUserinfo', (userInfoResponse, req) => {
     userInfoResponse.body = {
       id: 1245,
       email: 'test@test.com',
@@ -35,7 +34,7 @@ async function main() {
   await server.issuer.keys.generate('RS256');
 
   // Start the server
-  await server.start(18080, 'localhost');
+  await server.start(18080, '0.0.0.0');
   console.log('Issuer URL:', server.issuer.url); // -> http://localhost:8080
 
   // Do some work with the server

diff --git a/scripts/oauth-mock/pyroscope-config-base-url.yml b/scripts/oauth-mock/pyroscope-config-base-url.yml
@@ -0,0 +1,22 @@
+---
+base-url: /pyroscope
+no-self-profiling: true
+
+auth:
+  signup-default-role: Admin
+  internal:
+    enabled: true
+
+  gitlab:
+    enabled: true
+    api-url: http://localhost:18080
+    auth-url: http://localhost:18080/authorize
+    token-url: http://localhost:18080/token
+    client-id: 42fe36a3c42334416f36f71049aa5efe
+    client-secret: 16f36f71049aa5efe42fe36a3c423344
+    redirect-url: http://localhost:8080/pyroscope/auth/gitlab/callback
+
+    # allowed-groups:
+    #   - allowed-group-example
+storage-path: tmp/oauth-mock-storage
+log-level: debug
diff --git a/webapp/README.md b/webapp/README.md
@@ -55,3 +55,24 @@ for example `yarn dev --progress=profile`
 
 
 Another interesting flag is `--json`, which you can then analyze on https://chrisbateman.github.io/webpack-visualizer/
+
+# Testing baseURL
+It can be a bit of a pain in the ass.
+
+Install nginx
+```
+nginx -c cypress/base-url/nginx.conf -g 'daemon off;'
+```
+
+Then run the server with `PYROSCOPE_BASE_URL=/pyroscope`
+
+## Testing baseURL + auth
+Same as before, but also run the `oauth2-mock-server`:
+```
+node scripts/oauth-mock/oauth-mock.js
+```
+
+Also run the server with
+```
+make dev SERVERPARAMS=--config=scripts/oauth-mock/pyroscope-config-base-url.yml
+```