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

🐛 Hide Sign Out link during MFA based on settings #175

Conversation

mauriciocastillosilva-okta
Copy link
Contributor

Settings new prop: hideSignOutLinkInMFA
This is used for App Sign in Flows, when we only need to do MFA. It does not make sense to have a SignOut link in that flow (that sign out will not kill the user session, but only the app session we are trying to login in too. Plus it redirects to the main login form, which also does not make sense)

Resolves: OKTA-114022
Bacon: test

@rchild-okta
Copy link
Contributor

rchild-okta commented Feb 16, 2017

Do we need a flag or can we just hide it from MFA for all users? I.e. if it doesn't make sense, why even make it an option?

Btw, if you want to add another setting to the widget, make sure that you update the README as well to document it.

Edit: I guess we need this because we want to show the sign out link in the sign-in flow, but not in the per-app mfa flows. Is there another way we can know which flow we're in?

@mauriciocastillosilva-okta
Copy link
Contributor Author

I'll update the README, thanks for reminding me (note to myself: I need to keep this in mind for every PR that adds a setting or something that needs doc)

It does not make sense in App MFA, but not in MFA in general... that is why I figured adding the option on the settings was the best way, so it is up to the consumer to set that flag if the sign out link is not needed.

@rchild-okta
Copy link
Contributor

So main question I have is if there's another way to know this information without having to pass the flag in explicitly - i.e. for example when bootstrapping the widget with the per-app stateToken, can it pass us back a signal (or is it already somehow different) that we're doing a step up flow rather than primary auth.

@mauriciocastillosilva-okta
Copy link
Contributor Author

I don't know if we can do it without the flag... the stateToken does not have that info as far as I know, and just by going to the refresh state will not work, since other flows could get there (for example, user settings page to verify password and MFA).

@rchild-okta
Copy link
Contributor

rchild-okta commented Feb 17, 2017

Makes sense. Can you talk to the auth team and see if they're open to the idea of giving us context back on refreshToken requests (not a blocker for merging this PR, but something they can think about to make it easier for consumers like the OSW in the future).

@mauriciocastillosilva-okta
Copy link
Contributor Author

@srinivasanagandla-okta take a look at Reman's comment

README.md Outdated
@@ -867,6 +870,14 @@ Options for the [OpenId Connect](http://developer.okta.com/docs/api/resources/oi
recoveryToken: 'x0whAcR02i0leKtWMZVc'
```

## MFA Challenge flow without sign out link
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, good thing we added the doc - looking at this now, would prefer to add this under our feature settings rather than a top level config item.

Copy link
Contributor

@rchild-okta rchild-okta left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

new feature added: hideSignOutLinkInMFA
This is used for App Sign in Flows, when we only need to do MFA. It does not make sense to have a SignOut link in that flow (that sign out will not kill the user session, but only the app session we are trying to login in too. Plus it redirects to the main login form, which also does not make sense)

Resolves: OKTA-114022
Bacon: test
@mauriciocastillosilva-okta mauriciocastillosilva-okta merged commit cd94d48 into okta:master Mar 6, 2017
@mauriciocastillosilva-okta mauriciocastillosilva-okta deleted the mcs-okta-114022-hideSignOutOnAppMFA branch April 27, 2017 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants