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

EP-2362 - Okta #1080

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

EP-2362 - Okta #1080

wants to merge 50 commits into from

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Dec 11, 2023

Description

Replacing our login/logout functionality with Okta. This PR allows users to login and create an account with Okta.

Sign in

To sign, it's very much like other applications where the user gets redirected to Okta and brought back to the redirect URL.
We no longer maintain login/session data on our local storage or session storage. This is now stored via Okta.

Register

The user enters their email, first name and last name. On submit, we do a post request to create an account on Okta while keeping the user on the Give site.
Once that is complete, they go into the next phase of activating their email. Once activated it will automatically update the modal to send the user to the next step.

Jira

@wrandall22
Copy link
Contributor

Doesn't seem to like import { afterEach } from 'node:test'

@@ -388,6 +389,11 @@ class ProductConfigFormController {
this.$window.location = url
}
}

shouldShowForcedUserToLogoutError() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tell me more about this new function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On adding an item to the cart, we check to see if the user has a public role, is logged into Okta and has no items in their account. If so, we automatically force the user to logout.

This was happening before, but when it was logging out, it didn't require a refresh. Okta requires a refresh since we send the user to the Okta site to clear Okta's session data and then return to the Give site.

We handle this here src/common/services/api/cart.service.js - Lines 164 - 181

On the sessionService.oktaSignOut we pass a prop which tells session data to store a prop called forcedUserToLogout. We also ensure Okta returns the user to the same page.
src/common/services/session/session.service.js Lines 317 - 322

Onsrc/app/productConfig/productConfigForm/productConfigForm.component.js, we watch for the forcedUserToLogout session data and show the warning to the user if present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@dr-bizz dr-bizz added the On Staging Will be merged to the staging branch by Github Actions label Dec 19, 2023
@wrandall22 wrandall22 changed the title 2362 okta EP-2362 - Okta Dec 20, 2023
@wrandall22
Copy link
Contributor

the *cloud and nonprod environments are not in use anymore and can just be deleted, with the exception of devcloud.

@wrandall22
Copy link
Contributor

wrandall22 commented Jan 11, 2024

Not sure if the latest has been merged to stage yet or not, but something is still sending me to the home page when hitting branded checkout. You can test by going to https://give-stage2.cru.org/branded-checkout.html

@wrandall22
Copy link
Contributor

wrandall22 commented Jan 12, 2024

Here is a test branded checkout page that is not on give itself, we'll need to make sure the Okta logout works on pages like this as well.

Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

Working through the files again to try and find anything we might have missed. Haven't gotten through it all, but want this feedback at least to get through.

Comment on lines +120 to +131
<div class="row">
<div class="col-xs-12">
<button
type="button"
class="btn pull-left add-spouse-btn"
ng-class="{ 'btn-default': $ctrl.showSpouseFields, 'btn-primary': !$ctrl.showSpouseFields }"
ng-click="$ctrl.toggleSpouseFields()"
ng-if="$ctrl.donorDetails['donor-type'] === 'Household'"
translate
>{{$ctrl.showSpouseFields ? "Remove spouse" : "Add spouse"}}</button>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check if it is ok to modify branded checkout like this. I'm guessing they will want to only do this for the sign up process, not during checkout and branded checkout which is what is being done here.

Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

Got through a few more.

