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

Make triggerchallenge HTTP response consistent #1355

Merged
merged 4 commits into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@cornelinux
Copy link
Member

cornelinux commented Dec 21, 2018

The endpoints /validate/check and /validate/triggerchallenge
are able to trigger a challenge. They where not consisitent.
Now both endpoints return the same JSON, when a challenge is triggered.
If there are several C/R tokens, only ONE transaction_id is created,
which makes it easier for the calling application.

Closes #1353

Merging #1355 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
- Coverage   96.64%   96.64%   -0.01%
==========================================
Files         144      144
Lines       17291    17288       -3
==========================================
- Hits        16711    16708       -3
Misses        580      580
Impacted Files Coverage Δ
privacyidea/api/validate.py 100% <100%> (ø) ⬆️
privacyidea/lib/token.py 94.95% <86.2%> (+0.02%) ⬆️

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 42a210c...4169b9b. Read the comment docs.

  • OK, I now understand what this PR is doing and I think it is a sensible change :) Some points:
  • I think it would be nice to have some unit tests (1) to improve the diff coverage (2) and for the new /validate/triggerchallenge behavior in which only one transaction ID is generated. I can also look into this if you want.
  • I don't really like the structure of the JSON response that results from adding the challenge info to the detail dict:
    reply_dict.update(challenge_info)

    because the values of serial and attributes depend on the order of tokens in token_list, and I think we don't have a defined order there. But I see your point about backwards compatibility, so we can keep that.
  • We should decide whether to merge this into branch-2.23 or master. If we want to include this in a potential maintenance release, we could first merge it to branch-2.23 and merge branch-2.23 into master afterwards.
  • > * I think it would be nice to have some unit tests (1) to improve the diff coverage (2) and for the new /validate/triggerchallenge behavior in which only one transaction ID is generated. I can also look into this if you want.
    Will take a look at it.
  • We should decide whether to merge this into branch-2.23 or master. If we want to include this in a potential maintenance release, we could first merge it to branch-2.23 and merge branch-2.23 into master afterwards.
    I also thought about adding it to either a release 2.23.4 (2.24).
    Either by cherry picking or by merging the brach-2.23 back in master (Phew, not sure if this will work).
    As there are also some other changes, we might want to add to a 2.23.4 cherry picking might be attractive? Lets talk about this later.
  • In #e2f788a8 I added some tests for the code I touched. No tests for line I did not touch.
  •  File: privacyidea/lib/token.py:L1985-2042
  • Independent of merging this into master or 2.23, I want to keep this for now, for backward compatibility. We can not asure that all plugins will update the transaction_ids immediately.
  • OK, just one more thought: If I understand correctly, all challenges will now have the same transaction ID, e.g. for three challenges we have the following detail:
...
"transaction_ids": [
"04049536927188455149",
"04049536927188455149",
"04049536927188455149"
]
},

Should we keep this as it is, or should we modify transaction_id to only contain unique IDs? i.e.

...
"transaction_ids": [
"04049536927188455149"
]
},
  • "No" for the sake of backward compatibility ;-)
  •  File: privacyidea/lib/token.py:L1985-2042
  • Could we move this line to the very end of the function for the sake of clarity?
    Initially, I was confused because I thought this line overwrites the messages from the tokens at the beginning of token_list, but it actually doesn't, because new messages are appended to the end of message_list in each iteration.
  • I think we can do this.
    Will do.
  • done in 17bdb17
  •  File: privacyidea/lib/token.py:L1985-2042
  • Yes, will do.
  • done in 17bdb17
  •  File: privacyidea/lib/token.py:L1985-2042
  • I do not know. I will have to check this. Depends on the reset-pin policies.
  • Currently: If there are more tokens, then the pin-change information in the top level details branch will be the info of the first token.
    If we change it: the info in the top level branch will be this of the last token.
    But: If we change this, then the multi_challenge branch will contain the correct pin-change info for each token.
    So I think it makes sense to change this.
  • done in 6bd173a

  

