move the user_id field to conditions

hackmod commented 5 years ago

user_id는 업데이트되는 field가 아니며, 다른 값으로 변경되어도 안됩니다. conditions으로 넣습니다.

XE는 grant all privileges를 가정하나, 좀 더 안전하게 만들려면 alter,drop는 revoke시키고, update는 특정 컬럼에 유효하게 고칠 수도 있을 것입니다.

간단히 xe_member에 대해서 user_id 및 member_srl에 대해서 update를 revoke시키고, 나머지 컬럼들만 grant update(...)한 후에 테스트를 해보니 이 문제때문에 제대로 작동을 하지 않았습니다.

kijin commented 5 years ago

대부분의 사이트에서는 아이디 변경을 허용하지 않지만, 사이트에 따라서는 아이디 변경을 허용하는 모듈을 만들어 쓸 수도 있습니다. 타 CMS와 달리 XE에서는 user_id가 아닌 member_srl로 회원을 식별하며 아이디는 로그인에 사용할 뿐이므로, user_id가 변경된다고 해서 특별히 문제가 될 일도 없고요.

hackmod commented 5 years ago

XE 기본 기능은 user_id 변경을 지원하지 않고 있습니다. 그렇다면 이에 맞게 기본 xml은 user_id변경이 되면 안되는 쿼리문을 가져야 맞겠죠. 모듈이 user_id 변경을 지원한다면 해당 모듈이 자체적으로 query문을 조금 고치고 그걸 쓰면 그만입니다.

kijin commented 5 years ago

기본으로 지원하지 않는 것과 허용하지 않는 것은 다릅니다. 서드파티 자료는 가능하면 코어에서 제공하는 함수와 쿼리를 활용하는 것이 좋습니다. 지금도 누군가는 해당 쿼리에서 user_id 변경을 허용하는 것을 이용하여 어떤 기능을 만들어 쓰고 있을지도 모릅니다. 코어는 확장 기능을 편리하게 만들어쓸 수 있도록 안정적인 기반이 되어 주어야 합니다. 10년 넘게 잘 되던 것이 갑자기 안 되면 @hackmod 님이 책임지고 고쳐주실 건가요?

현실적으로 버그를 일으키는 기능도 아니고 보안취약점도 아닌데, 님이 특수한 설정 하에서 사용하는 데 지장을 준다는 이유만으로 XE를 사용하는 다른 모든 사용자에게서 update 권한을 박탈하려고 하시면 곤란합니다.

게다가 member_srl만 condition으로 주어도 충분히 레코드를 특정할 수 있는데 user_id는 왜 condition으로 옮기셨는지도 의문이군요. user_id를 PK로 사용하는 다른 CMS의 사고방식을 XE에 끌고 오신 것이 아닌가 싶습니다. 심지어 notnull 조건이 붙어 있어서 user_id를 넣어주지 않으면 쿼리가 아예 실행되지 않습니다. 이건 개선이 아니라 개악이네요.

hackmod commented 5 years ago

이유없이 너무 공격적이신것 같네요?

XE는 코어에서 user_id 변경을 지원하지 않으니 그 의도에 맞게 user_id를 변경을 지원하는 쿼리문은 의도에 맞지 않다고 봤습니다.

updateMember.xml은 다음의 파일에서 사용되고 있으며, executeQuery('member.updateMember')는 여기서만 호출되고 있습니다. https://github.com/xpressengine/xe-core/blob/develop/modules/member/member.controller.php#L2440

user_id가 지정되지 않은 경우 다음과 같이 user_id가 세팅됩니다.

if(!$args->user_id) $args->user_id = $orgMemberInfo->user_id;

member_srl 를 이용해 유저정보를 다시 가져오기대문에 user_id는 항상 null이 아닙니다.

그리고 user_id를 손쉽게 변경이 가능하게 허락한 커뮤니티가 과연 얼마나 될까요? 현재 운용중인 커뮤니티는 7년째 XE를 사용중입니다.

현재 XE는 단순히 form상으로 id를 편집하지 못하게 disable시킨 상태이므로 사용자가 임의로 폼을 고쳐 user_id를 수정하는것이 가능한 것처럼 보입니다.

그러나 잘 살펴보면, 이렇게 사용자가 임의로 user_id를 고치는 것은 이미 막혀있습니다.

https://github.com/xpressengine/xe-core/blob/develop/modules/member/member.controller.php#L2364

(위 줄을 커멘트 처리하면 user_id를 임의로 바꾸는 것이 가능해짐) 따라서 xml 쿼리문상으로 고치더라도 전혀 문제가 되지 않는다고 보고 PR을 올린것입니다.

bjrambo commented 5 years ago

@hackmod 음.. 이미 https://xetown.com/thirdparties/1092 아이템 샵이라는 서드파티에서는 아이디 변경권이라는 포인트 아이템이 존재하고 있고 결국은 아이디는 변경될 수 있는 여지가 있습니다.

만약 지금 PR주신것처럼 바꾸게 된다면 이러한 서드파티를 잘 이용하고 있던 유저들은(심지어 유료이며 사용자들도 꾀있습니다.) 불편함을 겪을 수 있다는 것입니다.

실제로 XE함수 및 쿼리문을 이미 서드파티에서 많이 사용하고 있기도 합니다.

그래서 이런 변경점에 대해서 부정적인 시각을 가질 수 있습니다.

코어가 가지고 있던 원래 기본 기능이 변경되면 지금까지 쭉 유지되어왓던 사이트들에서 모두 문제가 발생되기 마련입니다.

따라서 PR이 코어에 적용되지 않는 것이 좋을 것 같아요.

hackmod commented 5 years ago

XE 게시판에서 서드파티 얘기만 나오면 논의가 급격하게 중단되거나 식어버리더군요.

궁금증이 생겨 더 찾아봤습니다. user_id 변경을 막은 PR입니다. 513077ffee 이때 왜 막았는지 이유를 생각해본다면 서드파티 프로그램에 임의로 user_id 변경을 하게 하는 것이 어떤 모양으로 지원되길래 유료인지 아닌지 모르겠으나, 그다지 바람직해보이지 않네요.

현재 운용중인 커뮤니티는, 닉네임 등을 사용자가 바꾸는 것도 몇개월에 한번씩만 고칠 수 있도록 제한하고 있는 상태입니다.

코어에서는 막혀있는데 서드파티 모듈에서 user_id를 고치는 것이 가능하다면 어떤 방식일까요? 저라면 queries/updateMember.xml를 서드파티 모듈에 복사해서 혹시 모를 core 업데이트시 오작동할 수 있는 여지를 제거했을 것 같습니다.

kijin commented 5 years ago

@hackmod

코어에서는 막혀있는데 서드파티 모듈에서 user_id를 고치는 것이 가능하다면 어떤 방식일까요?

말씀하셨다시피 executeQuery('member.updateMember') 하면 됩니다. 많은 서드파티 자료들이 코어의 쿼리를 직접 호출하고 있습니다.

XE 게시판에서 서드파티 얘기만 나오면 논의가 급격하게 중단되거나 식어버리더군요.

제3자가 멀쩡하게 잘 쓰고 있던 기능을 고장낼 가능성이 있는 패치라면 당연히 조심해야지요. 님 사이트에서 해당 기능을 쓰는지 안 쓰는지는 상관없습니다. XE는 님 혼자 쓰는 물건이 아니니까요.

서드파티 얘기가 나와도 논의가 끊기지 않도록 하는 방법을 알려드릴게요. 기존 유저들에게 영향을 주지 않도록 코드를 짜면 됩니다. 이를 위해 호환 레이어를 제공해야 할 수도 있고, 다양한 환경에서 장기간 테스트해야 할 수도 있고, 특정 옵션을 켰을 때만 새 기능이 작동하도록 해야 할 수도 있습니다. 예를 들어 #889 #894 는 다양한 상황에서의 호환성 문제를 고려하여 보완한 끝에 9개월만에 코어에 반영된 바 있습니다.

이 PR의 경우에는 updateMember.xml에서 user_id 부분의 notnull="notnull"을 제거하고, $args->user_id = $orgMemberInfo->user_id; 이것도 unset($args->user_id);로 변경하면 님이 원하시는 결과를 얻을 수 있을 것 같습니다. 코어의 작동 방식에도 변함이 없고, 쿼리를 직접 호출하는 서드파티 자료에도 영향을 주지 않고, user_id 컬럼에 update 권한을 줄 필요도 없게 되지요. 조금만 생각해 보면 서드파티 자료를 배려하는 것은 어렵지 않습니다.

hackmod commented 5 years ago

게다가 member_srl만 condition으로 주어도 충분히 레코드를 특정할 수 있는데 user_id는 왜 condition으로 옮기셨는지도 의문이군요. user_id를 PK로 사용하는 다른 CMS의 사고방식을 XE에 끌고 오신 것이 아닌가 싶습니다. 심지어 notnull 조건이 붙어 있어서 user_id를 넣어주지 않으면 쿼리가 아예 실행되지 않습니다. 이건 개선이 아니라 개악이네요.

member_srl 컨디션만으로도 유효한 쿼리이므로 user_id 컨디션에서 notnull을 빼봤습니다.

updateMember()를 호출하는 경우도 기존과 같은 결과. 문제 없음
서드파티 모듈이 user_id를 바꾸는 기능이 아닌 이상 정상 작동
executeQuery('member.updateMember')를 직접호출하는 경우 문제 발생 가능

서드파티 얘기가 나와도 논의가 끊기지 않도록 하는 방법을 알려드릴게요. 기존 유저들에게 영향을 주지 않도록 코드를 짜면 됩니다. 이를 위해 호환 레이어를 제공해야 할 수도 있고, 다양한 환경에서 장기간 테스트해야 할 수도 있고, 특정 옵션을 켰을 때만 새 기능이 작동하도록 해야 할 수도 있습니다. 예를 들어 #889 #894 는 다양한 상황에서의 호환성 문제를 고려하여 보완한 끝에 9개월만에 코어에 반영된 바 있습니다.

이 PR의 경우에는 updateMember.xml에서 user_id 부분의 notnull="notnull"을 제거하고, $args->user_id = $orgMemberInfo->user_id; 이것도 unset($args->user_id);로 변경하면 님이 원하시는 결과를 얻을 수 있을 것 같습니다. 코어의 작동 방식에도 변함이 없고, 쿼리를 직접 호출하는 서드파티 자료에도 영향을 주지 않고, user_id 컬럼에 update 권한을 줄 필요도 없게 되지요. 조금만 생각해 보면 서드파티 자료를 배려하는 것은 어렵지 않습니다.

흠.. 아래처럼 고치면 $user_id를 세팅하지 않았을 경우 update 컬럼에 포함되지는 않겠네요.

diff --git a/modules/member/queries/updateMember.xml b/modules/member/queries/updateMember.xml
index 71cc337..e49e10d 100644
--- a/modules/member/queries/updateMember.xml
+++ b/modules/member/queries/updateMember.xml
@@ -6,7 +6,7 @@
         <column name="password" var="password" notnull="notnull" />
         <column name="user_name" var="user_name" notnull="notnull" minlength="2" maxlength="40" />
         <column name="nick_name" var="nick_name" notnull="notnull" minlength="2" maxlength="40" />
-        <column name="user_id" var="user_id" notnull="notnull" />
+        <column name="user_id" var="user_id" />
                <column name="email_address" var="email_address" notnull="notnull"/>
                <column name="email_id" var="email_id" />
                <column name="email_host" var="email_host" />

kijin commented 5 years ago

흠.. 아래처럼 고치면 $user_id를 세팅하지 않았을 경우 update 컬럼에 포함되지는 않겠네요.

네, 기존의 쿼리에서 notnull만 빼고 member.controller.php에서 unset()해주면 코어와 서드파티의 작동 방식이 모두 기존과 동일하게 유지될 것 같습니다. 그러면 저도 반대할 명분이 없습니다.^^

컨디션 쪽으로 옮기셨던 user_id 조건은... 음... 코어에서 user_id로 회원을 특정하여 업데이트하는 기능을 지원하지 않는데요. 님의 논리에 따르면 코어에서 지원하지 않는 기능이니 해당 컨디션은 없어야 하는데, 왜 추가하셨는지 아직도 모르겠다는;;;

hackmod commented 5 years ago

흠.. 아래처럼 고치면 $user_id를 세팅하지 않았을 경우 update 컬럼에 포함되지는 않겠네요.

네, 기존의 쿼리에서 notnull만 빼고 member.controller.php에서 unset()해주면 코어와 서드파티의 작동 방식이 모두 기존과 동일하게 유지될 것 같습니다. 그러면 저도 반대할 명분이 없습니다.^^

컨디션 쪽으로 옮기셨던 user_id 조건은... 음... 코어에서 user_id로 회원을 특정하여 업데이트하는 기능을 지원하지 않는데요. 님의 논리에 따르면 코어에서 지원하지 않는 기능이니 해당 컨디션은 없어야 하는데, 왜 추가하셨는지 아직도 모르겠다는;;;

음.. user_id를 조건절에 넣는 경우 다음과 같은 효과가 있을 것이고, updateMember()와 xml 쿼리 레벨에서 작동이 일관성있게 작동된다고 본것이죠.

코어에서는 user_id 변경을 지원하지 않으니 => user_id가 조건절에 있건 없건 상관 없음. (updateMember()는 현재 user_id를 변경하려 해도 막혀있음. (다만 변경을 시도해도 오류를 내거나 하지는 않음))
executeQuery("member.updateMember")를 직접 호출하는 경우에 user_id 변경을 시도하면 쿼리 레벨의 오류가 남. (이 경우 서드파티 모듈 호환 안될 수 있음)

xpressengine / xe-core

move the user_id field to conditions #2405