Add "MFA Verified" check to workflowengine #37195
Conversation
szaimen
requested review from
nickvergessen,
juliushaertl,
a team,
ArtificialOwl,
icewind1991 and
blizzz
and removed request for
a team
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| $this->container->query(FileName::class), | ||
| $this->container->query(FileSize::class), | ||
| $this->container->query(FileSystemTags::class), | ||
| $this->container->query(MfaVerified::class), |
Check notice
Code scanning / Psalm
DeprecatedMethod
|
Any news on getting this merged? |
|
I'd feel the check would be more fitting in the SAML app since it is bound to the SAML implementation. I could imagine there are some troubles on getting it in there, so I'd also be OK with merging to server, however then we should make sure to name it properly in the UI so that its relation to SAML MFA is clear. Otherwise this might get confused with the Nextcloud-native two factor auth. |
If I am missing some context, I apologise, but this should work with native mfa as well and not only if mfa is picked up from saml, that was implemented in the changes in gss. The idea is that no matter the way that mfa verification is done, the workflow engine should know about it, so this should be agnostic and not only apply to saml. |
|
I was mainly referring to https://github.com/nextcloud/server/pull/37195/files#diff-324119d2db6f3d5a6d7546558dd8011e16e04683d83c4d29d69ff5f250f353d1R62 but might be missing some context there. If there are other sources for this flag to be set it would be good to have a code comment inline as well. I might be overseeing something but as far as I understand the passed over options by the GSS also originate from user_saml. Noting that user_saml is not exclusively for SAML but can also provide other SSO mechanisms maybe I should rather have asked for "SAML/SSO multi-factor authentication" as the user facing wording there. Would that be a better fit? |
|
Thanks for raising this @juliushaertl - the Check should actually be looking for 3 sources of MFA info as we tested in https://github.com/pondersource/nextcloud-mfa-awareness/blob/7011a58/mfachecker/lib/Controller/PageController.php#L33-L53 - we forgot to add all 3 of them as we moved the code into the Check. Sorry! Will be fixed in pondersource/nextcloud-mfa-awareness#54 and then it will no longer be SAML-only and no longer be a misnomer. :) |
|
@juliushaertl @nickvergessen Accepted change requests, Rebased and Ready to merge. |
|
It heavily conflicts at the moment it seems. can you rebase again? |
Signed-off-by: Michiel de Jong <michiel@unhosted.org>
michielbdejong
force-pushed
the
mfa-check-rebased
branch
from
fef6003
to
03d95d7
Compare
Signed-off-by: MohammadReza vahedi <34796044+mrvahedi68@users.noreply.github.com>
Signed-off-by: MohammadReza vahedi <34796044+mrvahedi68@users.noreply.github.com>
| } | ||
|
|
||
| if ($operator === 'is') { | ||
| return $mfaVerified === '1'; // checking whether the current user is MFA-verified |
There was a problem hiding this comment.
Can we make mfaVerified a boolean value otherwise this will not work with the two_factor_auth_passed check and seems more consistent if we do the actual value comparison in the above if-conditions if needed.
|
Continued in #37914 |
Rebase of #36045
This PR goes together with:
It allows the creation of work flows that depend on whether the logged-in user has passed MFA Verification for the current session or not.