Make triggerchallenge HTTP response consistent
The endpoints /validate/check and /validate/triggerchallenge
are able to trigger a challenge. They where not consisitent.
Now both endpoints return the same JSON, when a challenge is triggered.
If there are several C/R tokens, only ONE transaction_id is created,
which makes it easier for the calling application.

Closes #1353

@cornelinux cornelinux requested review from fredreichbier and privacyidea/core Dec 21, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #1355 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
- Coverage   96.64%   96.64%   -0.01%     
==========================================
  Files         144      144              
  Lines       17291    17288       -3     
==========================================
- Hits        16711    16708       -3     
  Misses        580      580
Impacted Files Coverage Δ
privacyidea/api/validate.py 100% <100%> (ø) ⬆️
privacyidea/lib/token.py 94.95% <86.2%> (+0.02%) ⬆️

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 42a210c...4169b9b. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #1355 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
+ Coverage   96.64%   96.69%   +0.04%     
==========================================
  Files         144      144              
  Lines       17291    17286       -5     
==========================================
+ Hits        16711    16714       +3     
+ Misses        580      572       -8
Impacted Files Coverage Δ
privacyidea/api/validate.py 100% <100%> (ø) ⬆️
privacyidea/lib/token.py 95.86% <100%> (+0.93%) ⬆️

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 42a210c...e2f788a. Read the comment docs.

challenge_info["password_change"] = \
token_list[0].is_pin_change(
password=True)
for k, v in challenge_info.items():

This comment has been minimized.

@fredreichbier

fredreichbier Jan 2, 2019

Member

I believe we could replace this with reply_dict.update(challenge_info). What do you think?

This comment has been minimized.

@cornelinux

cornelinux Jan 2, 2019

Member

Yes, will do.

This comment has been minimized.

@cornelinux

This comment has been minimized.

@fredreichbier
challenge_info["attributes"] = attributes
challenge_info["serial"] = token_obj.token.serial
# If exist, add next pin and next password change
next_pin = token_list[0].get_tokeninfo(

This comment has been minimized.

@fredreichbier

fredreichbier Jan 2, 2019

Member

Is it intentional that the lines handling the next PIN/password change refer to token_list[0] and not token_obj?

This comment has been minimized.

@cornelinux

cornelinux Jan 2, 2019

Member

I do not know. I will have to check this. Depends on the reset-pin policies.

This comment has been minimized.

@cornelinux

cornelinux Jan 2, 2019

Member

Currently: If there are more tokens, then the pin-change information in the top level details branch will be the info of the first token.
If we change it: the info in the top level branch will be this of the last token.

But: If we change this, then the multi_challenge branch will contain the correct pin-change info for each token.
So I think it makes sense to change this.

This comment has been minimized.

@cornelinux

This comment has been minimized.

@fredreichbier
for k, v in challenge_info.items():
reply_dict[k] = v
reply_dict["multi_challenge"].append(challenge_info)
# TODO: These two lines are deprecated: Add the information for the old administrative triggerchallenge

This comment has been minimized.

@fredreichbier

fredreichbier Jan 2, 2019

Member

As we merge this into master, i.e. for the next major release, couldn't we just omit these keys here?

Or should we rather merge this PR to branch-2.23 for the maintenance release?

This comment has been minimized.

@cornelinux

cornelinux Jan 2, 2019

Member

Independent of merging this into master or 2.23, I want to keep this for now, for backward compatibility. We can not asure that all plugins will update the transaction_ids immediately.
Won't change this.

This comment has been minimized.

@fredreichbier

fredreichbier Jan 3, 2019

Member

OK, just one more thought: If I understand correctly, all challenges will now have the same transaction ID, e.g. for three challenges we have the following detail:

    ...
        "transaction_ids": [
            "04049536927188455149",
            "04049536927188455149",
            "04049536927188455149"
        ]
    },

Should we keep this as it is, or should we modify transaction_id to only contain unique IDs? i.e.

    ...
        "transaction_ids": [
            "04049536927188455149"
        ]
    },

