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

Don't allow enforcing 2FA when no provider is enabled #16463

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 19 additions & 2 deletions settings/Controller/TwoFactorSettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,32 @@

use OC\Authentication\TwoFactorAuth\EnforcementState;
use OC\Authentication\TwoFactorAuth\MandatoryTwoFactor;
use OC\Authentication\TwoFactorAuth\Manager;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\JSONResponse;
use OCP\IRequest;
use OC\User\Manager as UserManager;

class TwoFactorSettingsController extends Controller {

/** @var MandatoryTwoFactor */
private $mandatoryTwoFactor;

/** @var Manager */
private $manager;

private $userManager;

public function __construct(string $appName,
IRequest $request,
MandatoryTwoFactor $mandatoryTwoFactor) {
MandatoryTwoFactor $mandatoryTwoFactor,
Manager $manager,
UserManager $userManager) {
parent::__construct($appName, $request);

$this->mandatoryTwoFactor = $mandatoryTwoFactor;
$this->manager = $manager;
$this->userManager = $userManager;
}

public function index(): JSONResponse {
Expand All @@ -57,4 +68,10 @@ public function update(bool $enforced, array $enforcedGroups = [], array $exclud
return new JSONResponse($this->mandatoryTwoFactor->getState());
}

}
public function getEnabledProvidersForUser($uid): JSONResponse {
error_log('uid: '.$uid,3,'uid');
$user = $this->userManager->get($uid);
return new JSONResponse($this->manager->getProviderSet($user)->getPrimaryProviders());
}

}
5 changes: 5 additions & 0 deletions settings/css/settings.scss
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,11 @@ table.nostyle {
height: 16px;
vertical-align: sub;
}

.warning {
margin-top: 24px;
background-color: var(--color-warning);
}
}

.social-button {
Expand Down
1 change: 1 addition & 0 deletions settings/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
['name' => 'ChangePassword#changeUserPassword', 'url' => '/settings/users/changepassword', 'verb' => 'POST'],
['name' => 'TwoFactorSettings#index', 'url' => '/settings/api/admin/twofactorauth', 'verb' => 'GET'],
['name' => 'TwoFactorSettings#update', 'url' => '/settings/api/admin/twofactorauth', 'verb' => 'PUT'],
['name' => 'TwoFactorSettings#getEnabledProvidersForUser', 'url' => '/settings/api/users/{uid}/twoFactorProviders', 'verb' => 'GET'],
]
]);

Expand Down
30 changes: 27 additions & 3 deletions settings/src/components/AdminTwoFactor.vue
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@
<label for="two-factor-enforced">{{ t('settings', 'Enforce two-factor authentication') }}</label>
</p>
<template v-if="enforced">
<p id="two-factor-warning-global" class="warning" v-if="noProviderGlobally">
{{ t('settings', 'No Two-Factor authentication provider enabled on this server. Are you sure that you want to enforce Two-Factor authentication?') }}
</p>
<p id="two-factor-warning-admin" class="warning" v-if="noProviderAdmin">
{{ t('settings', 'No Two-Factor authentication provider enabled for your account. Are you sure that you want to enforce Two-Factor authentication?') }}
</p>
<h3>{{ t('settings', 'Limit to groups') }}</h3>
{{ t('settings', 'Enforcement of two-factor authentication can be set for certain groups only.') }}
<p>
Expand Down Expand Up @@ -78,6 +84,10 @@
components: {
Multiselect
},
beforeMount(){
this.$store.dispatch('getAllApps');
Copy link
Member

Choose a reason for hiding this comment

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

Loading state here?

this.$store.dispatch('getEnabledProvidersCurrentUser');
},
data () {
return {
loading: false,
Copy link
Member

Choose a reason for hiding this comment

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

If so maybe set it to true by default?

Expand All @@ -89,7 +99,7 @@
computed: {
enforced: {
get: function () {
return this.$store.state.enforced
return this.$store.state.security.enforced
},
set: function (val) {
this.dirty = true
Expand All @@ -98,7 +108,7 @@
},
enforcedGroups: {
get: function () {
return this.$store.state.enforcedGroups
return this.$store.state.security.enforcedGroups
},
set: function (val) {
this.dirty = true
Expand All @@ -107,13 +117,27 @@
},
excludedGroups: {
get: function () {
return this.$store.state.excludedGroups
return this.$store.state.security.excludedGroups
},
set: function (val) {
this.dirty = true
this.$store.commit('setExcludedGroups', val)
}
},
noProviderGlobally: {
Copy link
Member

Choose a reason for hiding this comment

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

You can directly use the function if you don't have a setter here.

Suggested change
noProviderGlobally: {
noProviderGlobally() {
var providers = this.$store.getters.getAllApps.filter( function(app) {
return ('two-factor-providers' in app && 'provider' in app['two-factor-providers'] && app['active'] === true);
});
return (providers.length === 0);
}

get: function () {
var providers = this.$store.getters.getAllApps.filter( function(app) {
return ('two-factor-providers' in app && 'provider' in app['two-factor-providers'] && app['active'] === true);
});
return (providers.length === 0);
}
},
noProviderAdmin: {
get: function () {
var providers = this.$store.getters.getEnabledProvidersCurrentUser;
return (providers.length === 0);
}
},
},
mounted () {
// Groups are loaded dynamically, but the assigned ones *should*
Expand Down
9 changes: 5 additions & 4 deletions settings/src/main-admin-security.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Vue from 'vue'

import AdminTwoFactor from './components/AdminTwoFactor.vue'
import store from './store/admin-security'
import store from './store/'

__webpack_nonce__ = btoa(OC.requestToken)

Expand All @@ -11,9 +11,10 @@ Vue.prototype.t = t;
window.OC = window.OC || {};
window.OC.Settings = window.OC.Settings || {};

store.replaceState(
OCP.InitialState.loadState('settings', 'mandatory2FAState')
)
let initialState = OCP.InitialState.loadState('settings', 'mandatory2FAState');
store.commit('setEnforced', initialState.enforced);
Copy link
Member

Choose a reason for hiding this comment

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

This is considered bad practice to commit from a component.
You need to use actions for that.

store.commit('setEnforcedGroups', initialState.enforcedGroups);
store.commit('setExcludedGroups', initialState.excludedGroups);

const View = Vue.extend(AdminTwoFactor)
new View({
Expand Down
55 changes: 40 additions & 15 deletions settings/src/store/admin-security.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,18 @@

import Vue from 'vue'
import Vuex from 'vuex'
import api from './api';

Vue.use(Vuex)

export const mutations = {
const state = {
enforced: false,
enforcedGroups: [],
excludedGroups: [],
enabledProvidersCurrentUser: [],
};

const mutations = {
setEnforced(state, enabled) {
Vue.set(state, 'enforced', enabled)
},
Expand All @@ -33,31 +41,48 @@ export const mutations = {
},
setExcludedGroups(state, used) {
Vue.set(state, 'excludedGroups', used)
},
setEnabledProvidersCurrentUser(state, providers) {
Vue.set(state, 'enabledProvidersCurrentUser', providers)
}
}

const getters = {
getEnabledProvidersCurrentUser(state) {
return state.enabledProvidersCurrentUser;
}
}

export const actions = {
const actions = {
save ({commit}, ) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird?

commit('setEnabled', false);

return generateCodes()
.then(({codes, state}) => {
commit('setEnabled', state.enabled);
commit('setTotal', state.total);
commit('setUsed', state.used);
commit('setCodes', codes);
return true;
});
}
commit('setTotal', state.total);
commit('setUsed', state.used);
commit('setCodes', codes);
return true;
});
},
getEnabledProvidersCurrentUser(context) {
context.commit('startLoading', 'providers');
var user = OC.getCurrentUser().uid;
return api.get(OC.generateUrl(`/settings/api/users/${user}/twoFactorProviders`))
.then((response) => {
context.commit('setEnabledProvidersCurrentUser', response.data);
context.commit('stopLoading', 'providers');
return true;
})
.catch((error) => context.commit('API_FAILURE', error));
},
}

export default new Vuex.Store({
export default {
Copy link
Member

Choose a reason for hiding this comment

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

Much cleaner!! 👍

strict: process.env.NODE_ENV !== 'production',
state: {
enforced: false,
enforcedGroups: [],
excludedGroups: [],
},
state,
mutations,
getters,
actions
})
}
4 changes: 3 additions & 1 deletion settings/src/store/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import users from './users';
import apps from './apps';
import settings from './settings';
import oc from './oc';
import security from './admin-security';

Vue.use(Vuex)

Expand All @@ -49,7 +50,8 @@ export default new Vuex.Store({
users,
apps,
settings,
oc
oc,
security
},
strict: debug,

Expand Down