Skip to content

add events to check passwords with the password policy app - #221

Merged
MorrisJobke merged 3 commits into
masterfrom
password_policy_events
Jun 27, 2016
Merged

add events to check passwords with the password policy app#221
MorrisJobke merged 3 commits into
masterfrom
password_policy_events

Conversation

@schiessle

@schiessleschiessle commented Jun 24, 2016

Copy link
Copy Markdown
Member

Send "validate password events" to allow the password policy app (https://github.com/nextcloud/password_policy) to validate the password.

@schiessleschiessle added the 2. developing Work in progress label Jun 24, 2016
@schiessleschiessle added this to the Nextcloud Next milestone Jun 24, 2016
@LukasReschke

Copy link
Copy Markdown
Member

Any need of a new hook over '\OC\Share', 'verifyPassword'?

I'm fine with adding this one here as public API as well. But then the old one should die, it's non-public API anyways.

@schiessle

schiessle commented Jun 27, 2016

Copy link
Copy Markdown
MemberAuthor

Any need of a new hook over '\OC\Share', 'verifyPassword'?

I would like to have this new event as some kind of "public API" that other apps can check their passwords as well. This already speaks against a hook in the OC namespace. Further the symphony events have some advantages. E.g. the password checker can throw a exception and you can handle it outside of the hook, while the old hook system swallows all exception.

Let's try to have clean implementations of new stuff.

@schiessle
schiessleforce-pushed the password_policy_events branch 2 times, most recently from 6fd0e4c to 95ababfCompareJune 27, 2016 09:53
@schiessleschiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 27, 2016
@schiessle

Copy link
Copy Markdown
MemberAuthor

Ready to review. You need the password policy app to test it: nextcloud/password_policy#1

Password policy should be checked for public links and for the database backend password change in the user management and password change in the personal settings

cc @LukasReschke@MorrisJobke@blizzz

@schiessleschiessle mentioned this pull request Jun 27, 2016
6 tasks
@LukasReschke

Copy link
Copy Markdown
Member

@schiessle Merge conflict 🙈

\OC_JSON::error();
}
} catch (HintException $e) {
return \OC_JSON::error(['data' => ['message' => $e->getHint()]]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's a void method. No return required.

@schiessle
schiessleforce-pushed the password_policy_events branch from 95ababf to 79107baCompareJune 27, 2016 12:07
@schiessle
schiessleforce-pushed the password_policy_events branch from 79107ba to 2a990a0CompareJune 27, 2016 12:08
@schiessle

Copy link
Copy Markdown
MemberAuthor

@LukasReschke merge conflict resolved.

@LukasReschke

Copy link
Copy Markdown
Member

LGTM 👍

@MorrisJobke

Copy link
Copy Markdown
Member

Tested and works 👍

@MorrisJobke
MorrisJobke merged commit ed25d73 into masterJun 27, 2016
@MorrisJobke
MorrisJobke deleted the password_policy_events branch June 27, 2016 14:55
@MorrisJobke

Copy link
Copy Markdown
Member

Backport? cc @karlitschek

@karlitschek

Copy link
Copy Markdown
Member

please backport 👍

@schiessle

Copy link
Copy Markdown
MemberAuthor

backport for 9.0.52 #240

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@schiessle@LukasReschke@MorrisJobke@karlitschek