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

Reimplement GetVariables based on new segments layer #53

Merged
merged 8 commits into from
Sep 23, 2019
Merged

Reimplement GetVariables based on new segments layer #53

merged 8 commits into from
Sep 23, 2019

Conversation

Philipp91
Copy link
Contributor

@Philipp91 Philipp91 commented Sep 22, 2019

This PR adds flexible parsing for HITANS segments version 1 through 6 and exposes the parsed segments with a common interface. This allows reimplementing GetVariables:parseTanModes() (which is a business logic layer function) without any interspersed parsing logic. If necessary, the common interface allows adding other HITANS versions without much thinking or danger of breaking other ones. The unit test remains unchanged besides plumbing.

@roben I'd appreciate your review, as you are probably most familiar with how HITANS works across the various banks. I'm not sure if your unit tests cover all the special cases that you considered when implementing the code. In particular, I see a comment about Sparkasse declaring version 6 and you suspecting that they send version 5, yet the unit tests still works with my HITANSv6 class -- I haven't even implemented HITANSv5.

@nemiah
Copy link
Owner

nemiah commented Sep 22, 2019

The changes look sane to me and I'd give them a go.

@roben What do you say?

@Philipp91
Copy link
Contributor Author

Philipp91 commented Sep 22, 2019

Found this in the specification:

B.5.1 Geschäftsvorfall HKTAN in Segmentversion #6
In der BPD können sich mehrere Segmentversionen von HITANSSegmenten befinden, wobei den einzelnen HITANS-Segmenten
über das Element „Sicherheitsfunktion, kodiert“ unterschiedliche
Verfahren zugeordnet sein können. Ein Kundenprodukt sollte – beginnend mit der höchsten Segmentversion – alle in der BPD enthaltenen HITANS-Segmente analysieren, um so dem Kunden alle vom
Kreditinstitut unterstützten Sicherheitsverfahren anbieten zu können.
Beispiel: Die BPD enthält Definitionen für HITANS#6 und
HITANS#5. In HITANS#6 gilt für starke Kundenauthentifizierung
analog PSD2, mit HKTAN#5 ist übergangsweise auch noch eine
Dialoginitialisierung ohne starke Authentifizierung möglich.

tl;dr: Need to read all HITANS versions, though any version less than 6 is no good for PSD2 authentication. That said, not everything needs that strong authentication nowadays, e.g. stock/broker functions do not. So I have now implemented the other HITANS versions as well and removed findLatestSegment() after all (it's still in commit f7d6bb0161a29ff6c3e231aa3a60c2f858bd3638 if anyone needs it).

This allows us to make parseTanModes() private, as it should be.
The new segment classes contain much more boilerplate, but no index arithmetic or magic numbers at all. The HITANSTest still succeeds without modifications.
This was referenced Sep 22, 2019
@roben
Copy link

roben commented Sep 23, 2019

Looks solid to me, too. Thanks for all the work @Philipp91!

@nemiah nemiah merged commit 6f118f2 into nemiah:master Sep 23, 2019
@Philipp91 Philipp91 deleted the newsegments branch September 28, 2019 18:16
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