Make user search GDPR compliant #927

eljulians · 2020-04-16T08:19:05Z

The username field is deprecated and Jira is gradually removing it
from the cloud instances. This is the second time such changes break
our integrations; until now, our workaround consisted of first searching
for the user based on the email, and then use the account id from the
response for the other requests. But now we cannot search anymore for
users based on the email, and we need to use the query field. Even if
we just pass the same exact value we passed to username.

I can imagine there're several places in the code that would require of
being changed for being completely GDPR compliant, but I have no time
at the moment to fix all of them :(

Thanks for your work!

eliaschneider · 2020-04-16T16:00:56Z

LGTM, A great pull request!

eliaschneider

LGTM

eljulians · 2020-04-16T16:09:55Z

@eliaschneider Many thanks for such a quick feedback!

Although, I've just noticed the failing Travis build, and from the error message, looks like we won't be able to make it backwards compatible keeping the username field because it's complaining specifically about that: https://travis-ci.com/github/pycontribs/jira/jobs/319921094

How should we proceed here? I'm seeing that the recent PRs that got merged also had failed checks, but the merge is currently blocked. Do you manually sign off on these checks to allow the merges?

jira/client.py

darlev91 · 2020-04-21T12:36:41Z

jira/client.py

+        if user:
+            params["username"] = user


@julenpardo Thanks for your efforts on this stuff! We are looking forward for it will be merged! ❤️
Just one question about changes, isn't it redundant to add this check if the username parameter is already in params dictionary ?

jira/jira/client.py

Line 2843 in b5ef528

"username": user,

Hey @darlev91, thanks for your feedback! You're absolutely right, thanks for the catch! I'll push the change asap

@darlev91 I've already made the changes, thanks again for the feedback!

@julienadriano Thanks a lot!
What's about query parameter ? It is compatible with both JIRA API versions. I think it's better to keep things simple. Have you had a chance to check this comment?
#927 (comment)

@darlev91 is it? This is actually what @ngenovictor pointed out above. So we can just use the query parameter right away and keep it backwards compatible?

I did it this way just to be 100% sure that it was compatible with both versions, as I can only test it with the latest one, so I would not be sure about replacing the username parameter. But if that's the case, then I can just apply @ngenovictor's suggestion

@julienadriano yes, if checking JIRA API docs, it says that query parameter is required (can contain not only emailAddress as a value, and displayName too), but it shouldn't replace username because it is separate one:
Link on JIRA API v3
Link on JIRA API v2
I've checked it already and it really works fine for both API versions, so I think it will be better to add it directly in parameters like separate parameter that should be send on API.

@julienadriano @ngenovictor I think this point of view make sense in this case:

def search_users(self, query, username=None, startAt=0, maxResults=50, includeActive=True, includeInactive=False): ... params = { "query": query, "username": username, "includeActive": includeActive, "includeInactive": includeInactive, } ...

In this case the client code shouldn't be broken if it uses displayName or emailAddress as a query
What do you think about it, guys ?

@darlev91 Makes sense to me, I've already pushed the changes

studioj · 2021-05-14T00:53:40Z

likely a duplicate of #940

studioj · 2021-05-14T00:57:04Z

@julenpardo please black jira/client.py and merge in changes from master

eljulians · 2021-05-14T08:58:26Z

@studioj Thanks for the feedback, I've already pushed the changes, is there something I should do now to trigger the checks?

studioj · 2021-05-14T13:42:15Z

im not too aquianted with jira api but as stated in above review => why add query, if you can query for user ?

i think PR https://github.com/pycontribs/jira/pull/940/files suggests to do this ... would that be ok? or is there a reason not to?

eljulians · 2021-05-14T14:42:17Z

@studioj I probably did it that way to still keep the other parameter for backward compatibility, as the change appeared to be a canary release affecting only some servers

I'm fine with either approach; I did it this way back then in a rush to fix broken production systems :)

The thing is that we pinned the changes made on my branch as dependency, and it's been that way since the PR was open, so I'm aware that change may break some stuff. Let me test this on Monday

adehad · 2021-05-14T19:06:19Z

Checking the documentation of

Self-hosted:
https://docs.atlassian.com/software/jira/docs/api/REST/8.13.6/#api/2/user-findUsers
query is not present

Cloud:
https://developer.atlassian.com/cloud/jira/platform/rest/v2/api-group-user-search/#api-rest-api-2-user-search-get
query and username are present

It seems that query is only really available on Cloud. That would explain the current failures seen on #940 - no users are returned according to the logs:

That also explains why this PR passes those same checks.

It seems like the API doesn't have a problem with passing in incorrect parameters, as discussed in other threads on this PR. So I'm happy approving the approach for this PR.

jira/client.py

The `username` field is deprecated and Jira is gradually removing it from the cloud instances. This is the second time such changes break our integrations; until now, our workaround consisted of first searching for the user based on the email, and then use the account id from the response for the other requests. But now we cannot search anymore for users based on the email, and we need to use the `query` field. Even if we just pass the same exact value we passed to `username`. I can imagine there're several places in the code that would require of being changed for being completely GDPR compliant, but I have no time at the moment to fix all of them :( Thanks for your work!

The payload object was already initialized with the `username` value, so there's no need to check whether is specified or not.

`query` is a different parameter from `username` so it shouldn't replace it actually. Tested with v3 API and it's working.

Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

eljulians · 2021-05-17T09:23:02Z

Many thanks @adehad! I've already committed the suggestion and rebased the head branch with latest master.

adehad

just a few minor comments on the docstring and the typehint docstring format, glad you managed to get through the rebase alright, it was quite a hefty one !

jira/client.py

Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

eljulians · 2021-05-17T16:24:17Z

@adehad applied, thank you!

eljulians · 2021-05-18T08:13:46Z

Thanks @adehad for your patience and for approving, what's the process now to merge the PR?

adehad · 2021-05-18T09:00:50Z

@julenpardo as I've approved I've requested a review from @ssbarnea who has the permissions for the merge.

We have a somewhat weekly cadence for merging stuff in, most happens on the weekend. But I'm sure this time it won't take a year !

eljulians · 2021-05-18T12:56:33Z

Many thanks guys!

adehad · 2021-05-18T17:59:28Z

and thank you @julenpardo for your patience ! Hope you contribute in the future too !

lucasolinden · 2021-06-24T15:29:20Z

When will this be included in a proper PIP release?

* Make user search GDPR compliant The `username` field is deprecated and Jira is gradually removing it from the cloud instances. This is the second time such changes break our integrations; until now, our workaround consisted of first searching for the user based on the email, and then use the account id from the response for the other requests. But now we cannot search anymore for users based on the email, and we need to use the `query` field. Even if we just pass the same exact value we passed to `username`. I can imagine there're several places in the code that would require of being changed for being completely GDPR compliant, but I have no time at the moment to fix all of them :( Thanks for your work! * Remove redundant check The payload object was already initialized with the `username` value, so there's no need to check whether is specified or not. * Init payload iwth both username and query `query` is a different parameter from `username` so it shouldn't replace it actually. Tested with v3 API and it's working. * Update docblock regarding Jira cloud parameters Co-authored-by: adehad <26027314+adehad@users.noreply.github.com> * Fix optional `user` param * Update docblock Co-authored-by: adehad <26027314+adehad@users.noreply.github.com> * Update docblock Co-authored-by: adehad <26027314+adehad@users.noreply.github.com> * Update docblock Co-authored-by: adehad <26027314+adehad@users.noreply.github.com> Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

adehad · 2021-11-10T09:18:36Z

FYI @lucasolinden (and others following) this is now included in the pre-release pip install jira==3.1.0rc1 - expect a proper release soon.

ssbarnea · 2021-11-10T10:11:24Z

@adehad I just wanted to mention that you have all my support to tag a full release whenever you think is ok. I liked the feedback I seen so far and once we have that release up it should be easier to make new ones w/o having to do pre-releases. If we release often, the need for a pre-release goes down. Thanks for reviving maintenance on jira library!

eliaschneider previously approved these changes Apr 16, 2020

View reviewed changes

ngenovictor reviewed Apr 17, 2020

View reviewed changes

jira/client.py Show resolved Hide resolved

darlev91 reviewed Apr 21, 2020

View reviewed changes

eljulians dismissed eliaschneider’s stale review via b328c3e April 22, 2020 07:21

adehad added the bug label May 2, 2021

adehad mentioned this pull request May 2, 2021

GDPR Strict Mode Fix #940

Closed

adehad mentioned this pull request May 12, 2021

Any plan for the official 3.0 release ? #967

Closed

studioj closed this May 13, 2021

studioj reopened this May 13, 2021

studioj added the duplicate label May 14, 2021

studioj mentioned this pull request May 14, 2021

Fixed search users to use query instead of user. #941

Closed

eljulians force-pushed the julenpardo/gdpr-compliant-user-search branch from 241daa3 to 23ace30 Compare May 14, 2021 08:50

adehad requested changes May 14, 2021

View reviewed changes

jira/client.py Outdated Show resolved Hide resolved

eljulians and others added 4 commits May 17, 2021 11:00

Remove redundant check

b401c5f

The payload object was already initialized with the `username` value, so there's no need to check whether is specified or not.

Init payload iwth both username and query

5a0d1ec

`query` is a different parameter from `username` so it shouldn't replace it actually. Tested with v3 API and it's working.

Update docblock regarding Jira cloud parameters

8abc16f

Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

eljulians force-pushed the julenpardo/gdpr-compliant-user-search branch from b69bad7 to 8abc16f Compare May 17, 2021 09:01

eljulians requested a review from adehad May 17, 2021 11:21

Fix optional user param

f42acd3

eljulians force-pushed the julenpardo/gdpr-compliant-user-search branch from fd1bf01 to f42acd3 Compare May 17, 2021 14:54

adehad requested changes May 17, 2021

View reviewed changes

jira/client.py Outdated Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

eljulians and others added 3 commits May 17, 2021 18:23

Update docblock

31b653e

Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

Update docblock

4985fe0

Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

Update docblock

8248d73

Co-authored-by: adehad <26027314+adehad@users.noreply.github.com>

adehad approved these changes May 17, 2021

View reviewed changes

adehad requested a review from ssbarnea May 18, 2021 09:00

ssbarnea merged commit b360dcc into pycontribs:master May 18, 2021

adehad removed the duplicate label May 18, 2021

adehad mentioned this pull request May 18, 2021

search_users - allow search by query #924

Closed

adehad linked an issue May 18, 2021 that may be closed by this pull request

search_users - allow search by query #924

Closed

adehad mentioned this pull request May 30, 2021

Unable to log in with basic-auth due to GDPR mode #935

Closed

nelli-acc mentioned this pull request Aug 5, 2021

Make search_assignable_users_for_issues function GDPR compliant. #1117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make user search GDPR compliant #927

Make user search GDPR compliant #927

eljulians commented Apr 16, 2020

eliaschneider commented Apr 16, 2020

eliaschneider left a comment

eljulians commented Apr 16, 2020

darlev91 Apr 21, 2020

eljulians Apr 21, 2020

eljulians Apr 22, 2020

darlev91 Apr 22, 2020 •

edited

Loading

eljulians Apr 22, 2020

darlev91 Apr 22, 2020

darlev91 Apr 22, 2020 •

edited

Loading

eljulians Apr 23, 2020

studioj commented May 14, 2021

studioj commented May 14, 2021

eljulians commented May 14, 2021

studioj commented May 14, 2021 •

edited

Loading

eljulians commented May 14, 2021

adehad commented May 14, 2021 •

edited

Loading

eljulians commented May 17, 2021

adehad left a comment

eljulians commented May 17, 2021

eljulians commented May 18, 2021

adehad commented May 18, 2021

eljulians commented May 18, 2021

adehad commented May 18, 2021

lucasolinden commented Jun 24, 2021

adehad commented Nov 10, 2021

ssbarnea commented Nov 10, 2021

Make user search GDPR compliant #927

Make user search GDPR compliant #927

Conversation

eljulians commented Apr 16, 2020

eliaschneider commented Apr 16, 2020

eliaschneider left a comment

Choose a reason for hiding this comment

eljulians commented Apr 16, 2020

darlev91 Apr 21, 2020

Choose a reason for hiding this comment

eljulians Apr 21, 2020

Choose a reason for hiding this comment

eljulians Apr 22, 2020

Choose a reason for hiding this comment

darlev91 Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

eljulians Apr 22, 2020

Choose a reason for hiding this comment

darlev91 Apr 22, 2020

Choose a reason for hiding this comment

darlev91 Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

eljulians Apr 23, 2020

Choose a reason for hiding this comment

studioj commented May 14, 2021

studioj commented May 14, 2021

eljulians commented May 14, 2021

studioj commented May 14, 2021 • edited Loading

eljulians commented May 14, 2021

adehad commented May 14, 2021 • edited Loading

eljulians commented May 17, 2021

adehad left a comment

Choose a reason for hiding this comment

eljulians commented May 17, 2021

eljulians commented May 18, 2021

adehad commented May 18, 2021

eljulians commented May 18, 2021

adehad commented May 18, 2021

lucasolinden commented Jun 24, 2021

adehad commented Nov 10, 2021

ssbarnea commented Nov 10, 2021

darlev91 Apr 22, 2020 •

edited

Loading

darlev91 Apr 22, 2020 •

edited

Loading

studioj commented May 14, 2021 •

edited

Loading

adehad commented May 14, 2021 •

edited

Loading