Skip to content
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

DCJ-654: Sign-in via OIDC #2667

Merged
merged 32 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
db33dbc
feat: sign-in via OIDC
rushtong Sep 9, 2024
386d0f2
feat: re-add register metrics
rushtong Sep 9, 2024
456cb7f
feat: rm unused
rushtong Sep 9, 2024
9bf40f3
feat: fix typo
rushtong Sep 9, 2024
d79a57e
feat: configs
rushtong Sep 9, 2024
924b8ee
feat: update test
rushtong Sep 9, 2024
becd2c8
feat: styling
rushtong Sep 9, 2024
1115338
feat: auth test cleanup
rushtong Sep 10, 2024
907746f
Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in
rushtong Sep 10, 2024
a3eaeba
feat: sign-in test fix
rushtong Sep 10, 2024
2e4e93b
Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in
rushtong Sep 12, 2024
7deecc4
feat: handle errors, simplify login options
rushtong Sep 12, 2024
df09f44
feat: cleanup
rushtong Sep 12, 2024
dfa721a
feat: auth tests
rushtong Sep 13, 2024
59cd7be
feat: oidc client tests
rushtong Sep 13, 2024
20dad74
feat: sign-in tests and cleanup
rushtong Sep 13, 2024
25d21fd
Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in
rushtong Sep 13, 2024
f2bb354
feat: merge fix
rushtong Sep 14, 2024
28b4231
feat: docs and cleanup
rushtong Sep 14, 2024
c5c67a0
feat: test cleanup
rushtong Sep 14, 2024
1c1288b
feat: revert comment
rushtong Sep 14, 2024
e100a0f
feat: test cleanup
rushtong Sep 14, 2024
14176ab
feat: tos path test
rushtong Sep 16, 2024
a45492f
Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in
rushtong Sep 16, 2024
b035073
feat: fix new user registration path
rushtong Sep 16, 2024
a0b9fba
feat: rm duplicated metrics calls
rushtong Sep 17, 2024
18e7b13
feat: registration test
rushtong Sep 17, 2024
8944f48
feat: minor rename
rushtong Sep 17, 2024
9e5f497
feat: revert comment change
rushtong Sep 17, 2024
3c59ed3
Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in
rushtong Sep 17, 2024
ad2d8ed
feat: PR feedback
rushtong Sep 18, 2024
54f025b
feat: additional documentation
rushtong Sep 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion config/alpha.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"hash": "alpha",
"apiUrl": "https://consent.dsde-alpha.broadinstitute.org",
"ontologyApiUrl": "https://consent-ontology.dsde-alpha.broadinstitute.org/",
"clientId": "1020846292598-hd801vsmmbhh97vaf6aar17lu0q2evfj.apps.googleusercontent.com",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought alpha didn't exist anymore? Why do we need this config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config file name is not important and is, unfortunately, an artifact of our build image process. Originally, it did reference real environment variables. I'll change this in a separate PR/ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created https://broadworkbench.atlassian.net/browse/DCJ-678 to cover this bit of tech debt.

"errorApiKey": "1234567890abcdefghijklmnop",
"gaId": "",
"profileUrl": "https://profile-dot-broad-shibboleth-prod.appspot.com/dev",
Expand Down
1 change: 0 additions & 1 deletion config/base_config.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"ontologyApiUrl": "",
"terraUrl": "",
"tdrApiUrl": "",
"clientId": "",
"errorApiKey": "",
"nihUrl": "",
"gaId": "",
Expand Down
37 changes: 28 additions & 9 deletions cypress/component/Auth/auth.spec.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,47 @@
/* eslint-disable no-undef */

import {OidcBroker} from '../../../src/libs/auth/oidcBroker';
import {Auth} from '../../../src/libs/auth/auth';
import {Config} from '../../../src/libs/config';
import {GoogleIS} from '../../../src/libs/googleIS';
import {OAuth2} from '../../../src/libs/ajax/OAuth2';
import {Storage} from '../../../src/libs/storage';
import { v4 as uuid } from 'uuid';
import {v4 as uuid} from 'uuid';
import {mockOidcUser} from './mockOidcUser';

