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

feat: Allow organisation admins to mandate 2fa for their organisation #2877

Merged
merged 1 commit into from
Nov 13, 2023
Merged

feat: Allow organisation admins to mandate 2fa for their organisation #2877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions api/organisations/migrations/0047_organisation_force_2fa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 3.2.20 on 2023-10-23 14:33

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('organisations', '0046_allow_allowed_projects_to_be_null'),
]

operations = [
migrations.AddField(
model_name='organisation',
name='force_2fa',
field=models.BooleanField(default=False),
),
]
1 change: 1 addition & 0 deletions api/organisations/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class Organisation(LifecycleModelMixin, SoftDeleteExportableModel):
feature_analytics = models.BooleanField(
default=False, help_text="Record feature analytics in InfluxDB"
)
force_2fa = models.BooleanField(default=False)

class Meta:
ordering = ["id"]
Expand Down
1 change: 1 addition & 0 deletions api/organisations/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class Meta:
"persist_trait_data",
"block_access_to_admin",
"restrict_project_create_to_admin",
"force_2fa",
)
read_only_fields = (
"id",
Expand Down
3 changes: 2 additions & 1 deletion frontend/common/dispatcher/app-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ const AppActions = Object.assign({}, require('./base/_app-actions'), {
identity,
})
},
confirmTwoFactor(pin, onError) {
confirmTwoFactor(pin, onError, isLoginPage) {
Dispatcher.handleViewAction({
actionType: Actions.CONFIRM_TWO_FACTOR,
isLoginPage,
onError,
pin,
})
Expand Down
20 changes: 16 additions & 4 deletions frontend/common/stores/account-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const controller = {
API.ajaxHandler(store, e)
})
},
confirmTwoFactor: (pin, onError) => {
confirmTwoFactor: (pin, onError, isLoginPage) => {
store.saving()

return data
Expand All @@ -50,6 +50,9 @@ const controller = {
store.model.twoFactorConfirmed = true

store.saved()
if (isLoginPage) {
window.location.href = `/projects`
}
})
.catch((e) => {
if (onError) {
Expand Down Expand Up @@ -158,7 +161,7 @@ const controller = {
})
.then((res) => {
API.trackEvent(Constants.events.LOGIN)

store.samlOrOauth = false
if (res.ephemeral_token) {
store.ephemeral_token = res.ephemeral_token
store.model = {
Expand Down Expand Up @@ -189,6 +192,7 @@ const controller = {
},
)
.then((res) => {
store.samlOrOauth = true
if (res.ephemeral_token) {
store.ephemeral_token = res.ephemeral_token
store.model = {
Expand Down Expand Up @@ -381,8 +385,12 @@ const store = Object.assign({}, BaseStore, {
return false
}

if (store.samlOrOauth) {
Copy link
Member

Choose a reason for hiding this comment

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

Can be replaced with my other suggestion.

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 prevents the 2factor from being forced on Oauth or Saml accounts

return false
}

return (
Utils.getFlagsmithHasFeature('forced_2fa') &&
Utils.getFlagsmithHasFeature('force_2fa') &&
store.getOrganisations() &&
store.getOrganisations().find((o) => o.force_2fa)
)
Expand Down Expand Up @@ -505,7 +513,11 @@ store.dispatcherIndex = Dispatcher.register(store, (payload) => {
controller.enableTwoFactor()
break
case Actions.CONFIRM_TWO_FACTOR:
controller.confirmTwoFactor(action.pin, action.onError)
controller.confirmTwoFactor(
action.pin,
action.onError,
action.isLoginPage,
)
break
case Actions.DISABLE_TWO_FACTOR:
controller.disableTwoFactor()
Expand Down
2 changes: 1 addition & 1 deletion frontend/web/components/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ const App = class extends Component {
)
}
if (AccountStore.forced2Factor()) {
Copy link
Member

@kyle-ssg kyle-ssg Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think instead of making all of the isLoginChanges, just adjust this function to return false if 2fa has been set. It should remove the need for any other frontend change in this PR.

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 change was made to handle the case where you are already logged in, your organization forces 2factor and you disable your 2factor, it prevents the page from reloading, which does happen if 2factor is enabled on login.

return <AccountSettingsPage />
return <AccountSettingsPage isLoginPage={true} />
}
const projectNotLoaded =
!ProjectStore.model && document.location.href.includes('project/')
Expand Down
8 changes: 6 additions & 2 deletions frontend/web/components/TwoFactor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export default class TheComponent extends PureComponent {
onError = () => this.setState({ error: true })

render() {
// const { props } = this;
const { isLoginPage } = this.props
return (
<div>
<AccountProvider>
Expand All @@ -38,7 +38,11 @@ export default class TheComponent extends PureComponent {
}
onRegister={() => {
this.setState({ error: null })
confirmTwoFactor(this.state.pin, this.onError)
confirmTwoFactor(
this.state.pin,
this.onError,
isLoginPage || false,
)
}}
/>
</div>
Expand Down
2 changes: 1 addition & 1 deletion frontend/web/components/pages/AccountSettingsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class TheComponent extends Component {
One of your organisations has enfoced Two-Factor Authentication,
please enable it to continue.
</p>
<TwoFactor />
<TwoFactor isLoginPage={true} />
</div>
) : (
<div className='app-container container'>
Expand Down