This comment has been minimized.

@cornelinux

cornelinux Jan 7, 2019

Member

"No" for the sake of backward compatibility ;-)

This comment has been minimized.

@fredreichbier
transactionid=transaction_id, options=options)
# Add the reply to the response
message_list.append(message)
reply_dict["message"] = ", ".join(message_list)

This comment has been minimized.

@fredreichbier

fredreichbier Jan 2, 2019

Member

Could we move this line to the very end of the function for the sake of clarity?

Initially, I was confused because I thought this line overwrites the messages from the tokens at the beginning of token_list, but it actually doesn't, because new messages are appended to the end of message_list in each iteration.

This comment has been minimized.

@cornelinux

cornelinux Jan 2, 2019

Member

I think we can do this.
Will do.

This comment has been minimized.

@cornelinux

This comment has been minimized.

@fredreichbier

cornelinux added some commits Jan 2, 2019

Return pin-change info per token
Previously only the pin change info of the
first token-object was returned.
@fredreichbier

This comment has been minimized.

Copy link
Member

fredreichbier commented Jan 3, 2019

OK, I now understand what this PR is doing and I think it is a sensible change :) Some points:

  • I think it would be nice to have some unit tests (1) to improve the diff coverage (2) and for the new /validate/triggerchallenge behavior in which only one transaction ID is generated. I can also look into this if you want.
  • I don't really like the structure of the JSON response that results from adding the challenge info to the detail dict:
    reply_dict.update(challenge_info)

    because the values of serial and attributes depend on the order of tokens in token_list, and I think we don't have a defined order there. But I see your point about backwards compatibility, so we can keep that.
  • We should decide whether to merge this into branch-2.23 or master. If we want to include this in a potential maintenance release, we could first merge it to branch-2.23 and merge branch-2.23 into master afterwards.
@cornelinux

This comment has been minimized.

Copy link
Member

cornelinux commented Jan 7, 2019

  • I think it would be nice to have some unit tests (1) to improve the diff coverage (2) and for the new /validate/triggerchallenge behavior in which only one transaction ID is generated. I can also look into this if you want.

Will take a look at it.

  • We should decide whether to merge this into branch-2.23 or master. If we want to include this in a potential maintenance release, we could first merge it to branch-2.23 and merge branch-2.23 into master afterwards.

I also thought about adding it to either a release 2.23.4 (2.24).
Either by cherry picking or by merging the brach-2.23 back in master (Phew, not sure if this will work).

As there are also some other changes, we might want to add to a 2.23.4 cherry picking might be attractive? Lets talk about this later.

@cornelinux

This comment has been minimized.

Copy link
Member

cornelinux commented Jan 7, 2019

In e2f788a I added some tests for the code I touched. No tests for line I did not touch.

@fredreichbier

This comment has been minimized.

Copy link
Member

fredreichbier commented Jan 8, 2019

Thanks!

@fredreichbier fredreichbier merged commit 7ff71f3 into master Jan 8, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.64%)
Details
codecov/project 96.69% (+0.04%) compared to 42a210c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@fredreichbier fredreichbier deleted the 1353/challenge-response branch Jan 8, 2019

fredreichbier added a commit that referenced this pull request Jan 8, 2019

Make triggerchallenge HTTP response consistent (#1355)
* Make triggerchallenge HTTP response consistent

The endpoints /validate/check and /validate/triggerchallenge
are able to trigger a challenge. They where not consisitent.
Now both endpoints return the same JSON, when a challenge is triggered.
If there are several C/R tokens, only ONE transaction_id is created,
which makes it easier for the calling application.

Closes #1353

* Add review comments for better readability

* Return pin-change info per token

Previously only the pin change info of the
first token-object was returned.

* increase diff coverage

cornelinux added a commit that referenced this pull request Jan 8, 2019

Merge pull request #1366 from privacyidea/1353/backport-challenge-res…
…ponse

Backport: Make triggerchallenge HTTP response consistent (#1355)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment