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

Disallow Updating of Federated User's Passwords #444

Merged
merged 9 commits into from
Feb 26, 2018

Conversation

chakrabortyr
Copy link
Contributor

@chakrabortyr chakrabortyr commented Feb 23, 2018

Description

As above.

Old:
screenshot from 2018-02-26 09-40-28

New:
screenshot from 2018-02-26 09-46-05

Motivation and Context

Ben's comments on testing.

Tests performed

Yeah.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project as found in the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@chakrabortyr chakrabortyr requested a review from plessbd February 23, 2018 18:42
@@ -544,9 +544,11 @@ public static function getUserByID($uid, &$targetInstance = NULL)

public function setPassword($raw_password)
{
if ($this->getUserType() === FEDERATED_USER_TYPE){
throw new Exception("Permission Denied. Only local accounts may have their passwords modified.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This message does not imply what the condition is checking.

"Federated users are not allow to have their passwords modified."

would probably be better :)

listeners: {
blur: XDMoD.utils.trimOnBlur,
focus: removeFieldHighlight,
invalid: function (thisField/* , msg */) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Either remove the commented code or pass the parameter since it is in the function spec.

Copy link
Contributor

@plessbd plessbd Feb 26, 2018

Choose a reason for hiding this comment

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

Leave function arguments:
ubccr/xdmod-qa@ff9ae2e

msgTarget: 'under',
submitValue: false,

validator: function (/* value */) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Either remove the commented code or pass the parameter since it is in the function spec.

var decodedSuccess;
if (success) {
data = CCR.safelyDecodeJSONResponse(response);
decodedSuccess = CCR.checkDecodedJSONResponseSuccess(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicky, but shouldn't this be named "decodedSuccessResponse"?

@smgallo smgallo added this to the v7.5.0 milestone Feb 26, 2018
@plessbd
Copy link
Contributor

plessbd commented Feb 26, 2018

After talking with smgallo, we have decided to update the linting rules (ubccr/xdmod-qa@ff9ae2e) to allow unused function arguments. These should stay in

plessbd
plessbd previously approved these changes Feb 26, 2018
@@ -544,9 +545,11 @@ public static function getUserByID($uid, &$targetInstance = NULL)

public function setPassword($raw_password)
{
if ($this->getUserType() === FEDERATED_USER_TYPE){
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a space between closing parens and left brace.

@chakrabortyr chakrabortyr merged commit 805c793 into ubccr:xdmod7.5 Feb 26, 2018
@chakrabortyr chakrabortyr deleted the RC_DisableFederatedPWUpdate branch February 26, 2018 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants