Skip to content

Fix broken resend email validation link #4002

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

Conversation

funkenstrahlen
Copy link
Contributor

This fixes issue: #3857 (Missing mount point in "invalid_verification_link.html")

The old approach was to use Javascript and work with regex on the url. This is a bad approach.

I therefore took another approach using templating. I discovered that changePassword in PublicAPURouter already uses (some kind of) templating. This is very basic but seems to work.

  changePassword(req) {
    return new Promise((resolve, reject) => {
      const config = new Config(req.query.id);
      if (!config.publicServerURL) {
        return resolve({
          status: 404,
          text: 'Not found.'
        });
      }
      // Should we keep the file in memory or leave like that?
      fs.readFile(path.resolve(views, "choose_password"), 'utf-8', (err, data) => {
        if (err) {
          return reject(err);
        }
        data = data.replace("PARSE_SERVER_URL", `'${config.publicServerURL}'`);
        resolve({
          text: data
        })
      });
    });
  }

I adopted this approach for invalidVerificationLink.

@flovilmart This change is currently untested and just a first POC. I would love to get your comment on this.

@funkenstrahlen
Copy link
Contributor Author

Also the spec needs to be fixed. I did some changes in the spec but I do not think it is correct yet.

@flovilmart
Copy link
Contributor

Hey @funkenstrahlen, this approach seems to be the best one, perhaps we should cache the file in memory, instead of reading from it all the times.

@funkenstrahlen
Copy link
Contributor Author

perhaps we should cache the file in memory, instead of reading from it all the times.

Do you think the increased memory footprint is worth it? This page will not be accessed very frequently. But I will leave this decision to you as you know more about parse server than I do.

How can I keep it in memory? Maybe just a global variable containing the parsed page which will be initialised on first access?

@codecov
Copy link

codecov bot commented Jul 24, 2017

Codecov Report

Merging #4002 into master will decrease coverage by 2.44%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4002      +/-   ##
==========================================
- Coverage   92.84%   90.39%   -2.45%     
==========================================
  Files         118      118              
  Lines        8381     8267     -114     
==========================================
- Hits         7781     7473     -308     
- Misses        600      794     +194
Impacted Files Coverage Δ
src/Routers/PublicAPIRouter.js 68.51% <66.66%> (-22.66%) ⬇️
src/Routers/LogsRouter.js 38.88% <0%> (-55.23%) ⬇️
src/Controllers/UserController.js 60.52% <0%> (-31.93%) ⬇️
src/Push/utils.js 80.32% <0%> (-17.95%) ⬇️
src/Push/PushWorker.js 83.6% <0%> (-9.5%) ⬇️
src/Routers/UsersRouter.js 84.37% <0%> (-8.91%) ⬇️
src/Controllers/PushController.js 89.47% <0%> (-8.31%) ⬇️
src/RestWrite.js 87.52% <0%> (-5.95%) ⬇️
src/Adapters/Auth/meetup.js 84.21% <0%> (-5.27%) ⬇️
... and 61 more

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 f0f1870...2beca12. Read the comment docs.

@funkenstrahlen
Copy link
Contributor Author

@flovilmart I implemented a first approach caching the template file in memory. It does not feel perfectly right to put this kind of code into the router file though. Also I am not an experienced JS developer. I am happy if you can take a look at my approach.

7602991

@flovilmart
Copy link
Contributor

@funkenstrahlen can you check, it seems there are some linting issues.

@funkenstrahlen
Copy link
Contributor Author

Will take a look at it someday next week

@@ -36,8 +36,7 @@ describe("Email Verification Token Expiration: ", () => {
request.get(sendEmailOptions.link, {
followRedirect: false,
}, (error, response) => {
expect(response.statusCode).toEqual(302);
expect(response.body).toEqual('Found. Redirecting to http://localhost:8378/1/apps/invalid_verification_link.html?username=testEmailVerifyTokenValidity&appId=test');
expect(response.body).toEqual('Found. Redirecting to http://localhost:8378/1/apps/invalid_verification_link');
Copy link
Contributor

Choose a reason for hiding this comment

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

why are those changed? That seems to be a breaking change

@funkenstrahlen
Copy link
Contributor Author

I fixed the syntax errors, but the test cases for invalid verification link pages do still fail.

@stale
Copy link

stale bot commented Sep 18, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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.

2 participants