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

Fix reauthorization to work beyond the first time called #128

Merged
merged 3 commits into from
Oct 8, 2022

Conversation

timharsch
Copy link
Contributor

See also PR #127 and issue #112

This PR fixes a situation where the refresh_token is not properly address on calls beyond the first.

@timharsch
Copy link
Contributor Author

@ducu Can this PR be merged please?

@ducu
Copy link
Collaborator

ducu commented Sep 28, 2022

Hey @timharsch, no it cannot be merged like this.
Please align your changes with the upstream master first, here's the conflict I see:

<<<<<<< HEAD
        # The refresh token issued by the authorization server. If present, the
        # app should discard any previous refresh_token associated with this
        # launch, replacing it with this new value.
        refresh_token = ret_params.get('refresh_token')
        if refresh_token is not None:
            self.refresh_token = refresh_token
            del ret_params['refresh_token']
        
        logger.debug("SMART AUTH: Received access token: {0}, refresh token: {1}"
=======
        self.refresh_token = ret_params.get('refresh_token') or params.get('refresh_token')
        if self.refresh_token is not None and 'refresh_token' in ret_params:
            del ret_params['refresh_token']

        logging.debug("SMART AUTH: Received access token: {0}, refresh token: {1}"
>>>>>>> c4f02e92a255ed09e2711f80e3b1d0ddd525589a

@timharsch
Copy link
Contributor Author

Thanks for your reply.

I don't understand where you are seeing a conflict. The PR would state if there were a conflict and not allow a merge. Currently it is just blocked waiting on your review, once you review the PR it should allow you to merge.

If you click on this link you can see a comparison of master and my branch. There are no conflicts.. it's just a few lines of change.
master...Unite-Genomics:client-py:unite-112

If you examine the git history of my branch you will see it is just the two commits on top of the head of the current master of this repository.
https://github.com/Unite-Genomics/client-py/commits/unite-112

fhirclient/auth.py Outdated Show resolved Hide resolved
@timharsch
Copy link
Contributor Author

Hi @ducu I've now done some fairly extensive testing with the latest commit. I think it is safe to merge now. Can you merge it please?

Copy link
Collaborator

@ducu ducu left a comment

Choose a reason for hiding this comment

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

Approved, thank you.

@ducu ducu merged commit 38366f7 into smart-on-fhir:master Oct 8, 2022
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