if (angular.isDefined(this.verificationServiceSubscription)) this.verificationServiceSubscription.unsubscribe()
this.verificationServiceSubscription = this.verificationService.postDonorMatches().subscribe({
next: () => { this.stateChanged('user-match') }, // Donor match success, Proceed to step 5.
error: () => { this.onCancel() } // Donor Match failed, Register Account workflow failed
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should handle timeouts (503 response) differently. It currently brings the donor back to the home page with no indication that something went wrong. Probably out of scope for this PR since it is handling it the same way as before.

src/common/components/signInForm/signInForm.tpl.html Outdated Show resolved Hide resolved
Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

Got through all the files, hopefully caught all the interesting things.

src/common/components/signUpModal/signUpModal.tpl.html Outdated Show resolved Hide resolved
src/common/services/session/session.service.spec.js Outdated Show resolved Hide resolved
src/common/services/session/session.service.spec.js Outdated Show resolved Hide resolved
src/common/services/session/session.service.spec.js Outdated Show resolved Hide resolved
src/common/services/session/sessionModal.service.js Outdated Show resolved Hide resolved
src/common/services/session/sessionModal.service.spec.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

I left a few comments and recommendations.

We should also add a index.page.ts which forwards /settings -> /settings/preferences/

Comment on lines +70 to +72
this.sessionHandleOktaRedirectService.errorMessageSubject.subscribe((errorMessage) => {
this.errorMessage = errorMessage
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logging the err to the this.errorMessage variable was in the original version of this file, so I've added it, but nowhere uses errorMessage so we can take it out unless you think we could use it in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing this in the master branch. If it fails, it redirects to the home page. However, I do see $ctrl.errorMessage being used on the sign in form. Would that show this error message?

Comment on lines +48 to +50
this.sessionHandleOktaRedirectService.errorMessageSubject.subscribe((errorMessage) => {
this.errorMessage = errorMessage
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here again with the errorMessage I've added it in, but since we don't use errorMessage we can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think the signInForm would inherit this error message

}

$onInit () {
if (includes([Roles.identified, Roles.registered], this.sessionService.getRole())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if (includes([Roles.identified, Roles.registered], this.sessionService.getRole())) {
if (this.sessionService.getRole() !== 'PUBLIC')) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably fine since we aren't planning to add more roles anytime soon I think.

if (this.unverifiedAccount.email) {
this.initialLoading = false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test if we need this in If statement or if we should wrap the whole this.sessionService.checkCreateAccountStatus in the email If statement.

Comment on lines +53 to +57
if (!this.donorDetails.name['given-name'] && angular.isDefined(this.sessionService.session.first_name)) {
this.donorDetails.name['given-name'] = this.sessionService.session.first_name
}
if (!this.donorDetails.name['family-name'] && angular.isDefined(this.sessionService.session.last_name)) {
this.donorDetails.name['family-name'] = this.sessionService.session.last_name
}
if (angular.isUndefined(this.donorDetails.email) && angular.isDefined(this.sessionService.session.email)) {
this.donorDetails.email = this.sessionService.session.email
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should do all these the same. angular.isUndefined or !this.donorDetails...

it('does not broadcast an event if not registered/new', () => {
$ctrl.sessionEnforcerService.mock.calls[0][1]['change'](Roles.registered, 'COMPLETED')
expect($ctrl.$rootScope.$broadcast).not.toHaveBeenCalled()
it('should call onHandleOktaRedirect', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not just a duplicate of the one above now (it('should call onHandleOktaRedirect onInit')?


expect($log.error).toHaveBeenCalledWith('Failed to redirect from Okta', 'ERROR_MESSAGE')
sessionHandleOktaRedirectService.errorMessageSubject.subscribe((errorMessage) => {
expect(errorMessage).toEqual('generic')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually get called?

expect(sessionService.updateCurrentProfile).toHaveBeenCalled()
expect($rootScope.$broadcast).toHaveBeenCalledWith(LoginOktaOnlyEvent, 'register-account')
})
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove semicolon

@@ -259,24 +259,21 @@ describe('cart service', () => {
).respond(200)
})

it('should delete cookies and addItem to cart', () => {
it('should delete cookies and addItem to cart when not authenicated', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should delete cookies and addItem to cart when not authenicated', () => {
it('should delete cookies and addItem to cart when not authenticated', () => {

src/app/signIn/signIn.spec.js Outdated Show resolved Hide resolved
@@ -189,7 +189,7 @@ describe('signUpActivationModal', function () {
expect ($ctrl.unverifiedAccount).toEqual({
...createAccountDataCookieData,
...checkCreateAccountStatusData,
status: 'Awaiting Admin approval'
status: 'Sending Activation Email'
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a case where it would have been helpful to have a constant that we reference instead of hard-coding the language in multiple places.

Comment on lines +70 to +72
this.sessionHandleOktaRedirectService.errorMessageSubject.subscribe((errorMessage) => {
this.errorMessage = errorMessage
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing this in the master branch. If it fails, it redirects to the home page. However, I do see $ctrl.errorMessage being used on the sign in form. Would that show this error message?

Comment on lines +48 to +50
this.sessionHandleOktaRedirectService.errorMessageSubject.subscribe((errorMessage) => {
this.errorMessage = errorMessage
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think the signInForm would inherit this error message

}

$onInit () {
if (includes([Roles.identified, Roles.registered], this.sessionService.getRole())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably fine since we aren't planning to add more roles anytime soon I think.

@wrandall22
Copy link
Contributor

I noticed the last commit didn't make it to stage, maybe due to being out of sync

Note: Do not use the authClient to logout, just have cortex gateway delete the cookies. This matches the current site experience, and also keeps us from having to add a separate logout uri for every branded checkout page.
Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

What are your thoughts about the register-account vs sign-up language differences throughout the code?

src/app/signOut/signOut.component.js Show resolved Hide resolved
<div>
<loading>
<p><translate>Successfully logged out.</translate></p>
<p><translate>Redirecting to your previous page...</translate></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be dynamic language based on the logic in the controller.

src/app/signOut/signOut.spec.js Outdated Show resolved Hide resolved
src/app/signOut/signOut.spec.js Outdated Show resolved Hide resolved
src/app/main/main.component.js Show resolved Hide resolved
Comment on lines 30 to 32
<li>Open the email and locate the verification link provided.</li>
<li>Click on the verification link. This will typically be a button or a hyperlink.</li>
<li>You will then be redirected to Okta to create your account password.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

These should also get translate properties in case they get translated at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants