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

Conversation

MorrisJobke
Copy link
Member

Resubmit of #13735

Fixes #12267

I haven't committed compiled assets yet.

cc @rummatee

rummatee added 2 commits July 18, 2019 21:20
Signed-off-by: Florian Schunk <florian.schunk@rwth-aachen.de>
Signed-off-by: Florian Schunk <florian.schunk@rwth-aachen.de>
}
}

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?

}

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!! 👍

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.

},
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);
}

@@ -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?

beforeMount(){
this.$store.dispatch('getAllApps');
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?

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

A few comments :)

@MorrisJobke
Copy link
Member Author

A few comments :)

Feel free to take over - I just untangled the git history.

@rullzer
Copy link
Member

rullzer commented Mar 30, 2021

I'm going to close this due to lack of activity.
Feel free to reopen if anybody wants to continue.

@rullzer rullzer closed this Mar 30, 2021
@rullzer rullzer deleted the issue12267 branch March 30, 2021 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn/check if any 2FA provider is active before enforcing 2FA
4 participants