Skip to content

Comments

SQSERVICES 1099 Public API end-point for re-sending email validation#1948

Merged
battermann merged 25 commits intodevelopfrom
SQSERVICES-231-public-endpoint-for-resending-validation-emails
Dec 3, 2021
Merged

SQSERVICES 1099 Public API end-point for re-sending email validation#1948
battermann merged 25 commits intodevelopfrom
SQSERVICES-231-public-endpoint-for-resending-validation-emails

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Nov 29, 2021 •
edited
Loading

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If HTTP endpoint paths have been added or renamed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.
    • If new config options introduced: added usage description under docs/reference/config-options.md
    • If new config options introduced: recommended measures to be taken by on-premise instance operators.
    • If a cassandra schema migration is backwards incompatible (see also these docs), measures to be taken by on-premise instance operators are explained.
    • If a data migration (not schema migration) introduced: measures to be taken by on-premise instance operators.
    • If public end-points have been changed or added: does nginz need un upgrade?
    • If internal end-points have been added or changed: which services have to be deployed in a specific order?

@battermann battermann marked this pull request as draft November 29, 2021 16:46
@battermann battermann requested a review from fisx November 29, 2021 16:47
fisx
fisx requested changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Good progress! 👍

@fisx
Copy link
Contributor

fisx commented Nov 30, 2021

(ci failure seems unrelated)

Leif Battermann and others added 6 commits November 30, 2021 14:02
@battermann
Copy link
Contributor Author

battermann commented Nov 30, 2021

TODO: Integration tests.

battermann
battermann commented Nov 30, 2021
View reviewed changes
fisx
fisx reviewed Dec 1, 2021
View reviewed changes
@battermann battermann requested a review from fisx December 1, 2021 22:32
fisx
fisx reviewed Dec 1, 2021
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

All but 3 of these comments are just because it's late and I'm chatty, hope that's not too annoying. I think you're close! Let's see if concourse agrees.

services/brig/src/Brig/Data/User.hs
where
select :: PrepQuery R (Identity UserId) (Identity (Maybe ApiFt.TeamFeatureStatusValue))
select = fromString $ "select feature_conference_calling from user where id = ?"
select = fromString "select feature_conference_calling from user where id = ?"
Copy link
Contributor

@fisx fisx Dec 1, 2021

Choose a reason for hiding this comment

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

Ideally, these changes would go into a separate PR.

Copy link
Contributor Author

@battermann battermann Dec 2, 2021

Choose a reason for hiding this comment

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

I know, but this makes the barrier for improving the code on the fly much higher.

@fisx
Copy link
Contributor

fisx commented Dec 1, 2021

Ah, and can you try and complete the checkbox list? In the next pairing session we can go through the things you'd prefer to do together before anything else.

Leif Battermann and others added 4 commits December 2, 2021 08:42
@fisx
better comment
69848bb 
Co-authored-by: fisx <mf@zerobuzz.net>
@fisx
clean up
abc3938 
Co-authored-by: fisx <mf@zerobuzz.net>
@fisx
removed unused function
18a749b 
Co-authored-by: fisx <mf@zerobuzz.net>
@battermann
pr review issues fixed
cab6d98
@battermann battermann marked this pull request as ready for review December 2, 2021 09:54
@battermann battermann requested a review from fisx December 2, 2021 09:54
@battermann battermann force-pushed the SQSERVICES-231-public-endpoint-for-resending-validation-emails branch from 908ec1a to e2b8d82 Compare December 2, 2021 10:06
@battermann battermann force-pushed the SQSERVICES-231-public-endpoint-for-resending-validation-emails branch from e2b8d82 to 1743a06 Compare December 2, 2021 12:50
fisx
fisx approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

LGTM! I have two more change requests, but neither is essential, so I've (pre-)approved this PR and leave you with deciding if you think you should follow up on them.

services/brig/test/integration/API/User/Account.hs
-- and then assert that if the email is verified there is no new activation code created
-- when the set email function is called again (idempotency)
checkSetUserEmail teamOwner emailOwner newEmail 200
checkUnauthorizedRequests emailOwner otherTeamMember teamOwnerDifferentTeam newEmail
Copy link
Contributor

@fisx fisx Dec 2, 2021

Choose a reason for hiding this comment

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

Can we remove this line? ff not, can you add a comment helping me to find out what this is testing?

Copy link
Contributor Author

@battermann battermann Dec 2, 2021

Choose a reason for hiding this comment

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

Line 168: Ideally if the email has been verified the team owner can call the endpoint again and nothing will happen (no new activation code will be generated). In order to test this specifically we would have to wait until the activation code expires. This is too costly for the test, but at least we can test that the request still responds with status code 200 in this case.

Line: 169: Similar, if the request is made with insufficient permissions, the responses should be the same as before (regardless of the state of the activation)

Copy link
Contributor Author

@battermann battermann Dec 2, 2021

Choose a reason for hiding this comment

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

Maybe these are not the most useful tests, but they don't hurt.

Copy link
Contributor Author

@battermann battermann Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As discussed in another conversation, I added the expiration of the token again, so this conversation is obsolete, I think.

Leif Battermann and others added 3 commits December 2, 2021 14:26
@fisx
better changelog
37add87 
Co-authored-by: fisx <mf@zerobuzz.net>
@fisx
update comment
dc6ff6b 
Co-authored-by: fisx <mf@zerobuzz.net>
@battermann
let token expire for a more useful tests
ca6e70e
@battermann battermann requested a review from fisx December 2, 2021 13:51
@fisx
Copy link
Contributor

fisx commented Dec 3, 2021 

#9 1351.2 Copied executables to /wire-server/dist:
#9 1351.2 - api-loadtest
#9 1351.2 - api-smoketest
[...]
#9 1351.2 - stern
#9 1351.2 - zauth
#9 1351.2
#9 1351.2 Warning: Installation path /wire-server/dist not found on the PATH environment variable.
#9 DONE 1360.5s
[...]
#13 exporting to oci image format
#13 sha256:69a2560eef4d3ece902c3b5149d142e9bd132f25db0bc9e35b94201534c415d2
#13 exporting layers
#13 exporting layers 92.7s done
#13 exporting manifest sha256:a1159bd4b920fd682b9ccef8f31c9c69b7632a8d2d0f18dec909c0b736e5adff 0.0s done
#13 exporting config sha256:6440c8a581fb380640fd6ad9d9c4cc058f4b20d7a92c4dd56afad7f41897b1fa 0.0s done
#13 ERROR: no active session for 7hrhca6ne62i9uyj4ksy826la: context deadline exceeded
------
 > exporting to oci image format:
------
error: failed to solve: rpc error: code = DeadlineExceeded desc = no active session for 7hrhca6ne62i9uyj4ksy826la: context deadline exceeded
FATA[1653] failed to build: build: exit status 1        
FATA[1653] failed to run task: exit status 1

not sure where this rpc error is coming from, but probably not from a compilation error?

@battermann battermann merged commit cab62c1 into develop Dec 3, 2021
@battermann battermann deleted the SQSERVICES-231-public-endpoint-for-resending-validation-emails branch December 3, 2021 12:49
@sysvinit sysvinit mentioned this pull request Dec 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fisx fisx fisx approved these changes

@pcapriotti pcapriotti Awaiting requested review from pcapriotti

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@battermann @fisx @pcapriotti