describe('Auth', function () {
describe('Auth Failure', function () {
it('Sign In error throws expected message', async function () {
cy.stub(OidcBroker, 'signIn').returns(null);
cy.on('fail', (err) => {
return err.message !== Auth.signInError();
});
Auth.signIn();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function still seems to be async, does it need to be awaited?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, but await-ing breaks the test, I think I need to use then

expect(Storage.getOidcUser()).to.be.null;
expect(Storage.userIsLogged()).to.be.false;
});
});

describe('Auth Success', function () {
// Intercept configuration calls
beforeEach(async () => {
beforeEach(() => {
cy.intercept({
method: 'GET',
url: '/config.json',
hostname: 'localhost',
}, {'env': 'ci'});
cy.stub(OAuth2, 'getConfig').returns({
'authorityEndpoint': 'authorityEndpoint',
'authorityEndpoint': Cypress.config().baseUrl,
'clientId': 'clientId'
});
await Auth.initialize();
Auth.initialize();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function still seems to be async, does it need to be awaited?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially confused by this. Cypress gives you a warning if you use async in beforeEach:

Cypress Warning: Cypress detected that you returned a promise in a test, but also invoked one or more cy commands inside of that promise.

The test title was:

  > Auth Success "before each" hook

While this works in practice, it's often indicative of an anti-pattern. You almost never need to return both a promise and also invoke cy commands.

Cy commands themselves are already promise like, and you can likely avoid the use of the separate Promise.

https://on.cypress.io/returning-promise-and-commands-in-test

});

it('Sign In stores the current user', async function () {
cy.stub(OidcBroker, 'signIn').returns(mockOidcUser);
await Auth.signIn();
expect(Storage.getOidcUser()).to.not.be.empty;
expect(Storage.userIsLogged()).to.be.true;
});

it('Sign Out Clears the session when called', async function () {
cy.stub(Config, 'getGoogleClientId').returns('12345');
cy.stub(GoogleIS, 'revokeAccessToken');
Storage.setUserIsLogged(true);
Storage.setAnonymousId(uuid());
Storage.setData('key', 'val');
Expand All @@ -38,4 +56,5 @@ describe('Auth', function () {
expect(Storage.getData('key')).to.be.null;
expect(Storage.getEnv()).to.be.null;
});

});
29 changes: 29 additions & 0 deletions cypress/component/Auth/mockOidcUser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import {OidcUser} from "../../../src/libs/auth/oidcBroker";

export const mockOidcUser: OidcUser = {
access_token: '',
get expires_in(): number | undefined {
return undefined;
},
session_state: undefined,
state: undefined,
token_type: '',
get expired(): boolean | undefined {
return undefined;
},
get scopes(): string[] {
return [];
},
toStorageString(): string {
return '';
},
profile: {
jti: undefined,
nbf: undefined,
sub: undefined,
iss: '',
aud: '',
exp: 0,
iat: 0
}
};
46 changes: 40 additions & 6 deletions cypress/component/Auth/oidcBroker.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,27 @@
/* eslint-disable no-undef */

import {Config} from '../../../src/libs/config';
import {GoogleIS} from '../../../src/libs/googleIS';
import {OAuth2} from '../../../src/libs/ajax/OAuth2';
import {OidcBroker} from '../../../src/libs/auth/oidcBroker';

describe('OidcBroker', function () {
describe('OidcBroker Failure', function () {

it('Get User Manager Fails without initialization', function () {
cy.on('fail', (err) => {
return !err.message.includes('initialized');
});
OidcBroker.getUserManager();
});

it('Get User Manager Settings Fails without initialization', function () {
cy.on('fail', (err) => {
return !err.message.includes('initialized');
});
OidcBroker.getUserManagerSettings();
});

});

describe('OidcBroker Success', function () {
// Intercept configuration calls
beforeEach(() => {
cy.intercept({
Expand All @@ -14,13 +30,30 @@ describe('OidcBroker', function () {
hostname: 'localhost',
}, {'env': 'ci'});
cy.stub(OAuth2, 'getConfig').returns({
'authorityEndpoint': 'authorityEndpoint',
'authorityEndpoint': Cypress.config().baseUrl,
'clientId': 'clientId'
});
});

it('Initialization Succeeds', async function () {
await OidcBroker.initialize();
expect(OidcBroker.getUserManager()).to.not.be.null;
expect(OidcBroker.getUserManagerSettings()).to.not.be.null;
});

it('Sign In calls Oidc Broker UserManager sign-in popup function', async function () {
await OidcBroker.initialize();
const um = OidcBroker.getUserManager();
cy.spy(um, 'signinPopup').as('signinPopup');
// Since we are not calling a real sign-in url, we expect oidc-client errors when doing so
cy.on('uncaught:exception', (err) => {
return !(err.message.includes('Invalid URL'))
});
OidcBroker.signIn();
expect(um.signinPopup).to.be.called;
});

it('Sign Out calls Oidc UserManager sign-out functions', async function () {
cy.stub(Config, 'getGoogleClientId').returns('12345');
cy.stub(GoogleIS, 'revokeAccessToken');
await OidcBroker.initialize();
const um = OidcBroker.getUserManager();
cy.spy(um, 'removeUser').as('removeUser');
Expand All @@ -29,4 +62,5 @@ describe('OidcBroker', function () {
expect(um.removeUser).to.be.called;
expect(um.clearStaleState).to.be.called;
});

});
48 changes: 0 additions & 48 deletions cypress/component/SignIn/google_is.spec.js

This file was deleted.

119 changes: 101 additions & 18 deletions cypress/component/SignIn/sign_in_button.spec.js
Original file line number Diff line number Diff line change
@@ -1,30 +1,113 @@
/* eslint-disable no-undef */

import React from 'react';
import { mount } from 'cypress/react';
import {mount} from 'cypress/react18';
import SignInButton from '../../../src/components/SignInButton';
import { Config } from '../../../src/libs/config';
import {User} from '../../../src/libs/ajax/User';
import {Auth} from '../../../src/libs/auth/auth';
import {Storage} from '../../../src/libs/storage';
import {Metrics} from '../../../src/libs/ajax/Metrics';
import {StackdriverReporter} from '../../../src/libs/stackdriverReporter';
import {ToS} from '../../../src/libs/ajax/ToS';
import {mockOidcUser} from '../Auth/mockOidcUser';

const signInText = 'Sign-in';
const signInText = 'Sign In';

// Note that we do not want to click the signin button
// in tests as that would trigger an auth-flow we cannot
// replicate in a test environment.
describe('Sign In Component', function() {
it('Sign In Button Loads when client id is valid', function () {
const duosUser = {
displayName: 'display name',
email: 'test@user.com',
roles: [{
name: 'Admin'
}]
};

const userStatus = {
'adminEnabled': true,
'enabled': true,
'inAllUsersGroup': true,
'inGoogleProxyGroup': true,
'tosAccepted': true
};

const notAcceptedUserStatus = Object.assign({}, userStatus, {'tosAccepted': false});

describe('Sign In: Component Loads', function () {

it('Sign In Button Loads', function () {
cy.viewport(600, 300);
mount(<SignInButton history={undefined}/>);
cy.contains(signInText).should('exist');
});

it('Sign In: On Success', function () {
cy.viewport(600, 300);
cy.stub(Auth, 'signIn').returns(Promise.resolve(mockOidcUser));
cy.stub(User, 'getMe').returns(duosUser);
cy.stub(StackdriverReporter, 'report');
cy.stub(Metrics, 'identify');
cy.stub(Metrics, 'syncProfile');
cy.stub(Metrics, 'captureEvent');
cy.stub(ToS, 'getStatus').returns(userStatus);
mount(<SignInButton history={[]}/>);
cy.get('button').click().then(() => {
expect(Storage.getCurrentUser()).to.deep.equal(duosUser);
expect(Storage.getAnonymousId()).to.not.be.null;
expect(StackdriverReporter.report).to.not.be.called;
expect(Metrics.identify).to.be.called;
expect(Metrics.syncProfile).to.be.called;
expect(Metrics.captureEvent).to.be.called;
});
});

it('Sign In: No Roles Error Reporter Is Called', function () {
const bareUser = {email: 'test@user.com'};
cy.viewport(600, 300);
// Load the client id from perf so we can have a valid button
cy.readFile('config/alpha.json').then((config) => {
const clientId = config.clientId;
cy.stub(Config, 'getGoogleClientId').returns(clientId);
mount(<SignInButton />);
cy.contains(signInText).should('exist');
cy.stub(Auth, 'signIn').returns(Promise.resolve(mockOidcUser));
cy.stub(User, 'getMe').returns(bareUser);
cy.stub(StackdriverReporter, 'report');
cy.stub(Metrics, 'identify');
cy.stub(Metrics, 'syncProfile');
cy.stub(Metrics, 'captureEvent');
cy.stub(ToS, 'getStatus').returns(userStatus);
mount(<SignInButton history={[]}/>);
cy.get('button').click().then(() => {
expect(StackdriverReporter.report).to.be.called;
});
});
it('Spinner loads when client id is empty', function () {

it('Sign In: Redirects to ToS if not accepted', function () {
cy.viewport(600, 300);
cy.stub(Config, 'getGoogleClientId').returns('');
mount(<SignInButton />);
cy.contains(signInText).should('not.exist');
cy.stub(Auth, 'signIn').returns(Promise.resolve(mockOidcUser));
cy.stub(User, 'getMe').returns(duosUser);
cy.stub(ToS, 'getStatus').returns(notAcceptedUserStatus);
cy.stub(Metrics, 'identify');
cy.stub(Metrics, 'syncProfile');
cy.stub(Metrics, 'captureEvent');
let history = [];
mount(<SignInButton history={history}/>);
cy.get('button').click().then(() => {
expect(history).to.not.be.empty;
expect(history[0].includes('tos_acceptance')).to.be.true;
});
});

it('Sign In: Registers user if not found and redirects to ToS', function () {
cy.viewport(600, 300);
cy.stub(Auth, 'signIn').returns(Promise.resolve(mockOidcUser));
// Simulate user not found
cy.stub(User, 'getMe').throws();
cy.stub(User, 'registerUser').returns(duosUser);
cy.stub(ToS, 'getStatus').returns(notAcceptedUserStatus);
cy.stub(Metrics, 'identify');
cy.stub(Metrics, 'syncProfile');
cy.stub(Metrics, 'captureEvent');
let history = [];
mount(<SignInButton history={history}/>);
cy.get('button').click().then(() => {
expect(User.registerUser).to.be.called;
expect(history).to.not.be.empty;
expect(history[0].includes('tos_acceptance')).to.be.true;
});
});

});
1 change: 0 additions & 1 deletion public/config-example.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"ontologyApiUrl": "https://ontologyURL.org/",
"terraUrl": "https://terraURL.org/",
"tdrApiUrl": "https://tdrApiUrl.org/",
"clientId": "111111111111-11111111111111111111111111111111.apps.googleusercontent.com",
"errorApiKey": "example",
"gaId": "",
"profileUrl": "https://profile-dot-broad-shibboleth-prod.appspot.com/dev",
Expand Down
9 changes: 2 additions & 7 deletions src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,13 @@ function App() {
setUserIsLogged();
});

const signIn = async () => {
await Storage.setUserIsLogged(true);
await setIsLoggedIn(true);
};

return (
<div className="body">
<div className="wrap">
<div className="main">
<DuosHeader onSignIn={signIn} />
<DuosHeader/>
<Spinner name="mainSpinner" group="duos" loadingImage={loadingImage} />
<Routes onSignIn={signIn} isLogged={isLoggedIn} env={env} />
<Routes isLogged={isLoggedIn} env={env} />
</div>
</div>
<DuosFooter />
Expand Down
Loading
Loading