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

return group members #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

mtakeda15
Copy link

Why?

To be available in "Application Gallery (Azure AD)", we have to support return group members.

What?

return group members

@mtakeda15
Copy link
Author

mtakeda15 commented Apr 14, 2023

Azure AD の アプリケーションギャラリーで利用可能になるためには、少なくともGroups に対する GET でパラメータに「attributes=members」が指定されていたら、membersを付けて返す必要があると指摘されています。
(ユーザをグループに追加したあとに、追加出来たことを確認出来るようにするため)

本PRではひとまず、全てのリクエストに対して members を返す実装をしていますが、後に以下の対応をしようと考えています。
1:patch, put では 204 No Content を返す
2:excluded_attributes=members が指定されたときは members が返らないようにする
これらはいずれもAzureADのドキュメント上で推奨されている内容です。

本PRではmembersの返し方が適切かを見て頂きたいです。

@mtakeda15 mtakeda15 requested a review from valerauko April 14, 2023 06:54
@mtakeda15 mtakeda15 self-assigned this Apr 14, 2023
app/controllers/concerns/scimaenaga/response.rb Outdated Show resolved Hide resolved
app/controllers/concerns/scimaenaga/response.rb Outdated Show resolved Hide resolved
def group_object_response(object)
response = find_value(object, Scimaenaga.config.group_schema.except(:members))
members_attribute = Scimaenaga.config.group_schema[:members]
return if members_attribute.nil?

Choose a reason for hiding this comment

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

こいつがnil返すとどうなる?

テストがほしいです...

Copy link
Author

Choose a reason for hiding this comment

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

return response するのを忘れてました・・・。
テスト作成します。

app/controllers/concerns/scimaenaga/response.rb Outdated Show resolved Hide resolved
find_value(object, Scimaenaga.config.user_schema)
end

def group_object_response(object)

Choose a reason for hiding this comment

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

なんでこういう実装になってるのか(なんでuserみたいにfind_valueだけじゃなくmembersを手動で作ってるのか)コメント書いた方がいいと思う

return if members_attribute.nil?

members_raw = find_value(object, members_attribute)
response[:members] = members_raw.map do |member|

Choose a reason for hiding this comment

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

objectはnilありえる?

ありえるならここヌルポになる

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