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

Use rawurldecode for allowing "+" in guests emails #384

Merged
merged 4 commits into from
Mar 4, 2020

Conversation

pako81
Copy link

@pako81 pako81 commented Dec 18, 2019

Description

Use rawurldecode instead of urldecode for allowing + in guests emails since urldecode decodes also + symbol to a space character.

Related Issue

Motivation and Context

Since urldecode decodes also + symbol to a space character it is not possible to create guest users having i.e. the following format [email protected], which is a perfectly fine format.

How Has This Been Tested?

Manually by sharing with guest users having the [email protected] format over the sharing dialogue.

Acceptance tests scenarios have been added - see 2nd commit.

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.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@pako81 pako81 self-assigned this Dec 18, 2019
@pako81 pako81 added this to the development milestone Dec 18, 2019
@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #384 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #384   +/-   ##
=========================================
  Coverage     17.85%   17.85%           
  Complexity       96       96           
=========================================
  Files            11       11           
  Lines           532      532           
=========================================
  Hits             95       95           
  Misses          437      437
Impacted Files Coverage Δ Complexity Δ
lib/Controller/UsersController.php 0% <0%> (ø) 10 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ceb68cb...0bd4d32. Read the comment docs.

@pako81 pako81 requested a review from phil-davis January 3, 2020 22:35
@phil-davis phil-davis self-assigned this Jan 9, 2020
@phil-davis
Copy link
Contributor

Assigned myself so I see this tomorrow and add some test scenario(s) for a guest with +
Also there are a couple of other guest acceptance test PRs to be merged tomorrow morning - so I will rebase after that...

@phil-davis
Copy link
Contributor

phil-davis commented Jan 31, 2020

Rebased just now. The code in core master is getting closer to 10.4 happening, so time to look at this again.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@phil-davis phil-davis merged commit 3d8abc0 into master Mar 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the validate-guests-emails branch March 4, 2020 07:00
@phil-davis
Copy link
Contributor

@HanaGemela @micbar a guests app release is needed.
IMO that release should set core min-version to 10.4
Otherwise sites could install the new guests app on a core 10.3.2 system, the guests app would allow + in a guest email/username but core would reject a username like that, and something might go 💥

@HanaGemela HanaGemela mentioned this pull request Mar 4, 2020
31 tasks
@HanaGemela HanaGemela modified the milestones: development, QA Mar 9, 2020
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.

password change test fails with stable10
4 participants