Skip to content
This repository has been archived by the owner on Oct 23, 2019. It is now read-only.

Record all signup attempts in DB #418

Closed
wants to merge 11 commits into from
Closed

Conversation

fhartwig
Copy link
Contributor

This is a sort of work-in-progress on https://tree.taiga.io/project/fisx-thentos/task/44
It could probably need some more tests and I'm still working on CSV output for the new api endpoint, but apart from that I think it's ready for review.

@fisx fisx self-assigned this Dec 15, 2015
getAllSignupAttempts :: ThentosQuery e [SignupAttempt]
getAllSignupAttempts = map (\(n, cc, t) -> SignupAttempt n cc t) <$>
queryT [sql| SELECT user_name, captcha_correct, timestamp FROM signup_attempts |] ()

Copy link
Collaborator

Choose a reason for hiding this comment

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

missing empty line.

@fisx
Copy link
Collaborator

fisx commented Dec 15, 2015

otherwise looks good!

@fisx fisx assigned fhartwig and unassigned fisx Dec 15, 2015
@ChristianSi
Copy link
Contributor

Though I notice it's a requirement, I'm very much against adding a publicly accessible endpoint (registration_attempts) revealing such sensitive information. Alternatives:

  • Don't make this part of the API at all, instead write a little Perl script that reads the DB table and converts it into csv format.
  • If part of the API, make sure that the endpoint is only accessible from certain configurable IPs (default: localhost).

@fisx
Copy link
Collaborator

fisx commented Dec 16, 2015

as agreed offline: let's go for the privileged-ip solution.

On Tue, Dec 15, 2015 at 10:02:38AM -0800, Christian Siefkes wrote:

Though I notice it's a requirement, I'm very much against adding a publicly accessible endpoint (registration_attempts) revealing such sensitive information. Alternatives:

  • Don't make this part of the API at all, instead write a little Perl script that reads the DB table and converts it into csv format.
  • If part of the API, make sure that the endpoint is only accessible from certain configurable IPs (default: localhost).

@fhartwig
Copy link
Contributor Author

I didn't get around to doing the IP authentication. I'm unassigning myself as I'll be away for the next 2 weeks. Apart from the authentication issue, I think this is good to review/merge.

@fhartwig fhartwig removed their assignment Dec 17, 2015
instance FromField Bool where
parseField "1" = pure True
parseField "0" = pure False
parseField _ = mzero
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fisx
Copy link
Collaborator

fisx commented Dec 31, 2015

What about using hs-logger for this? We could define a dedicated variant of logger that takes all the information on the event (signup attempt in this case), stores it to a dedicated Logger (which goes to a separate file), and then do all the processing of this file outside of the thentos server process?

Benefits:

  • @fhartwig, I suspect you might like that because it obsoletes the idea of using PrivilegedIP for authorization in the retrieval action (see Privileged IPs #420).
  • It would also be a lot more extensible: If we come accross some pattern of requests (either for thentos or for adhocracy) that we want to keep an eye on, we can easily and rapidly add them.
  • It would also be nice to be able to patch the analytics code independently of the production server.

@fhartwig
Copy link
Contributor Author

fhartwig commented Jan 6, 2016

Closing this in favour of #445

@fhartwig fhartwig closed this Jan 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants