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.

Sign up for GitHub

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

Jump to bottom

Add in automatic sign out after an interval of inactivity #652

Merged
merged 9 commits into from
May 22, 2019
Merged

Add in automatic sign out after an interval of inactivity #652

merged 9 commits into from
May 22, 2019

Conversation

rsasch
Copy link
Contributor

@rsasch rsasch commented May 21, 2019

Auto-sign-out will be enabled if the capabilities_config.json contains the following in the authentication section:

"forcedLogoutDomains": [
  <one or more domain>
],
"forcedLogoutTime": <number of milliseconds of inactivity that will result in sign-out>

Closes #623

@rsasch rsasch self-assigned this May 21, 2019
@gemmalam gemmalam requested review from aednichols and kshakir May 22, 2019 14:34
kshakir
kshakir approved these changes May 22, 2019
View reviewed changes
Copy link
Contributor

@kshakir kshakir left a comment

Choose a reason for hiding this comment

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

LGTM

ui/src/app/app.component.ts Outdated Show resolved Hide resolved
aednichols
aednichols reviewed May 22, 2019
View reviewed changes
Copy link
Collaborator

@aednichols aednichols left a comment

Choose a reason for hiding this comment

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

I'm still trying to test this locally, will finish reviewing once I rehabilitate my environment

ui/src/app/core/auth.service.ts Outdated
@@ -96,7 +115,47 @@ export class AuthService {
return auth2.signOut();
}

private revokeToken(): Promise<any> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to specify a specific type instead of any?

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 was based on how signOut() was implemented. I've now made signIn(), signOut() and revokeToken() use the same pattern.

ui/src/app/core/auth.service.ts
@@ -50,6 +63,12 @@ export class AuthService {
if (!capabilities.authentication || !capabilities.authentication.isRequired) {
return;
}

if (capabilities.authentication.forcedLogoutDomains && capabilities.authentication.forcedLogoutTime && capabilities.authentication.forcedLogoutTime > (this.WARNING_INTERVAL * 2)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the meaning of WARNING_INTERVAL * 2.

Is it saying that if the forcedLogoutTime is really short - only twice the warning interval - don't bother showing a warning?

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 isn't a test of whether or not to show a warning, it's whether or not to set up the variables that will be used to force a logout after an idle period (see line 47).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would we skip setting the variables if capabilities.authentication.forcedLogoutTime ≤ (this.WARNING_INTERVAL * 2)

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 was to guard against someone configuring the timeout to be an unreasonably short time and having the UI just keep logging users out. The only validation of capabilities.authentication.forcedLogoutTime that's in place is that it needs to be a valid int.

ui/src/app/core/auth.service.ts
this.snackBar.dismiss();

this.warningTimer = window.setTimeout(() => {
this.snackBar.open('You are about to be logged out due to inactivity');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be great to tell the user that they have 10 seconds in this message - it's pretty short compared to the one minute or 30 seconds I've seen elsewhere

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 think having the static value of "10 seconds" will be confusing to users. What if they only spot the message when they actually have only 5 seconds to go? Unfortunately, I haven't found a way to easily update the message in the SnackBar component without closing it and opening it again, so I don't know how to show an actual countdown.

aednichols
aednichols reviewed May 22, 2019
View reviewed changes
servers/cromwell/README.md Outdated
"openid",
"email",
"profile"
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing , breaks JSON

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops – thanks for catching that; fixed

@rsasch rsasch merged commit 977c5be into master May 22, 2019
@rsasch rsasch deleted the rsasch/auto-signout branch May 22, 2019 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kshakir kshakir kshakir approved these changes

@aednichols aednichols aednichols approved these changes

Assignees

@rsasch rsasch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatic Sign out
3 participants
@rsasch @kshakir @aednichols